| Message ID | 20241007221603.1703699-2-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | io_uring zero copy rx | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On Mon, Oct 7, 2024 at 3:16 PM David Wei <dw@davidwei.uk> wrote: > > From: Pavel Begunkov <asml.silence@gmail.com> > > Don't hide structure definitions under conditional compilation, it only > makes messier and harder to maintain. Move struct > dmabuf_genpool_chunk_owner definition out of CONFIG_NET_DEVMEM ifdef > together with a bunch of trivial inlined helpers using the structure. > To be honest I think the way it is is better? Having the struct defined but always not set (because the code to set it is always compiled out) seem worse to me. Is there a strong reason to have this? Otherwise maybe drop this?
On 10/9/24 21:17, Mina Almasry wrote: > On Mon, Oct 7, 2024 at 3:16 PM David Wei <dw@davidwei.uk> wrote: >> >> From: Pavel Begunkov <asml.silence@gmail.com> >> >> Don't hide structure definitions under conditional compilation, it only >> makes messier and harder to maintain. Move struct >> dmabuf_genpool_chunk_owner definition out of CONFIG_NET_DEVMEM ifdef >> together with a bunch of trivial inlined helpers using the structure. >> > > To be honest I think the way it is is better? Having the struct > defined but always not set (because the code to set it is always > compiled out) seem worse to me. > > Is there a strong reason to have this? Otherwise maybe drop this? I can drop it if there are strong opinions on that, but I'm allergic to ifdef hell and just trying to help to avoid it becoming so. I even believe it's considered a bad pattern (is it?). As for a more technical description "why", it reduces the line count and you don't need to duplicate functions. It's always annoying making sure the prototypes stay same, but this way it's always compiled and syntactically checked. And when refactoring anything like the next patch does, you only need to change one function but not both. Do you find that convincing?
On Wed, Oct 9, 2024 at 4:16 PM Pavel Begunkov <asml.silence@gmail.com> wrote: > > On 10/9/24 21:17, Mina Almasry wrote: > > On Mon, Oct 7, 2024 at 3:16 PM David Wei <dw@davidwei.uk> wrote: > >> > >> From: Pavel Begunkov <asml.silence@gmail.com> > >> > >> Don't hide structure definitions under conditional compilation, it only > >> makes messier and harder to maintain. Move struct > >> dmabuf_genpool_chunk_owner definition out of CONFIG_NET_DEVMEM ifdef > >> together with a bunch of trivial inlined helpers using the structure. > >> > > > > To be honest I think the way it is is better? Having the struct > > defined but always not set (because the code to set it is always > > compiled out) seem worse to me. > > > > Is there a strong reason to have this? Otherwise maybe drop this? > I can drop it if there are strong opinions on that, but I'm > allergic to ifdef hell and just trying to help to avoid it becoming > so. I even believe it's considered a bad pattern (is it?). > > As for a more technical description "why", it reduces the line count > and you don't need to duplicate functions. It's always annoying > making sure the prototypes stay same, but this way it's always > compiled and syntactically checked. And when refactoring anything > like the next patch does, you only need to change one function > but not both. Do you find that convincing? > To be honest the tradeoff wins in the other direction for me. The extra boiler plate is not that bad, and we can be sure that any code that touches net_devmem_dmabuf_binding will get a valid internals since it won't compile if the feature is disabled. This could be critical and could be preventing bugs.
On 10/10/24 19:01, Mina Almasry wrote: > On Wed, Oct 9, 2024 at 4:16 PM Pavel Begunkov <asml.silence@gmail.com> wrote: >> >> On 10/9/24 21:17, Mina Almasry wrote: >>> On Mon, Oct 7, 2024 at 3:16 PM David Wei <dw@davidwei.uk> wrote: >>>> >>>> From: Pavel Begunkov <asml.silence@gmail.com> >>>> >>>> Don't hide structure definitions under conditional compilation, it only >>>> makes messier and harder to maintain. Move struct >>>> dmabuf_genpool_chunk_owner definition out of CONFIG_NET_DEVMEM ifdef >>>> together with a bunch of trivial inlined helpers using the structure. >>>> >>> >>> To be honest I think the way it is is better? Having the struct >>> defined but always not set (because the code to set it is always >>> compiled out) seem worse to me. >>> >>> Is there a strong reason to have this? Otherwise maybe drop this? >> I can drop it if there are strong opinions on that, but I'm >> allergic to ifdef hell and just trying to help to avoid it becoming >> so. I even believe it's considered a bad pattern (is it?). >> >> As for a more technical description "why", it reduces the line count >> and you don't need to duplicate functions. It's always annoying >> making sure the prototypes stay same, but this way it's always >> compiled and syntactically checked. And when refactoring anything >> like the next patch does, you only need to change one function >> but not both. Do you find that convincing? >> > > To be honest the tradeoff wins in the other direction for me. The > extra boiler plate is not that bad, and we can be sure that any code We can count how often people break builds because a change was compiled just with one configuration in mind. Unfortunately, I did it myself a fair share of times, and there is enough of build robot reports like that. It's not just about boiler plate but rather overall maintainability. > that touches net_devmem_dmabuf_binding will get a valid internals > since it won't compile if the feature is disabled. This could be > critical and could be preventing bugs. I don't see the concern, if devmem is compiled out there wouldn't be a devmem provider to even create it, and you don't need to worry. If you think someone would create a binding without a devmem, then I don't believe it'd be enough to hide a struct definition to prevent that in the first place. I think the maintainers can tell whichever way they think is better, I can drop the patch, even though I think it's much better with it.
On 10/10/24 19:57, Pavel Begunkov wrote: > On 10/10/24 19:01, Mina Almasry wrote: ... >> >> To be honest the tradeoff wins in the other direction for me. The >> extra boiler plate is not that bad, and we can be sure that any code > > We can count how often people break builds because a change > was compiled just with one configuration in mind. Unfortunately, > I did it myself a fair share of times, and there is enough of > build robot reports like that. It's not just about boiler plate > but rather overall maintainability. > >> that touches net_devmem_dmabuf_binding will get a valid internals >> since it won't compile if the feature is disabled. This could be >> critical and could be preventing bugs. > > I don't see the concern, if devmem is compiled out there wouldn't > be a devmem provider to even create it, and you don't need to > worry. If you think someone would create a binding without a devmem, > then I don't believe it'd be enough to hide a struct definition > to prevent that in the first place. > > I think the maintainers can tell whichever way they think is > better, I can drop the patch, even though I think it's much > better with it. Having a second thought, I'll drop the patch as asked. The change is not essential to the series, I shouldn't care about devmem here.
diff --git a/net/core/devmem.h b/net/core/devmem.h index 76099ef9c482..cf66e53b358f 100644 --- a/net/core/devmem.h +++ b/net/core/devmem.h @@ -44,7 +44,6 @@ struct net_devmem_dmabuf_binding { u32 id; }; -#if defined(CONFIG_NET_DEVMEM) /* Owner of the dma-buf chunks inserted into the gen pool. Each scatterlist * entry from the dmabuf is inserted into the genpool as a chunk, and needs * this owner struct to keep track of some metadata necessary to create @@ -64,16 +63,6 @@ struct dmabuf_genpool_chunk_owner { struct net_devmem_dmabuf_binding *binding; }; -void __net_devmem_dmabuf_binding_free(struct net_devmem_dmabuf_binding *binding); -struct net_devmem_dmabuf_binding * -net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd, - struct netlink_ext_ack *extack); -void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding); -int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, - struct net_devmem_dmabuf_binding *binding, - struct netlink_ext_ack *extack); -void dev_dmabuf_uninstall(struct net_device *dev); - static inline struct dmabuf_genpool_chunk_owner * net_iov_owner(const struct net_iov *niov) { @@ -91,6 +80,11 @@ net_iov_binding(const struct net_iov *niov) return net_iov_owner(niov)->binding; } +static inline u32 net_iov_binding_id(const struct net_iov *niov) +{ + return net_iov_owner(niov)->binding->id; +} + static inline unsigned long net_iov_virtual_addr(const struct net_iov *niov) { struct dmabuf_genpool_chunk_owner *owner = net_iov_owner(niov); @@ -99,10 +93,18 @@ static inline unsigned long net_iov_virtual_addr(const struct net_iov *niov) ((unsigned long)net_iov_idx(niov) << PAGE_SHIFT); } -static inline u32 net_iov_binding_id(const struct net_iov *niov) -{ - return net_iov_owner(niov)->binding->id; -} +#if defined(CONFIG_NET_DEVMEM) + +void __net_devmem_dmabuf_binding_free(struct net_devmem_dmabuf_binding *binding); +struct net_devmem_dmabuf_binding * +net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd, + struct netlink_ext_ack *extack); +void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding); +int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, + struct net_devmem_dmabuf_binding *binding, + struct netlink_ext_ack *extack); +void dev_dmabuf_uninstall(struct net_device *dev); + static inline void net_devmem_dmabuf_binding_get(struct net_devmem_dmabuf_binding *binding) @@ -124,8 +126,6 @@ net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding); void net_devmem_free_dmabuf(struct net_iov *ppiov); #else -struct net_devmem_dmabuf_binding; - static inline void __net_devmem_dmabuf_binding_free(struct net_devmem_dmabuf_binding *binding) { @@ -165,16 +165,6 @@ net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding) static inline void net_devmem_free_dmabuf(struct net_iov *ppiov) { } - -static inline unsigned long net_iov_virtual_addr(const struct net_iov *niov) -{ - return 0; -} - -static inline u32 net_iov_binding_id(const struct net_iov *niov) -{ - return 0; -} #endif #endif /* _NET_DEVMEM_H */