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

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

Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:

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

Ah, yes! I'll do the common checks first in xp_assign_dev(), and branch
after.

> Why there are skb mode checks?

(Note to self: I need to make some details/designs more obvious in the
cover!)

Because the provider is turned on by the queue API at bind, not by an
XDP program. This is a big difference. Once the socket is bound, the
queue's page pool hands the device chunks from FILL, whatever program is
attached: native, generic or none. A generic redirect would copy into a
chunk taken from FILL behind the provider's back, and a provider pool
doesn't have the heads that copy path needs. So 07 drops those
redirects.

The checks make that visible (I hope): with a generic program attached,
bind falls back to copy mode, where generic XDP works, and attaching one
to a provider queue fails instead of dropping everything.

Driver ZC is different: ice and i40e use the pool only while a native
program is attached, so with a generic one the queue runs on normal
buffers, and the copy is the only FILL user.

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

Separate from driver ZC, yes, but not new: the provider binds the way
devmem and io_uring zcrx do, netif_mp_open_rxq() and a queue restart,
and the driver keeps its normal page pool RX path, with no ZC RX loop of
its own. The cost moves into the AF_XDP core, and xsk_pp.c is real code,
I know.

It can be split:

1. preparation, no new behaviour: netmem frag addresses, the page pool
   caps, fbnic's redirect and netmem patches, the TX metadata fix;
2. the provider for single-buffer packets, with fbnic, without the copy
   fallback or fbnic's shared header/payload pool;
3. multi-buffer.

Not sure it would help much TBH.

The provider, its bind and the delivery need a driver to land, so 2 is
the smallest set that does something. v2 also loses some things (but I'd
like to keep the validate ndo_bpf), e.g. nr_pools and the duplicate
checks. And I'm happy to look after xsk_pp.c. Would that work for you?

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

It is: this is core code, and no provider driver maps UMEM itself;
netdev_queue_get_dma_dev() gives the core the device, as for io_uring
zcrx. Driver ZC could map in xp_assign_dev() too, but drivers differ in
DMA device and attributes, so that's its own series.


Björn