On Tue, Sep 3, 2024 at 2:19 PM Jakub Kicinski [off-list ref] wrote:

On Sat, 31 Aug 2024 00:43:06 +0000 Mina Almasry wrote:

quoted

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.

quoted

+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.

quoted

      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.

quoted

+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