On 6/3/24 16:43, Mina Almasry wrote:
On Mon, Jun 3, 2024 at 7:52 AM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote:
On 6/3/24 15:17, Mina Almasry wrote:
On Fri, May 31, 2024 at 10:35 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
On Thu, May 30, 2024 at 08:16:01PM +0000, Mina Almasry wrote:
I'm unsure if the discussion has been resolved yet. Sending the series
anyway to get reviews/feedback on the (unrelated) rest of the series.
As far as I'm concerned it is not. I've not seen any convincing
argument for more than page/folio allocator including larger order /
huge page and dmabuf.
Thanks Christoph, this particular patch series adds dmabuf, so I
assume no objection there. I assume the objection is that you want the
generic, extensible hooks removed.
To be honest, I don't think the hooks are an integral part of the
design, and at this point I think we've argued for them enough. I
think we can easily achieve the same thing with just raw if statements
in a couple of places. We can always add the hooks if and only if we
actually justify many memory providers.
Any objections to me removing the hooks and directing to memory
allocations via simple if statements? Something like (very rough
draft, doesn't compile):
The question for Christoph is what exactly is the objection here? Why we
would not be using well defined ops when we know there will be more
users? Repeating what I said in the last thread, for io_uring it's used
to implement the flow of buffers from userspace to the kernel, the ABI,
which is orthogonal to the issue of what memory type it is and how it
came there. And even if you mandate unnecessary dmabuf condoms for user
memory in one form or another IMHO for no clear reason, the callbacks
(or yet another if-else) would still be needed.
Sure, Mina can drop and hard code devmem path to easy the pain for
him and delay the discussion, but then shortly after I will be
re-sending same shit.
You don't need to re-send the same ops again, right? You can add io
uring support without ops. Something like:
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 92be1aaf18ccc..2cc986455bce6 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -557,8 +557,8 @@ netmem_ref page_pool_alloc_netmem(struct page_pool
*pool, gfp_t gfp)
return netmem;
/* Slow-path: cache empty, do real allocation */
- if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
- netmem = pool->mp_ops->alloc_pages(pool, gfp);
+ if (unlikely(page_pool_is_dmabuf(pool)))
+ netmem = mp_dmabuf_devmem_alloc_pages():
+ else if (unlikely(page_pool_is_iouring(pool)))
+ netmem = mp_io_uring_alloc_pages():
else
netmem = __page_pool_alloc_pages_slow(pool, gfp);
return netmem;
So IMO, the ops themselves, which Christoph is repeatedly nacking, are
not that important.
I humbly think the energy should be spent convincing maintainers of
the use case of io uring memory, not the ops. The ops are a cosmetic
I haven't seen any arguments against from the (net) maintainers so
far. Nor I see any objection against callbacks from them (considering
that either option adds an if).
And just not to confuse folks, it's just user pages, not some
weird special io_uring memory.
change to the code, and can be added later. Christoph is nacking the
ops because it gives people too much rope [1].
Yes, it is cosmetic, just as much as removing it is a cosmetic
change. You can apply same "too much rope" argument basically
to anything.
Take io_uring, nothing would change in the process, it'd still
be sent to net and reviewed exactly same way, while being less
clean, with poorer subsystem separation, allowing custom
formats / argument list, etc. I think it's cleaner with callbacks,
Mr. Christoph has other beliefs and keeps coercing to them,
even though from time to time it backfires for the author, just
personal experience.
But if you disagree and think the ops themselves are important for a
reason I missed, I'm happy waiting until agreement is reached here.
Sorry, just voicing my 2 cents.
[1] https://lore.kernel.org/netdev/ZjjHUh1eINPg1wkn@xxxxxxxxxxxxx/
--
Pavel Begunkov
