Re: [PATCH net-next v1 04/11] xsk: Allocate UMEM chunks through page_pool

["Björn Töpel" <bjorn@kernel.org>]

Magnus Karlsson <magnus.karlsson@gmail.com> writes:

> On Tue, 10 Nov 2026 at 12:26, Björn Töpel <bjorn@kernel.org> wrote:

>> diff --git a/net/xdp/xsk.h b/net/xdp/xsk.h
>> index 7c811b5cce76..4dab7b9a1487 100644
>> --- a/net/xdp/xsk.h
>> +++ b/net/xdp/xsk.h
>> @@ -34,6 +34,12 @@ static inline struct xdp_sock *xdp_sk(struct sock *sk)
>>
>>  void xsk_map_try_sock_delete(struct xsk_map *map, struct xdp_sock *xs,
>>                              struct xdp_sock __rcu **map_entry);
>> +struct xsk_pp;
>
> How about a header file instead of a forward declaration?

Thanks for taking a look, Magnus!

Hmm, my take was this this was ont by itself sufficient for a header.
Let me think about it, and I might add net/xdp/xsk_pp.h.

>
>> +extern const struct memory_provider_ops xsk_pp_ops;
>> +
>> +struct xsk_pp *xsk_pp_create(struct xsk_buff_pool *pool);
>> +void xsk_pp_put(struct xsk_pp *pp);
>>  void xsk_clear_pool_at_qid(struct net_device *dev, u16 queue_id);
>>  int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool,
>>                         u16 queue_id);
>> diff --git a/net/xdp/xsk_pp.c b/net/xdp/xsk_pp.c
>> new file mode 100644
>> index 000000000000..fdd55265a6cd
>> --- /dev/null
>> +++ b/net/xdp/xsk_pp.c
>> @@ -0,0 +1,246 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* AF_XDP UMEM as a page pool memory provider
>> + *
>> + * Each aligned UMEM chunk is a readable net_iov with one owner at a time:
>> + *
>> + *   userspace --FILL--> page pool --release--> reuse --alloc--> page pool
>> + *       ^                   |
>> + *       +-----RX ring-------+
>> + */
>> +
>> +#include <net/page_pool/helpers.h>
>> +#include <net/page_pool/memory_provider.h>
>> +#include <net/xdp_sock_drv.h>
>> +
>> +#include "xsk.h"
>> +#include "xsk_queue.h"
>> +
>> +enum xsk_pp_chunk {
>> +       XSK_PP_USER,    /* owned by userspace */
>> +       XSK_PP_POOL,    /* owned by a page pool */
>> +       XSK_PP_REUSE,   /* on the reuse stack */
>> +};
>> +
>> +struct xsk_pp {
>> +       struct net_iov_area area;
>> +       struct xsk_buff_pool *pool;
>> +       /* Protects fq, state and reuse */
>> +       spinlock_t lock;
>> +       struct xsk_queue *fq;
>> +       refcount_t refs;
>> +       u32 nr_pools;
>> +       bool dma_need_sync;
>> +       u8 *state;
>> +       u32 reuse_cnt;
>> +       u32 reuse[];
>
> Is the reason that you have a reuse stack here and not use the one in
> xsk_buff_pool that it only exists in unaligned mode, which you are not
> supporting? Could you still use the "pre-calculated" heads in aligned
> mode and link them up? There is already a xskb_list and free_list in
> there that is used for that purpose. Just an idea that could cut down
> the state and maybe the amount of code.


Not quite. reuse is the counterpart of free_list, which aligned mode has
too. But free_list links xdp_buff_xsk, and this mode has none: the page
pool hands out net_iovs, and the driver has its own xdp_buff. Patch 5
stops allocating the heads here: 128 bytes per chunk, against 69 for
net_iov, reuse and state. devmem and zcrx use the same kind of index
stack.

free_list is also NAPI only, while the page pool releases chunks from
process context too (below). And state is needed either way; it is what
drops a FILL entry for a chunk the kernel still owns.


>> +};
>> +
>> +static void xsk_pp_free(struct xsk_pp *pp)
>> +{
>> +       kvfree(pp->state);
>> +       kvfree(pp->area.niovs);
>> +       kvfree(pp);
>> +}
>> +
>> +void xsk_pp_put(struct xsk_pp *pp)
>> +{
>> +       if (refcount_dec_and_test(&pp->refs))
>> +               xsk_pp_free(pp);
>> +}
>> +
>> +/* @pool must be DMA mapped and have aligned chunks. The provider must be
>> + * the only consumer of its FILL ring.
>> + */
>> +struct xsk_pp *xsk_pp_create(struct xsk_buff_pool *pool)
>> +{
>> +       struct net_iov *niov;
>> +       struct xsk_pp *pp;
>> +       dma_addr_t dma;
>> +       u32 i;
>> +
>> +       pp = kvzalloc_flex(*pp, reuse, pool->heads_cnt, GFP_KERNEL_ACCOUNT);
>> +       if (!pp)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       pp->area.niovs = kvzalloc_objs(*pp->area.niovs, pool->heads_cnt,
>> +                                      GFP_KERNEL_ACCOUNT);
>> +       pp->state = kvzalloc(pool->heads_cnt, GFP_KERNEL_ACCOUNT);
>> +       if (!pp->area.niovs || !pp->state) {
>> +               xsk_pp_free(pp);
>> +               return ERR_PTR(-ENOMEM);
>> +       }
>> +
>> +       pp->area.num_niovs = pool->heads_cnt;
>> +       pp->area.niov_shift = pool->chunk_shift;
>> +       pp->area.vaddr = pool->addrs;
>> +       pp->pool = pool;
>> +       pp->fq = pool->fq;
>> +       pp->dma_need_sync = dma_dev_need_sync(pool->dev);
>
> Are you copying these for performance reasons, i.e. no extra pointer
> dereference in the fast path? In the same manner as the pool copies
> some information from the umem?

Only dma_need_sync is a cache -- I'll drop that!

fq stays. It's the provider's handle on FILL: unbind clears it under the
lock (patch 5), so the provider stops reading FILL, while pool->fq stays
with the socket for mmap, need_wakeup and copy mode until the ring is
freed. The area fields are the generic net_iov_area that
netmem_address() needs.

Does that make sense?

>> +       spin_lock_init(&pp->lock);
>> +       refcount_set(&pp->refs, 1);
>> +
>> +       for (i = 0; i < pp->area.num_niovs; i++) {
>> +               niov = &pp->area.niovs[i];
>> +               dma = xp_raw_get_dma(pool, (u64)i << pool->chunk_shift);
>> +               net_iov_init(niov, &pp->area, NET_IOV_XSK);
>> +               if (net_mp_niov_set_dma_addr(niov, dma)) {
>> +                       xsk_pp_free(pp);
>> +                       return ERR_PTR(-ERANGE);
>> +               }
>> +       }
>> +
>> +       return pp;
>> +}
>> +
>> +static int xsk_pp_init(struct page_pool *pool)
>> +{
>> +       struct xsk_pp *pp = pool->mp_priv;
>> +
>> +       if (pool->p.order || pool->p.dma_dir != DMA_BIDIRECTIONAL ||
>> +           pool->p.dev != pp->pool->dev)
>> +               return -EINVAL;
>> +       if (pool->p.max_len > pp->pool->chunk_size ||
>> +           pool->p.offset > pp->pool->chunk_size - pool->p.max_len)
>> +               return -EINVAL;
>> +
>> +       refcount_inc(&pp->refs);
>> +       spin_lock_bh(&pp->lock);
>> +       pp->nr_pools++;
>
> What is the difference between this counter and the refcount?

refs is the lifetime of struct xsk_pp. The buffer pool holds one
reference, and each page pool that takes the provider holds one. A page
pool can outlive the socket, as page_pool_destroy() defers while buffers
are in flight. The struct is freed with the last reference.

nr_pools only tells whether a page pool still exists, when a failed bind
(patch 5) hands the leftover chunks to copy mode. But the page pools of
a failed bind never ran, and are gone by then, so a WARN on refs != 1
does the same job... I'll drop nr_pools. Thanks!

>> +       spin_unlock_bh(&pp->lock);
>> +       return 0;
>> +}
>> +
>> +static void xsk_pp_destroy(struct page_pool *pool)
>> +{
>> +       struct xsk_pp *pp = pool->mp_priv;
>> +
>> +       spin_lock_bh(&pp->lock);
>> +       pp->nr_pools--;
>> +       spin_unlock_bh(&pp->lock);
>> +       xsk_pp_put(pp);
>> +}
>> +
>> +static bool xsk_pp_claim(struct xsk_pp *pp, u32 idx, enum xsk_pp_chunk from)
>> +{
>> +       lockdep_assert_held(&pp->lock);
>> +
>> +       if (pp->state[idx] != from)
>> +               return false;
>> +
>> +       pp->state[idx] = XSK_PP_POOL;
>> +       return true;
>> +}
>> +
>> +static u32 xsk_pp_alloc_reuse(struct xsk_pp *pp, netmem_ref *netmems, u32 max)
>> +{
>> +       u32 idx, n = 0;
>> +
>> +       while (n < max && pp->reuse_cnt) {
>> +               idx = pp->reuse[--pp->reuse_cnt];
>> +               if (WARN_ON_ONCE(!xsk_pp_claim(pp, idx, XSK_PP_REUSE)))
>> +                       continue;
>> +               netmems[n++] = net_iov_to_netmem(&pp->area.niovs[idx]);
>> +       }
>> +
>> +       return n;
>> +}
>> +
>> +static u32 xsk_pp_alloc_fill(struct xsk_pp *pp, netmem_ref *netmems, u32 max)
>> +{
>> +       struct xsk_buff_pool *pool = pp->pool;
>> +       struct xsk_queue *fq = pp->fq;
>> +       u32 i, entries, cons, idx, n = 0;
>> +       u64 addr;
>> +
>> +       if (!fq || !max)
>> +               return 0;
>> +
>> +       entries = xskq_cons_nb_entries(fq, max);
>> +       if (!entries) {
>> +               fq->queue_empty_descs++;
>> +               return 0;
>> +       }
>> +
>> +       cons = fq->cached_cons;
>> +       for (i = 0; i < entries; i++) {
>> +               __xskq_cons_read_addr_unchecked(fq, cons++, &addr);
>> +               addr &= pool->chunk_mask;
>> +               idx = addr >> pp->area.niov_shift;
>> +               if (unlikely(addr >= pool->addrs_cnt ||
>> +                            !xsk_pp_claim(pp, idx, XSK_PP_USER))) {
>> +                       fq->invalid_descs++;
>> +                       continue;
>> +               }
>> +               netmems[n++] = net_iov_to_netmem(&pp->area.niovs[idx]);
>> +       }
>> +
>> +       xskq_cons_release_n(fq, entries);
>> +       __xskq_cons_release(fq);
>> +       return n;
>> +}
>> +
>> +static netmem_ref xsk_pp_alloc_netmems(struct page_pool *pool, gfp_t gfp)
>> +{
>> +       netmem_ref *netmems = pool->alloc.cache;
>> +       struct xsk_pp *pp = pool->mp_priv;
>> +       struct net_iov *niov;
>> +       dma_addr_t dma;
>> +       u32 i, n;
>> +
>> +       if (WARN_ON_ONCE(pool->alloc.count))
>
> What has the driver writer or system done if this ever happens?

A correct driver can't get there; the page pool calls ->alloc_netmems()
only with an empty cache. It fires on a page_pool core bug, or when a
driver recycles with allow_direct outside the pool's NAPI, and races the
cache. The provider fills the cache from index 0, so failing beats
overwriting live entries. devmem and zcrx have the same check; I'll add
zcrx's comment.


>> +               return 0;
>> +
>> +       spin_lock_bh(&pp->lock);
>
> For my understanding, what entities can call this function at the same
> time? With the xsk_buff_pool we were protected by NAPI.

Yeah... this is a bit gnarley. 

The provider outlives a page pool: a queue restart starts the new queue
before it frees the old pool, so the old pool releases chunks while the
new NAPI allocates. Teardown can be deferred to a work item, and the
initial fill, bind and unbind run in process context. A driver that
fills its new rings in ndo_queue_mem_alloc() even has two pools on one
FILL ring.

However, in steady state only in NAPI, once per refill, so the lock is
uncontended. I haven't been able to see it in perf...

I'll list that in a comment.


Björn