Message-ID: <8c90f485-192f-4f7a-ac94-8171d78f3c4a@gmail.com>
Date: Mon, 28 Jul 2025 21:57:49 +0100
From: Pavel Begunkov <asml.silence@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
 io-uring@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
 Willem de Bruijn <willemb@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 andrew+netdev@...n.ch, horms@...nel.org, davem@...emloft.net,
 sdf@...ichev.me, dw@...idwei.uk, michael.chan@...adcom.com,
 dtatulea@...dia.com, ap420073@...il.com
Subject: Re: [RFC v1 00/22] Large rx buffer support for zcrx

On 7/28/25 21:23, Mina Almasry wrote:
...>>>
>>> - I'm a bit confused that you're not making changes to the core net
>>> stack to support non-PAGE_SIZE netmems. From a quick glance, it seems
>>> that there are potentially a ton of places in the net stack that
>>> assume PAGE_SIZE:
>>
>> The stack already supports large frags and it's not new. Page pools
>> has higher order allocations, see __page_pool_alloc_page_order. The
>> tx path can allocate large pages / coalesce user pages.
> 
> Right, large order allocations are not new, but I'm not sure they
> actually work reliably. AFAICT most drivers set pp_params.order = 0;
> I'm not sure how well tested multi-order pages are.
> 
> It may be reasonable to assume multi order pages just work and see
> what blows up, though.
> 
>> Any specific
>> place that concerns you? There are many places legitimately using
>> PAGE_SIZE: kmap'ing folios, shifting it by order to get the size,
>> linear allocations, etc.
>>
> 
>  From a 5-min look:
> 
> - skb_splice_from_iter, this line: size_t part = min_t(size_t,
> PAGE_SIZE - off, len);

It does it for pages that it got from
iov_iter_extract_pages() a few lines above, those are PAGE_SIZE'd.

> - skb_pp_cow_data, this line: max_head_size =
> SKB_WITH_OVERHEAD(PAGE_SIZE - headroom);

This one should be about the linear part, not frags

> - skb_seq_read, this line: pg_sz = min_t(unsigned int, pg_sz -
> st->frag_off, PAGE_SIZE - pg_off

That's kmap handling, it can iterate a frag multiple times
in PAGE_SIZE chunks for high mem archs.

> - zerocopy_fill_skb_from_iter, this line: int size = min_t(int,
> copied, PAGE_SIZE - start);

Pages from iov_iter_get_pages2(), same as with
skb_splice_from_iter()

> I think the `PAGE_SIZE -` logic in general assumes the memory is
> PAGE_SIZEd. Although in these cases it seems page specifics, i.e.
> net_iovs wouldn't be exposed to these particular call sites.
> 
> I spent a few weeks acking the net stack for all page-access to prune
> all of them to add unreadable netmem... are you somewhat confident
> there are no PAGE_SIZE assumptions in the net stack that affect
> net_iovs that require a deep look? Or is the approach here to merge

The difference is that this one is already supported and the
stack is large page aware, while unreadable frags was a new
concept.

> this and see what/if breaks?

No reason for it not to work. Even if breaks somewhere on that,
it should be a pre-existent problem, which needs to be fixed
either way.

>>> cd net
>>> ackc "PAGE_SIZE|PAGE_SHIFT" | wc -l
>>> 468
>>>
>>> Are we sure none of these places assuming PAGE_SIZE or PAGE_SHIFT are
>>> concerning?
>>>
>>> - You're not adding a field in the net_iov that tells us how big the
>>> net_iov is. It seems to me you're configuring the driver to set the rx
>>> buffer size, then assuming all the pp allocations are of that size,
>>> then assuming in the rxzc code that all the net_iov are of that size.
>>> I think a few problems may happen?
>>>
>>> (a) what happens if the rx buffer size is re-configured? Does the
>>> io_uring rxrc instance get recreated as well?
>>
>> Any reason you even want it to work? You can't and frankly
>> shouldn't be allowed to, at least in case of io_uring. Unless it's
>> rejected somewhere earlier, in this case it'll fail on the order
>> check while trying to create a page pool with a zcrx provider.
>>
> 
> I think it's reasonable to disallow rx-buffer-size reconfiguration
> when the queue is memory-config bound. I can check to see what this
> code is doing.

Right, it doesn't make sense to reconfigure zcrx, and we can
only fail the operation one way or another.

>>> (b) what happens with skb coalescing? skb coalescing is already a bit
>>> of a mess. We don't allow coalescing unreadable and readable skbs, but
>>> we do allow coalescing devmem and iozcrx skbs which could lead to some
>>> bugs I'm guessing already. AFAICT as of this patch series we may allow
>>> coalescing of skbs with netmems inside of them of different sizes, but
>>> AFAICT so far, the iozcrx assume the size is constant across all the
>>> netmems it gets, which I'm not sure is always true?
>>
>> It rejects niovs from other providers incl. from any other io_uring
>> instances, so it only assume a uniform size for its own niovs.
> 
> Thanks. What is 'it' and where is the code that does the rejection?

zcrx does, you're familiar with this chunk:

io_uring/zcrx.c:

io_zcrx_recv_frag() {
	if (niov->pp->mp_ops != &io_uring_pp_zc_ops ||
	    io_pp_to_ifq(niov->pp) != ifq)
		return -EFAULT;
}

>> The
>> backing memory is verified that it can be chunked.
>>    > For all these reasons I had assumed that we'd need space in the
>>> net_iov that tells us its size: net_iov->size.
>>
>> Nope, not in this case.
>>
>>> And then netmem_size(netmem) would replace all the PAGE_SIZE
>>> assumptions in the net stack, and then we'd disallow coalescing of
>>> skbs with different-sized netmems (else we need to handle them
>>> correctly per the netmem_size).
>> I'm not even sure what's the concern. What's the difference b/w
>> tcp_recvmsg_dmabuf() getting one skb with differently sized frags
>> or same frags in separate skbs? You still need to handle it
>> somehow, even if by failing.
>>
> 
> Right, I just wanted to understand what the design is. I guess the
> design is allowing the netmems in the same skb to have different max
> frag lens, yes?

Yeah, and it's already allowed for higher order pages.

> I am guessing that it works, even in tcp_recvmsg_dmabuf. I guess the

And you won't see it unless adds support for that, that's why
I added this:

if (!net_is_devmem_iov(niov)) {
	err = -ENODEV;
	goto out;
}

> frag len is actually in frag->len, so already it may vary from frag to
> frag. Even if coalescing happens, some frags would have a frag->len =
> PAGE_SIZE and some > PAGE_SIZE. Seems fine to me off the bat.
> 
>> Also, we should never coalesce different niovs together regardless
>> of sizes. And for coalescing two chunks of the same niov, it should
>> work just fine even without knowing the length.
>>
> 
> Yeah, we should probably not coalesce 2 netmems together, although I
> vaguely remember reading code in a net stack hepler that does that
> somewhere already. Whatever.

Let know if that turns out to be true, because it should already
be broken. You shouldn't coalesce pages from different folios,
and to check that you need to get the head page / etc., which
niovs obviously don't have.

-- 
Pavel Begunkov

Powered by blists - more mailing lists