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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Tue, Nov 10, 2026 at 11:20:26AM +0100, Björn Töpel wrote:
> A page_pool driver should not need its own AF_XDP setup. When it sets
> netdev_queue_mgmt_ops.xsk_page_pool, a zero-copy bind installs the
> UMEM provider through the queue API, as devmem and io_uring do. The
> driver only judges the chunk size, headroom and multi-buffer mode
> against its MTU and XDP program, in XDP_VALIDATE_XSK_POOL; MTU and
> program changes of a bound device get the same check. The UMEM
> headroom belongs to the socket, so the queue gets it as rx_headroom,
> next to rx_page_size.
> 
> After a queue restart, the core checks that the new page pools took
> the provider. A readable provider must back the header pool, where an
> unsplit packet lands, so pools declare the buffers they supply in
> rx_roles. The old queue's pools carry the same provider during the
> check, so a per-queue generation, mp_gen, tells the new ones apart.
> 
> Generic XDP runs outside the queue NAPI, so it cannot feed the socket.
> 
> The provider needs no xdp_buff_xsk per chunk. The buffer pool
> allocates them only for copy mode and driver zero copy, which saves
> 128 bytes per chunk.
> 
> Signed-off-by: Björn Töpel <bjorn@kernel.org>
> ---
>  include/linux/netdevice.h     |   8 ++
>  include/net/netdev_queues.h   |  12 ++
>  include/net/netdev_rx_queue.h |   2 +
>  include/net/page_pool/types.h |  12 ++
>  include/net/xdp_sock.h        |   4 +
>  include/net/xdp_sock_drv.h    |  45 +++++++
>  include/net/xsk_buff_pool.h   |   3 +
>  net/core/dev.c                |  16 +++
>  net/core/netdev_config.c      |   2 +
>  net/core/netdev_rx_queue.c    |   9 +-
>  net/core/page_pool.c          |   6 +-
>  net/core/page_pool_user.c     |  25 ++--
>  net/xdp/xsk.h                 |   7 +-
>  net/xdp/xsk_buff_pool.c       |  69 ++++++++---
>  net/xdp/xsk_pp.c              | 221 ++++++++++++++++++++++++++++++++++
>  15 files changed, 410 insertions(+), 31 deletions(-)
> 

(...)

