Re: [PATCH net-next v1 05/11] xsk: Install UMEM page pool providers in the core

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

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

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

>> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
>> index 18966bbdb5b7..c620a0c32c17 100644
>> --- a/net/xdp/xsk_buff_pool.c
>> +++ b/net/xdp/xsk_buff_pool.c
>> @@ -104,10 +101,30 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
>>         pool->fq = xs->fq_tmp;
>>         pool->cq = xs->cq_tmp;
>>
>> -       for (i = 0; i < pool->free_heads_cnt; i++) {
>> +       return pool;
>> +
>> +out:
>> +       xp_destroy(pool);
>> +       return NULL;
>> +}
>> +
>> +/* Only copy mode and driver zero copy use heads */
>> +int xp_alloc_heads(struct xsk_buff_pool *pool)
>> +{
>> +       struct xdp_buff_xsk *xskb;
>> +       u32 i;
>> +
>> +       if (pool->heads)
>> +               return 0;
>> +
>> +       pool->heads = kvzalloc_objs(*pool->heads, pool->heads_cnt);
>> +       if (!pool->heads)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < pool->heads_cnt; i++) {
>
> Now I get why you are not reusing the existing xdp_buff_xsk infra, you
> are removing them here for the PP case. Please mention this in the
> cover letter of the previous patch. There might be a good argument for
> it at least in aligned mode. It is wasting memory versus performance.
> Today when memory is expensive and also for umems that are shared
> between many sockets, the current scheme in the xsk_buffer pool for
> aligned mode might not be worth it.

Will do!

> I have two questions for you. What would the performance be if you
> moved the aligned mode over to use the same free_list as unaligned
> mode and then use that in PP? Would reuse more code, but would it be
> fast enough?
>
> If that is not performant, what if you instead decreased the cost of
> the current pre-allocated xskbs in aligned mode by just creating a
> single copy per umem and using those in PP? Might be a good thing for
> regular xsk drivers too these days, but do not know if it will work
> for you.
>
> I do not know if any of these suggestions would work, but I think you
> should examine them as less code paths is generally a good thing. You
> might also improve the non-PP case too by optimizing it for your case!

Neither would help the provider. The page pool hands out net_iovs, so it
needs one per chunk anyway, and the driver brings its own xdp_buff.
net_iov, reuse and state are 69 bytes per chunk, against 128 for an
xdp_buff_xsk; free_list would add those 128 on top. Nor is reuse on the
hot path: the page pool recycles in its own cache and ring, and gives
chunks back only when it is destroyed or its ring is full. The hot path
is the FILL claim and the RX ring flush.

I don't like the reuse[] fixes size tho...

One copy per UMEM doesn't work either: an xdp_buff_xsk holds pool, rxq,
DMA addresses and cb, so it can only be shared by pools of the same
device and queue, and those already share the pool.

For driver ZC I agree, heads add up with a shared UMEM: 32 MB per pool
for 1 GB of 4K chunks. Unaligned mode allocates a head per chunk too,
though, so moving aligned mode to free_heads saves nothing by itself;
heads would have to be sized by the rings. Worth a look, with numbers,
but in a separate series.

>>                 xskb = &pool->heads[i];
>>                 xskb->pool = pool;
>> -               xskb->xdp.frame_sz = umem->chunk_size - umem->headroom;
>> +               xskb->xdp.frame_sz = pool->chunk_size - pool->headroom;
>>                 INIT_LIST_HEAD(&xskb->list_node);
>>                 if (pool->unaligned)
>>                         pool->free_heads[i] = xskb;
>> @@ -115,11 +132,7 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
>>                         xp_init_xskb_addr(xskb, pool, (u64)i * pool->chunk_size);
>>         }
>>
>> -       return pool;
>> -
>> -out:
>> -       xp_destroy(pool);
>> -       return NULL;
>> +       return 0;
>
> Agree with Stan that these changes should be in a separate commit.
> Think also about if it would make sense to put your page_pool changes
> in yet another commit.

ACK!

