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 Thu, Nov 12, 2026 at 03:48:31PM +0100, Björn Töpel wrote:
>> Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:

>> >> +/* 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) ?

Not dumb, and that's what the attach check does: a generic program
fails to attach while a provider socket is bound, and a bind with one
attached falls back to copy mode. With those two, the drop in 07 can't
trigger, so it goes, as I told Stan. I agree generic doesn't
deserve more than that.

And I'd rather not tie the bringup to a native program, as driver ZC
does. That adds state: attach and detach would install and remove the
provider, restart the queues and handle that failing half way, and
whether a bound socket gets zero copy would depend on the program. Here
a socket depends only on its allocator, set up at bind, and the program
just picks the packets that go to it; the two are orthogonal.

That also leaves room for a mode without an XDP program, steering flows
to the queue like devmem and io_uring zcrx do ;-)

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

TBH I fail to see how moving 1 to a separate set would make it easier to
review.

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

I'm not saying we're orphaning anything. I am trying to reuse as much as
possible, but some parts does not fit. Like using xsk_buff_head -- the
page_pool return netmems. Type type is PAGE_POOL, and driver typically
use a stack allocated xdp_buff.

The end goal is enable the XSK core, so that a netdev driver does not
need to build a secondary shadow driver for XSK. Instead, driver
supports the page pool + queue restart APIs, and you can then use XSK,
devmem, and iouring zc with less code. Initially Rx, but we need a plan
for Tx as well at some point.

It would be awesome if over time we move from the existing "shadow
driver" world, to page pool and the provider, and MEM_TYPE_XSK_BUFF_POOL
goes once the last one has. Until then driver ZC keeps working as it
does today, and v2 will have numbers for it. FWIW, I'll keep converting
drivers too, not just land the core.

The fact that Linux does not have a driver ABI, which IMO a strength, is
just for work like this. We can clean up drivers, as the world change,
without keeping old stuff around forever.


Björn