> +
> +/**
> + * xsk_validate_dev - check AF_XDP page pool queues against a new setting
> + * @dev: device of the queues
> + * @mtu: MTU to check
> + * @prog: XDP program to check, or NULL for the current one
> + * @mode: XDP mode of @prog
> + * @extack: netlink extended ack
> + *
> + * Context: holds the instance lock of @dev, or RTNL for a device without it.
> + * Return: 0 if the driver accepts all queues of @dev that have an AF_XDP
> + * memory provider, or its error.
> + */
> +int xsk_validate_dev(struct net_device *dev, unsigned int mtu,
> +		     struct bpf_prog *prog, enum bpf_xdp_mode mode,
> +		     struct netlink_ext_ack *extack)
> +{
> +	struct netdev_rx_queue *rxq;
> +	struct xsk_pp *pp;
> +	unsigned int i;
> +	int err;
> +
> +	netdev_assert_locked_ops_compat(dev);
> +
> +	for (i = 0; i < dev->real_num_rx_queues; i++) {
> +		rxq = __netif_get_rx_queue(dev, i);
> +		if (rxq->mp_params.mp_ops != &xsk_pp_ops)
> +			continue;
> +
> +		/* Generic XDP runs outside the queue NAPI */
> +		if (mode == XDP_MODE_SKB) {
> +			NL_SET_ERR_MSG(extack,
> +				       "generic XDP cannot feed an AF_XDP page pool socket");
> +			return -EBUSY;
> +		}
> +
> +		pp = rxq->mp_params.mp_priv;
> +		err = xsk_pp_validate(dev, pp->pool, mtu, prog, extack);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xsk_validate_dev);
> +
> +/**
> + * xsk_rxq_pool - get the AF_XDP pool that provides memory to an RX queue
> + * @dev: device of the queue
> + * @queue_id: RX queue
> + *
> + * The pool stays valid while the queue that the caller allocates runs.
> + *
> + * Context: holds the instance lock of @dev.
> + * Return: the buffer pool, or NULL.
> + */
> +struct xsk_buff_pool *xsk_rxq_pool(struct net_device *dev, u16 queue_id)
> +{
> +	struct netdev_rx_queue *rxq;
> +	struct xsk_pp *pp;
> +
> +	netdev_assert_locked(dev);
> +
> +	if (queue_id >= dev->real_num_rx_queues)
> +		return NULL;
> +
> +	rxq = __netif_get_rx_queue(dev, queue_id);
> +	if (rxq->mp_params.mp_ops != &xsk_pp_ops)
> +		return NULL;
> +
> +	pp = rxq->mp_params.mp_priv;
> +	return pp->pool;
> +}
> +EXPORT_SYMBOL_GPL(xsk_rxq_pool);
> +
> +bool xsk_pp_supported(struct net_device *dev)
> +{
> +	const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
> +
> +	return qops && qops->xsk_page_pool;
> +}
> +
> +/* Called with RTNL and the instance lock of @dev. On error, @pool->pp stays
> + * set if an old page pool still owns chunks; copy mode must not take the pool.
> + */
> +int xsk_pp_bind(struct xsk_buff_pool *pool, struct net_device *dev,
> +		u16 queue_id)
> +{
> +	struct pp_memory_provider_params mp = { .mp_ops = &xsk_pp_ops };
> +	struct device *dma_dev;
> +	struct xsk_pp *pp;
> +	int err;
> +
> +	if (queue_id >= dev->real_num_rx_queues || pool->unaligned)
> +		return -EOPNOTSUPP;
> +	if ((dev->xdp_features & NETDEV_XDP_ACT_XSK) != NETDEV_XDP_ACT_XSK ||
> +	    (xsk_pool_uses_sg(pool) && dev->xdp_zc_max_segs == 1))
> +		return -EOPNOTSUPP;

that is checked elsewhere, in xp_assign_dev() I assume.

Why there are skb mode checks?

You sort of introduce totally separate control path and rx data path. It
is hard for me to buy onto the management burden of all this new code :(

I believe we should break this up onto smaller chunks and don't try to
solve everything within a single set...or wil that not be possible?

> +	if (dev_xdp_prog_id(dev, XDP_MODE_SKB))
> +		return -EOPNOTSUPP;
> +	if (!netif_all_mps_have_cap(dev, MP_CAP_READABLE))
> +		return -EBUSY;
> +
> +	err = xsk_pp_validate(dev, pool, dev->mtu, NULL, NULL);
> +	if (err)
> +		return err;
> +
> +	dma_dev = netdev_queue_get_dma_dev(dev, queue_id, NETDEV_QUEUE_TYPE_RX);
> +	if (!dma_dev)
> +		return -EOPNOTSUPP;
> +
> +	err = xp_dma_map(pool, dma_dev, XSK_PP_DMA_ATTR, pool->umem->pgs,
> +			 pool->umem->npgs);
> +	if (err)
> +		return err;

Couldn't we have this part common across drivers wanting to do ZC?

> +
> +	pp = xsk_pp_create(pool);
> +	if (IS_ERR(pp)) {
> +		xp_dma_unmap(pool, XSK_PP_DMA_ATTR);
> +		return PTR_ERR(pp);
> +	}
> +
> +	mp.mp_priv = pp;
> +	mp.rx_page_size = pool->chunk_size;
> +	mp.rx_headroom = xsk_pool_get_headroom(pool);
> +	err = netif_mp_open_rxq(dev, queue_id, &mp, NULL);
> +	if (!err) {
> +		pp->rxq = __netif_get_rx_queue(dev, queue_id);
> +		pool->pp = pp;
> +		pool->xdp_zc_max_segs = dev->xdp_zc_max_segs;
> +		return 0;
> +	}
> +
> +	xp_dma_unmap(pool, XSK_PP_DMA_ATTR);
> +	/* Copy mode takes the chunks that the provider holds */
> +	if (!xp_alloc_heads(pool) && xsk_pp_abandon(pp))
> +		xsk_pp_put(pp);
> +	else
> +		pool->pp = pp;
> +
> +	return err;
> +}
> +
> +/* Called with RTNL and the instance lock of the device */
> +void xsk_pp_unbind(struct xsk_buff_pool *pool)
> +{
> +	struct xsk_pp *pp = pool->pp;
> +	struct pp_memory_provider_params mp = {
> +		.mp_ops = &xsk_pp_ops,
> +		.mp_priv = pp,
> +	};
> +
> +	/* Unregister removed the provider already */
> +	if (!pp->rxq)
> +		return;
> +
> +	netif_mp_close_rxq(pool->netdev, pool->queue_id, &mp);
> +	xsk_pp_detach(pp);
> +}
> -- 
> 2.56.0
>