Message-ID: <CAHS8izO6aBdHkN5QF8Z57qGwop3+XObd5T6P8VnMdyT=FUDO1A@mail.gmail.com>
Date: Fri, 1 Nov 2024 12:24:31 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: David Wei <dw@...idwei.uk>, io-uring@...r.kernel.org, netdev@...r.kernel.org, 
	Jens Axboe <axboe@...nel.dk>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH v1 06/15] net: page_pool: add ->scrub mem provider callback

On Fri, Nov 1, 2024 at 11:34 AM Pavel Begunkov <asml.silence@...il.com> wrote:
>
> On 11/1/24 17:18, Mina Almasry wrote:
> > On Wed, Oct 16, 2024 at 10:42 AM Pavel Begunkov <asml.silence@...il.com> wrote:
> ...
> >>> The critical point is as I said above, if you free the memory only
> >>> when the pp is destroyed, then the memory lives from 1 io_uring ZC
> >>> instance to the next. The next instance will see a reduced address
> >>> space because the previously destroyed io_uring ZC connection did not
> >>> free the memory. You could have users in production opening thousands
> >>> of io_uring ZC connections between rxq resets, and not cleaning up
> >>> those connections. In that case I think eventually they'll run out of
> >>> memory as the memory leaks until it's cleaned up with a pp destroy
> >>> (driver reset?).
> >>
> >> Not sure what giving memory from one io_uring zc instance to
> >> another means. And it's perfectly valid to receive a buffer, close
> >> the socket and only after use the data, it logically belongs to
> >> the user, not the socket. It's only bound to io_uring zcrx/queue
> >> object for clean up purposes if io_uring goes down, it's different
> >> from devmem TCP.
> >>
> >
> > (responding here because I'm looking at the latest iteration after
> > vacation, but the discussion is here)
> >
> > Huh, interesting. For devmem TCP we bind a region of memory to the
> > queue once, and after that we can create N connections all reusing the
> > same memory region. Is that not the case for io_uring? There are no
>
> Hmm, I think we already discussed the same question before. Yes, it
> does indeed support arbitrary number of connections. For what I was
> saying above, the devmem TCP analogy would be attaching buffers to the
> netlink socket instead of a tcp socket (that new xarray you added) when
> you give it to user space. Then, you can close the connection after a
> receive and the buffer you've got would still be alive.
>

Ah, I see. You're making a tradeoff here. You leave the buffers alive
after each connection so the userspace can still use them if it wishes
but they are of course unavailable for other connections.

But in our case (and I'm guessing yours) the process that will set up
the io_uring memory provider/RSS/flow steering will be a different
process from the one that sends/receive data, no? Because the former
requires CAP_NET_ADMIN privileges while the latter will not. If they
are 2 different processes, what happens when the latter process doing
the send/receive crashes? Does the memory stay unavailable until the
CAP_NET_ADMIN process exits? Wouldn't it be better to tie the lifetime
of the buffers of the connection? Sure, the buffers will become
unavailable after the connection is closed, but at least you don't
'leak' memory on send/receive process crashes.

Unless of course you're saying that only CAP_NET_ADMIN processes will
run io_rcrx connections. Then they can do their own mp setup/RSS/flow
steering and there is no concern when the process crashes because
everything will be cleaned up. But that's a big limitation to put on
the usage of the feature no?

-- 
Thanks,
Mina

Powered by blists - more mailing lists