On Tue, Sep 3, 2024 at 2:19 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > On Sat, 31 Aug 2024 00:43:06 +0000 Mina Almasry wrote: > > diff --git a/include/net/mp_dmabuf_devmem.h b/include/net/mp_dmabuf_devmem.h > > new file mode 100644 > > index 000000000000..6d1cf2a77f6b > > --- /dev/null > > +++ b/include/net/mp_dmabuf_devmem.h > > this header can live under net/core/ like netmem_priv.h right? > devmem internals should be of no interest outside of core networking. > Yes, those can be moved under net/core trivially. done. > In fact the same is true for include/net/devmem.h ? > This turned out to be possible, but with a minor moving around of some helpers. Basically netmem.h included devmem.h to get access to some devmem internals for some of the net_iov helpers specific to devmem. Moving these helpers to devmem.h enabled me to keep include/net/netmem.h but put devmem.h under net/core. Now netmem.h doesn't need to include devmem.h. I think this is an improvement. > > +static inline netmem_ref mp_dmabuf_devmem_alloc_netmems(struct page_pool *pool, > > + gfp_t gfp) > > Please break the lines after the return type if the line gets long: > > static inline netmem_ref > mp_dmabuf_devmem_alloc_netmems(struct page_pool *pool, gfp_t gfp) > > Please fix where you can (at least where it cases going over 80 chars) > FWIW I use a formatting tool (clang-format) which seems to prefer breaking in between the args, but I'll fix this manually and wherever else I notice. > > struct_group_tagged(page_pool_params_slow, slow, > > struct net_device *netdev; > > + struct netdev_rx_queue *queue; > > Why set a pointer? It should work but drivers don't usually deal with > netdev_rx_queue struct directly. struct xdp_rxq_info takes an integer > queue id, and it serves a somewhat similar function. > > Keep in mind that there will be more drivers than core code, so > convenience for them matters more. > Makes sense. > > +bool mp_dmabuf_devmem_release_page(struct page_pool *pool, netmem_ref netmem) > > +{ > > + if (WARN_ON_ONCE(!netmem_is_net_iov(netmem))) > > + return false; > > + > > + if (WARN_ON_ONCE(atomic_long_read(netmem_get_pp_ref_count_ref(netmem)) != > > + 1)) > > something needs factoring out here, to make this line shorter, please.. > either netmem -> net_iov conversion or at least reading of the ref > count? > Ah, sorry I think you pointed this out earlier and I missed applying it. Should be done in the next iteration. -- Thanks, Mina