On Tue, Sep 3, 2024 at 2:19 PM Jakub Kicinski kuba@kernel.org 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