Re: [PATCH net-next 0/2] net: devmem: remove gen_pool from dma-buf allocations

quoted

Now I don't know which approach is better. Thinking about the pros and
cons of your approach:

+ your approach is much simpler, and removes gen_pool overheads for a
single queue case. It should be (much?) faster for that case.
- your approach adds a lock and allocations from multiple queues in
parallel will contend on this lock. There should be some value of # of
queues N where your approach starts to completely trash. gen_pool is
lockless so I wouldn't expect it to degrade significantly in the
multi-queue case.

The question for me is what the performance is for a real use case
(NCCL all-to-all for example) over a realistic number of shared queues
(it's 4-8 for me). I need that perf data to be honest before judging
this.

The io_uring zcrx comparision is not completely valid. io_uring zcrx
is built from the ground up to be one-buffer-is-bound-to-one-rx-queue,
and devmem tcp is built from the ground up to be
one-buffer-can-be-bound-to-N-rx-queues.

What if we add batching similar to io_pp_zc_alloc_netmems? So we don't
have to spin lock on every netmem (and move PP_ALLOC_CACHE_REFILL-worth
of chunks). Untested, on top of this series:

diff --git a/net/core/devmem.c b/net/core/devmem.c
index 84d6c30516c8..5a1c996ba515 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c

@@ -58,25 +58,25 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
        kfree(binding);
 }

-struct net_iov *
-net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding)
+static unsigned int
+net_devmem_alloc_dmabuf_bulk(struct net_devmem_dmabuf_binding *binding,
+                            netmem_ref *netmems, unsigned int count)
 {
-       struct net_iov *niov;
+       unsigned int i;
+
        spin_lock_bh(&binding->freelist_lock);
-       if (unlikely(!binding->free_count)) {
-               spin_unlock_bh(&binding->freelist_lock);
-               return NULL;
+
+       count = min_t(size_t, count, binding->free_count);
+       for (i = 0; i < count; i++) {
+               struct net_iov *niov = binding->freelist[--binding->free_count];
+
+               binding->freelist[binding->free_count] = NULL;
+               netmems[i] = net_iov_to_netmem(niov);
        }

-       niov = binding->freelist[--binding->free_count];
-       binding->freelist[binding->free_count] = NULL;
        spin_unlock_bh(&binding->freelist_lock);

-       niov->desc.pp_magic = 0;
-       niov->desc.pp = NULL;
-       atomic_long_set(&niov->desc.pp_ref_count, 0);
-
-       return niov;
+       return count;
 }

 void net_devmem_free_dmabuf(struct net_iov *niov)

@@ -433,20 +433,35 @@ int mp_dmabuf_devmem_init(struct page_pool *pool)
 netmem_ref mp_dmabuf_devmem_alloc_netmems(struct page_pool *pool, gfp_t gfp)
 {
        struct net_devmem_dmabuf_binding *binding = pool->mp_priv;
-       struct net_iov *niov;
-       netmem_ref netmem;
+       netmem_ref *netmems = pool->alloc.cache;
+       unsigned int allocated, i;
+
+       if (WARN_ON_ONCE(pool->alloc.count))
+               return 0;

-       niov = net_devmem_alloc_dmabuf(binding);
-       if (!niov)
+       allocated = net_devmem_alloc_dmabuf_bulk(binding, netmems,
+                                                PP_ALLOC_CACHE_REFILL);
+       if (unlikely(!allocated))
                return 0;

-       netmem = net_iov_to_netmem(niov);
+       for (i = 0; i < allocated; i++) {
+               struct net_iov *niov = netmem_to_net_iov(netmems[i]);

-       page_pool_set_pp_info(pool, netmem);
+               niov->desc.pp_magic = 0;
+               niov->desc.pp = NULL;
+               atomic_long_set(&niov->desc.pp_ref_count, 0);
+
+               page_pool_set_pp_info(pool, netmems[i]);
+
+               pool->pages_state_hold_cnt++;
+               trace_page_pool_state_hold(pool, netmems[i],
+                                          pool->pages_state_hold_cnt);
+       }

-       pool->pages_state_hold_cnt++;
-       trace_page_pool_state_hold(pool, netmem, pool->pages_state_hold_cnt);
-       return netmem;
+       /* Return the last one, the rest stay in the page_pool cache. */
+       allocated--;
+       pool->alloc.count = allocated;
+       return netmems[allocated];
 }

 void mp_dmabuf_devmem_destroy(struct page_pool *pool)

diff --git a/net/core/devmem.h b/net/core/devmem.h
index 20a3eb90ea7f..7195769b8bd1 100644
--- a/net/core/devmem.h
+++ b/net/core/devmem.h

@@ -133,8 +133,6 @@ net_devmem_dmabuf_binding_put(struct net_devmem_dmabuf_binding *binding)
 void net_devmem_get_net_iov(struct net_iov *niov);
 void net_devmem_put_net_iov(struct net_iov *niov);

-struct net_iov *
-net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding);
 void net_devmem_free_dmabuf(struct net_iov *ppiov);

@@ -191,12 +189,6 @@ net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
        return -EOPNOTSUPP;
 }

-static inline struct net_iov *
-net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding)
-{
-       return NULL;
-}
-
 static inline void net_devmem_free_dmabuf(struct net_iov *ppiov)
 {
 }

quoted

Are you able to get NCCL all-to-all tests for N=4/8 yourself?
Otherwise please wait for me to backport this to my release kernel and
test it. ETA sometime this week, I hope.

I can definitely wait for you to access the perf impact on your side. Wonder
if we need to have a selftest to do that properly in NIPA. Doesn't have to
be a red/green signal, but some number for humans to compare
before-after (specifically this bind one dmabuf to multiple queues and pass
a lot of traffic). I can probably sketch something..