Re: [PATCH net-next v2 1/3] net: devmem: replace gen_pool with freelist
From: Mina Almasry <hidden>
Date: 2026-09-14 21:47:12
Also in:
lkml
On Fri, Sep 11, 2026 at 8:45 AM Stanislav Fomichev [off-list ref] wrote:
devmem only needs fixed-size net_iov allocations for each dma-buf binding. The gen_pool tracks the same free set indirectly through DMA addresses, which makes devmem depend on the generic allocator even though the users are fixed-size net_iov chunks. Mirror the io_uring zcrx model more closely by keeping a binding-level freelist protected by spin_lock_bh(). Use a single net_iov_area owner for the binding, populate each net_iov's DMA address while walking the SG table, and check at teardown that all net_iovs have returned to the freelist. Drop the NET_DEVMEM select of GENERIC_ALLOCATOR now that devmem no longer calls gen_pool APIs. Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
Approach is great, some suggested improvements.
quoted hunk ↗ jump to hunk
--- net/Kconfig | 1 - net/core/devmem.c | 172 +++++++++++++++++++++------------------------- net/core/devmem.h | 16 ++--- 3 files changed, 85 insertions(+), 104 deletions(-)diff --git a/net/Kconfig b/net/Kconfig index e38477393551..76ab44aa439a 100644 --- a/net/Kconfig +++ b/net/Kconfig@@ -68,7 +68,6 @@ config SKB_EXTENSIONS config NET_DEVMEM def_bool y - select GENERIC_ALLOCATOR depends on DMA_SHARED_BUFFER depends on PAGE_POOLdiff --git a/net/core/devmem.c b/net/core/devmem.c index f4d60654ce7f..4883eb7f3a95 100644 --- a/net/core/devmem.c +++ b/net/core/devmem.c@@ -8,7 +8,6 @@ */ #include <linux/dma-buf.h> -#include <linux/genalloc.h> #include <linux/mm.h> #include <linux/netdevice.h> #include <linux/types.h>@@ -30,23 +29,13 @@ static DEFINE_XARRAY_FLAGS(net_devmem_dmabuf_bindings, XA_FLAGS_ALLOC1); static const struct memory_provider_ops dmabuf_devmem_ops; -static void net_devmem_dmabuf_free_chunk_owner(struct gen_pool *genpool, - struct gen_pool_chunk *chunk, - void *not_used) +static void +net_devmem_dmabuf_free_chunk_owner(struct dmabuf_genpool_chunk_owner *owner) { - struct dmabuf_genpool_chunk_owner *owner = chunk->owner; - - kvfree(owner->area.niovs); - kfree(owner); -} - -static dma_addr_t net_devmem_get_dma_addr(const struct net_iov *niov) -{ - struct dmabuf_genpool_chunk_owner *owner; - - owner = net_devmem_iov_to_chunk_owner(niov); - return owner->base_dma_addr + - ((dma_addr_t)net_iov_idx(niov) << owner->binding->niov_shift); + if (owner) { + kvfree(owner->area.niovs); + kfree(owner); + } } static void net_devmem_dmabuf_binding_release(struct percpu_ref *ref)@@ -62,24 +51,18 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq) { struct net_devmem_dmabuf_binding *binding = container_of(wq, typeof(*binding), unbind_w); - size_t size, avail; - - gen_pool_for_each_chunk(binding->chunk_pool, - net_devmem_dmabuf_free_chunk_owner, NULL); - - size = gen_pool_size(binding->chunk_pool); - avail = gen_pool_avail(binding->chunk_pool); - - if (!WARN(size != avail, "can't destroy genpool. size=%zu, avail=%zu", - size, avail)) - gen_pool_destroy(binding->chunk_pool); + WARN(binding->free_count != binding->total_niovs, + "can't destroy dmabuf binding. total=%zu, free=%zu", + binding->total_niovs, binding->free_count);
You're warning here that you can't destroy the dmabuf binding but you're destroying it anyway. Something is off here. Do we want an early return or something else?
quoted hunk ↗ jump to hunk
+ net_devmem_dmabuf_free_chunk_owner(binding->chunk_owner); dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt, binding->direction); dma_buf_detach(binding->dmabuf, binding->attachment); dma_buf_put(binding->dmabuf); xa_destroy(&binding->bound_rxqs); percpu_ref_exit(&binding->ref); + kvfree(binding->freelist); kvfree(binding->tx_vec); kfree(binding); }@@ -87,21 +70,16 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq) struct net_iov * net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding) { - struct dmabuf_genpool_chunk_owner *owner; - unsigned long dma_addr; struct net_iov *niov; - ssize_t offset; - ssize_t index; - - dma_addr = gen_pool_alloc_owner(binding->chunk_pool, - 1UL << binding->niov_shift, - (void **)&owner); - if (!dma_addr) + spin_lock_bh(&binding->freelist_lock); + if (unlikely(!binding->free_count)) { + spin_unlock_bh(&binding->freelist_lock); return NULL; + } - offset = dma_addr - owner->base_dma_addr; - index = offset >> binding->niov_shift; - niov = &owner->area.niovs[index]; + niov = binding->freelist[--binding->free_count]; + binding->freelist[binding->free_count] = NULL;
The LLM thinks this NULL store in unnecassary. IDK if it will help anything in practice to remove it :-)
quoted hunk ↗ jump to hunk
+ spin_unlock_bh(&binding->freelist_lock); niov->desc.pp_magic = 0; niov->desc.pp = NULL;@@ -113,14 +91,15 @@ net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding) void net_devmem_free_dmabuf(struct net_iov *niov) { struct net_devmem_dmabuf_binding *binding = net_devmem_iov_binding(niov); - unsigned long dma_addr = net_devmem_get_dma_addr(niov); - size_t niov_size = 1UL << binding->niov_shift; - if (WARN_ON(!gen_pool_has_addr(binding->chunk_pool, dma_addr, - niov_size))) + spin_lock_bh(&binding->freelist_lock); + if (WARN_ON_ONCE(binding->free_count >= binding->total_niovs)) { + spin_unlock_bh(&binding->freelist_lock); return; + } - gen_pool_free(binding->chunk_pool, dma_addr, niov_size); + binding->freelist[binding->free_count++] = niov; + spin_unlock_bh(&binding->freelist_lock); } void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)@@ -194,12 +173,15 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev, struct netlink_ext_ack *extack) { struct net_devmem_dmabuf_binding *binding; + struct dmabuf_genpool_chunk_owner *owner; size_t niov_size = 1UL << niov_shift; static u32 id_alloc_next; struct scatterlist *sg; struct dma_buf *dmabuf; - unsigned int sg_idx, i; - unsigned long virtual; + unsigned int sg_idx; + size_t total_niovs; + size_t niov_idx; + size_t i; int err; if (!dma_dev) {@@ -230,6 +212,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev, goto err_free_binding; mutex_init(&binding->lock); + spin_lock_init(&binding->freelist_lock);
We don't need freelists on tx right? We should probably not allocate them then?
quoted hunk ↗ jump to hunk
binding->dmabuf = dmabuf; binding->direction = direction;@@ -262,20 +245,10 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev, goto err_unmap; } } - - binding->chunk_pool = gen_pool_create(niov_shift, - dev_to_node(&dev->dev)); - if (!binding->chunk_pool) { - err = -ENOMEM; - goto err_tx_vec; - } - - virtual = 0; + total_niovs = 0;
Do we really need a secondary for_each_sgtable_dma_sg loop just to calculate the total_niovs? In what edge case is the total_niovs not just dmabuf_len / niov_len? We do a bunch of alignment checks to make sure it all works out to that no?
quoted hunk ↗ jump to hunk
for_each_sgtable_dma_sg(binding->sgt, sg, sg_idx) { dma_addr_t dma_addr = sg_dma_address(sg); - struct dmabuf_genpool_chunk_owner *owner; size_t len = sg_dma_len(sg); - struct net_iov *niov; if (!IS_ALIGNED(dma_addr, niov_size) || !IS_ALIGNED(len, niov_size)) {@@ -283,63 +256,74 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev, NL_SET_ERR_MSG_FMT(extack, "dmabuf sg entry (addr=%pad, len=%zu) not aligned to niov size %zu", &dma_addr, len, niov_size); - goto err_free_chunks; + goto err_tx_vec; } - owner = kzalloc_node(sizeof(*owner), GFP_KERNEL, - dev_to_node(&dev->dev)); - if (!owner) { - err = -ENOMEM; - goto err_free_chunks; - } + total_niovs += len >> niov_shift; + } - owner->area.base_virtual = virtual; - owner->base_dma_addr = dma_addr; - owner->area.num_niovs = len >> niov_shift; - owner->binding = binding; + binding->freelist = kvmalloc_array(total_niovs, + sizeof(binding->freelist[0]), + GFP_KERNEL); + if (!binding->freelist) { + err = -ENOMEM; + goto err_tx_vec; + } + binding->total_niovs = total_niovs;
binding->total_niovs and binding->area.num_niovs seem the same thing always. please get rid of one, probably binding->total_niovs. I wonder if now that both zcrx and devmem use a freelist if the freelist should be part of the net_iov_area. The point of that field was to hold the common stuff actually, but I'm guessing there are micro-implementation differences that will make converging annoying. I'm fine either way. :shrug:
- err = gen_pool_add_owner(binding->chunk_pool, dma_addr,
- dma_addr, len, dev_to_node(&dev->dev),
- owner);
- if (err) {
- kfree(owner);
- err = -EINVAL;
- goto err_free_chunks;
- }
+ owner = kzalloc_node(sizeof(*owner), GFP_KERNEL,
+ dev_to_node(&dev->dev));
+ if (!owner) {
+ err = -ENOMEM;
+ goto err_free_freelist;
+ }
- owner->area.niovs = kvmalloc_objs(*owner->area.niovs,
- owner->area.num_niovs);
- if (!owner->area.niovs) {
- err = -ENOMEM;
- goto err_free_chunks;
- }
+ owner->area.num_niovs = total_niovs;
+ owner->binding = binding;
+ owner->area.niovs = kvmalloc_objs(*owner->area.niovs,
+ owner->area.num_niovs);
+ if (!owner->area.niovs) {
+ err = -ENOMEM;
+ goto err_free_owner;
+ }
+ binding->chunk_owner = owner;
+
+ niov_idx = 0;
+ for_each_sgtable_dma_sg(binding->sgt, sg, sg_idx) {
+ dma_addr_t dma_addr = sg_dma_address(sg);
+ size_t len = sg_dma_len(sg);len is referenced once now; not worth a local var.
+ struct net_iov *niov;
+ size_t nr_niovs = len >> niov_shift;
- for (i = 0; i < owner->area.num_niovs; i++) {
- niov = &owner->area.niovs[i];
+ for (i = 0; i < nr_niovs; i++, niov_idx++) {
+ niov = &owner->area.niovs[niov_idx];
net_iov_init(niov, &owner->area, NET_IOV_DMABUF);
page_pool_set_dma_addr_netmem(net_iov_to_netmem(niov),
- net_devmem_get_dma_addr(niov));
+ dma_addr);
if (direction == DMA_TO_DEVICE)
- binding->tx_vec[owner->area.base_virtual / PAGE_SIZE + i] = niov;
+ binding->tx_vec[niov_idx] = niov;
+ binding->freelist[binding->free_count++] = niov;if feels somewhat simple to exclude freelist from TX. Something like: if (direction == dma_to_device) <store into tx_vec> else <store in freelist> -- Thanks, Mina