>>  }
>>
>>  void xp_set_rxq_info(struct xsk_buff_pool *pool, struct xdp_rxq_info *rxq)
>> @@ -212,9 +225,24 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>>          */
>>         hlist_add_head(&pool->dev_node, &netdev->xsk_pools);
>>
>> -       if (force_copy)
>> +       if (force_copy) {
>>                 /* For copy-mode, we are done. */
>> +               err = xp_alloc_heads(pool);
>> +               goto out;
>> +       }
>> +
>> +       if (xsk_pp_supported(netdev)) {
>> +               err = xsk_pp_bind(pool, netdev, queue_id);
>> +               if (err)
>> +                       goto err_unreg_pool;
>> +
>> +               pool->umem->zc = true;
>>                 return 0;
>> +       }
>> +
>> +       err = xp_alloc_heads(pool);
>> +       if (err)
>> +               goto out;
>>
>>         if ((netdev->xdp_features & NETDEV_XDP_ACT_XSK) != NETDEV_XDP_ACT_XSK) {
>>                 err = -EOPNOTSUPP;
>> @@ -267,8 +295,10 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>>  err_unreg_xsk:
>>         xp_disable_drv_zc(pool);
>>  err_unreg_pool:
>> -       if (!force_zc)
>> -               err = 0; /* fallback to copy mode */
>> +       /* An old page pool may still own chunks of a failed provider. */
>> +       if (!force_zc && !pool->pp)
>> +               err = xp_alloc_heads(pool); /* fallback to copy mode */
>> +out:
>>         if (err) {
>>                 pool->umem->flags = umem_flags;
>>                 xsk_clear_pool_at_qid(netdev, queue_id);
>> @@ -301,7 +331,10 @@ static void __xp_clear_dev(struct xsk_buff_pool *pool)
>>         struct net_device *netdev = pool->netdev;
>>
>>         netdev_assert_locked_ops_compat(netdev);
>> -       xp_disable_drv_zc(pool);
>> +       if (pool->pp)
>> +               xsk_pp_unbind(pool);
>> +       else
>> +               xp_disable_drv_zc(pool);
>>         xsk_clear_pool_at_qid(netdev, pool->queue_id);
>>         hlist_del(&pool->dev_node);
>>         pool->netdev = NULL;
>> @@ -474,7 +507,7 @@ static void xp_check_dma_contiguity(struct xsk_dma_map *dma_map)
>>
>>  static int xp_init_dma_info(struct xsk_buff_pool *pool, struct xsk_dma_map *dma_map)
>>  {
>> -       if (!pool->unaligned) {
>> +       if (!pool->unaligned && pool->heads) {
>>                 u32 i;
>>
>>                 for (i = 0; i < pool->heads_cnt; i++) {
>> diff --git a/net/xdp/xsk_pp.c b/net/xdp/xsk_pp.c
>> index fdd55265a6cd..c2214e718cdb 100644
>> --- a/net/xdp/xsk_pp.c
>> +++ b/net/xdp/xsk_pp.c
>> @@ -8,6 +8,9 @@
>>   *       +-----RX ring-------+
>>   */
>>
>> +#include <net/netdev_lock.h>
>> +#include <net/netdev_queues.h>
>> +#include <net/netdev_rx_queue.h>
>>  #include <net/page_pool/helpers.h>
>>  #include <net/page_pool/memory_provider.h>
>>  #include <net/xdp_sock_drv.h>
>> @@ -15,6 +18,8 @@
>>  #include "xsk.h"
>>  #include "xsk_queue.h"
>>
>> +#define XSK_PP_DMA_ATTR        (DMA_ATTR_WEAK_ORDERING | DMA_ATTR_SKIP_CPU_SYNC)
>> +
>>  enum xsk_pp_chunk {
>>         XSK_PP_USER,    /* owned by userspace */
>>         XSK_PP_POOL,    /* owned by a page pool */
>> @@ -30,6 +35,8 @@ struct xsk_pp {
>>         refcount_t refs;
>>         u32 nr_pools;
>>         bool dma_need_sync;
>> +       /* RX queue of the provider, NULL once it is removed */
>> +       struct netdev_rx_queue *rxq;
>>         u8 *state;
>>         u32 reuse_cnt;
>>         u32 reuse[];
>> @@ -236,11 +243,225 @@ static int xsk_pp_nl_fill(void *mp_priv, struct sk_buff *rsp,
>>         return 0;
>>  }
>>
>> +/* The queue no longer uses the provider, and its DMA stopped */
>> +static void xsk_pp_detach(struct xsk_pp *pp)
>> +{
>> +       pp->rxq = NULL;
>> +       spin_lock_bh(&pp->lock);
>
> Just curious, but why this protection of the fq here?

After a normal unbind the queue is already stopped, so nothing reads
FILL anyway. It matters when the restart fails and the old queue keeps
running; I have a net fix that keeps the provider on such a queue until
a retry succeeds... but maybe it's better to introduce the lock then.
Let me noodle on it for the next rev.


Björn