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

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 08/31, Mina Almasry wrote:
> On Mon, Aug 31, 2026 at 11:35 AM Stanislav Fomichev
> <sdf.kernel@gmail.com> wrote:
> >
> > Replace devmem's gen_pool based fixed-size allocator with a binding-level
> > freelist similar to the one used by io_uring zero-copy receive.
> >
> > This is motivated by allocation latency observed in the NAPI receive path:
> >
> >   [ 1036.228913]  ? gen_pool_create+0x90/0x90
> >   [ 1036.228915]  net_devmem_alloc_dmabuf+0x1f/0x60
> >   [ 1036.228918]  mp_dmabuf_devmem_alloc_netmems+0x17/0x80
> >   [ 1036.228920]  mlx5e_post_rx_mpwqes+0xdbe/0xdd0
> >   [ 1036.228926]  mlx5e_napi_poll+0x113/0x830
> >   [ 1036.228928]  ? sched_clock+0x5/0x10
> >   [ 1036.228931]  ? wake_up_process+0x778/0x14b0
> >   [ 1036.228933]  net_rx_action+0x15d/0x570
> >   [ 1036.228934]  ? update_rq_clock+0x31/0x240
> >   [ 1036.228937]  ? __napi_schedule+0x55/0xa0
> >   [ 1036.228938]  ? mlx5_eq_comp_int+0x137/0x230
> >   [ 1036.228940]  ? atomic_notifier_call_chain+0x36/0x90
> >   [ 1036.228943]  ? sched_clock+0x5/0x10
> >   [ 1036.228944]  ? sched_clock_cpu+0xc/0x170
> >   [ 1036.228947]  irq_exit_rcu+0x12b/0x370
> >   [ 1036.228950]  common_interrupt+0x85/0x90
> >
> > udmabuf can create a very large number of SG entries. In the worst case,
> > devmem ends up adding one gen_pool chunk for each net_iov allocation
> > unit backed by those entries. The gen_pool allocation path then has to
> > traverse a linked list that can become too long for this hot path.
> >
> > Patch 1 removes the gen_pool and replaces it with a simple freelist of
> > net_iov pointers protected by the same spin_lock_bh() pattern used by
> > io_uring zcrx. Patch 2 removes the now-unnecessary chunk owner wrapper by
> > embedding the net_iov_area directly in the dma-buf binding.
> >
> 
> Oh boy, this is going to be a bit tricky.
> 
> I ran into this exact horrible perf bug (sorry for it in the first
> place), but my solution was different. My solution [1] was to coalesce
> the SG entries that are contigious (and they usually are in practice),
> and I got 'acceptable' perf after that. Kaifeng is actually working on
> cleaning up my hacky patch up to send it upstream now.

As you mention, coalescing might happen to work or it might not :-(
I'd like us to have something that's less probabilistic.

> 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)
 {
 }


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