Re: [PATCH net-next v2 1/3] net: devmem: replace gen_pool with freelist

[Mina Almasry]

On Fri, Sep 11, 2026 at 8:45 AM Stanislav Fomichev <sdf.kernel@xxxxxxxxx> 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@xxxxxxxxxxx>

Approach is great, some suggested improvements.

> ---
>  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_POOL
>
> diff --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?

> +       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 :-)

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

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

>         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