On 11/26/24 15:26, Pavel Begunkov wrote:
On 11/15/24 23:14, Jakub Kicinski wrote:
On Thu, 14 Nov 2024 12:56:14 -0800 Mina Almasry wrote:
But I've been bringing this up a lot in the past (and offering
alternatives that don't introduce this overloading) and I think this
conversation has run its course. I'm unsure about this approach and
this could use another pair of eyes. Jakub, sorry to bother you but
you probably are the one that reviewed the whole net_iov stuff most
closely. Any chance you would take a look and provide direction here?
Maybe my concern is overblown...
Sorry I haven't read this code very closely (still!?) really hoping
for next version to come during the merge window when time is more
abundant :S
From scanning the quoted context I gather you're asking about using
the elevated ->pp_ref_count for user-owned pages? If yes - I also
find that part.. borderline incorrect. The page pool stats will show
these pages as allocated which normally means held by the driver or
the stack. Pages given to the user should effectively leave the pool.
It can't just drop all net_iov refs here, otherwise the buffer might
be returned back to page pool and reused while the user still reading
the data. We can't even be smart in the release callback as it might
never get there and be reallocated purely via alloc.cache. And either
way, tunneling all buffers into ->release_netmem would be horrible
for performance, and it'd probably even need a check in
page_pool_recycle_in_cache().
Fixing it for devmem TCP (which also holds a net_iov ref while it's
given to user, so we're not unique here) sounds even harder as
they're stashed in a socket xarray page pool knows nothing about,
so it might need some extra counting on top?
This set has a problem with page_pool_alloc_frag*() helpers, so
we'd either need to explicitly chip away some bits from ->pp_ref_count
or move user counting out of net_iov and double the cost of one
of the main sources of overhead, and then being very inventive
optimising it in the future, but that won't solve the "should
leave the pool" problem.
If it's just stats printing, it should be quite easy to fix
for the current set, ala some kind of "mask out bits responsible
for user refs". And I don't immediately have an idea of how to
address it for devmem TCP.
Also note, that if sth happens with io_uring or such, those
"user" refs are going to be dropped by the kernel off a page
pool callback, so it's not about leaking buffers.
So I'm guessing this is some optimization.
Same for patch 8.
This one will need some more explanation, otherwise it'd be a guess
game. What is incorrect? The overall approach or some implementation
aspect? It's also worth to note that it's a private queue, stopping
the napi attached to it shouldn't interfere with other queues and
users in the system, that's it assuming steering configured right.
But let me not get sucked into this before we wrap up the net-next PR..
Jakub, there is hardly anything I can do with the series if you veto it
but remain silent on any details. It appears you've had some private
conversations with David I wasn't privy to. And it's hard to tell but it
sounds that your ask number (1) is that it should be able to serve
multiple page pools in parallel. And (2), you don't like something in
regards to refcounting.
(2) As I explained in the previous message, I can separate refcounting
to get rid of the io_uring ref bias, but it's not going to help with
"elevated ->pp_ref_count for user-owned pages" IIUC what you meant
here. Moreover, devmem is suffering from the same thing, and if you
feel like it's really a big deal, we should revert devmem TCP, which
would undesirable, or somehow solve it otherwise.
Another problem is performance, it doubles refcounting, which is just
not acceptable, so I'd need to do something about it. For example,
the syscall part of the path, ala recv(2), would do something like:
for (i = 0; i < nr_frags; i++) {
niov = skb->frags[i].netmem;
// for each user ref we also take a kernel ref so that
// the net_iov is not reallocated while the user space reads data.
atomic_inc(io_uring->user_refs[idx(niov)]);
atomic_inc(net_iov->pp_ref_count);
post_completion();
}
One way out would be to steal frags from an skb together with a
reference it holds.
bool steal_frag_refs = !skb->cloned;
for (i = 0; i < nr_frags; i++) {
niov = skb->frags[i].netmem;
atomic_inc(io_uring->user_refs[idx(niov)]);
if (!steal_frag_refs)
atomic_inc(net_iov->pp_ref_count);
post_completion();
}
// we reused the frag refs, so took it away from core net
if (steal_frag_refs)
skb->nr_frags = 0;
Does that sound reasonable to you? Or do you have any other suggestions?
(1) is also doable, but that can't be done without performance penalties,
or specifically it'll tie hands from many future optimisations for more
or less same reasons why page pools are not shared and can't be pulled
from by mulitple CPUs in parallel. For example, if you think that
page_pool_napi_local() is a good optimisation, then it'd also make
sense to optimise user refcounts in a similar fashion.
Perhaps, it can have a fast path that assumes a single page pool, assume
that the state when there are multiple pp's is transient, and do some
additional sync at queue restart and/or pp init/destroy callbacks when
switching between 1 -> many and many -> 1, which would likely need
to briefly stop the queue / napi in one way or another. Does that
sound reasonable?
--
Pavel Begunkov
