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

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

On Thu, Nov 12, 2026 at 03:48:31PM +0100, Björn Töpel wrote:
> 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.

Might be a dumb question but couldn't you forbid generic xdp attach if you
detect zc socket active on netdev? Or can't you have some deferring
mechanism like other drivers do? Or is it now not possible as decision at
that other drivers is done early at napi and here you don't really have a
way to distinguish it?

Another thought here - if user binds ZC socket and follows with generic
XDP prog, then i'd say something is wrong.

You generally *need* native xdp prog for resource bringup. I don't see why
we have to worry that much about generic hook, which probably lacks real
world use cases (sorry if I offended someone actually using it) ?

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

1 as prep would be good, but I believe we were saying new ZC drivers are
supposed to include mbuf support from day 0. Otherwise it might be hard to
enforce this later.

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

I meant existing drivers do this on their side, idea was to make it common
before your set so that you could reuse existing code.

I am wondering what is the end goal here. If we would have enough of work
force (or tokens) and time and willpower, I assume we would have every
current xsk ZC driver converted to page_pool and finally orphan the
MEM_TYPE_XSK_BUFF_POOL.

That might not happen, so I feel like I sound like a sentinel, but current
code can't just be abandonded. Hence all these asks for code reuse, etc.

> 
> 
> Björn
>