On 10/11/24 19:44, David Wei wrote:
On 2024-10-09 09:28, Stanislav Fomichev wrote:
On 10/08, Pavel Begunkov wrote:
On 10/8/24 16:46, Stanislav Fomichev wrote:
On 10/07, David Wei wrote:
From: Pavel Begunkov <asml.silence@xxxxxxxxx>
Currently net_iov stores a pointer to struct dmabuf_genpool_chunk_owner,
which serves as a useful abstraction to share data and provide a
context. However, it's too devmem specific, and we want to reuse it for
other memory providers, and for that we need to decouple net_iov from
devmem. Make net_iov to point to a new base structure called
net_iov_area, which dmabuf_genpool_chunk_owner extends.
Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
Signed-off-by: David Wei <dw@xxxxxxxxxxx>
---
include/net/netmem.h | 21 ++++++++++++++++++++-
net/core/devmem.c | 25 +++++++++++++------------
net/core/devmem.h | 25 +++++++++----------------
3 files changed, 42 insertions(+), 29 deletions(-)
diff --git a/include/net/netmem.h b/include/net/netmem.h
index 8a6e20be4b9d..3795ded30d2c 100644
--- a/include/net/netmem.h
+++ b/include/net/netmem.h
@@ -24,11 +24,20 @@ struct net_iov {
unsigned long __unused_padding;
unsigned long pp_magic;
struct page_pool *pp;
- struct dmabuf_genpool_chunk_owner *owner;
+ struct net_iov_area *owner;
Any reason not to use dmabuf_genpool_chunk_owner as is (or rename it
to net_iov_area to generalize) with the fields that you don't need
set to 0/NULL? container_of makes everything harder to follow :-(
It can be that, but then io_uring would have a (null) pointer to
struct net_devmem_dmabuf_binding it knows nothing about and other
fields devmem might add in the future. Also, it reduces the
temptation for the common code to make assumptions about the origin
of the area / pp memory provider. IOW, I think it's cleaner
when separated like in this patch.
Ack, let's see whether other people find any issues with this approach.
For me, it makes the devmem parts harder to read, so my preference
is on dropping this patch and keeping owner=null on your side.
I don't mind at this point which approach to take right now. I would
prefer keeping dmabuf_genpool_chunk_owner today even if it results in a
nullptr in io_uring's case. Once there are more memory providers in the
future, I think it'll be clearer what sort of abstraction we might need
here.
That's the thing about abstractions, if we say that devmem is the
only first class citizen for net_iov and everything else by definition
is 2nd class that should strictly follow devmem TCP patterns, and/or
that struct dmabuf_genpool_chunk_owner is an integral part of net_iov
and should be reused by everyone, then preserving the current state
of the chunk owner is likely the right long term approach. If not, and
net_iov is actually a generic piece of infrastructure, then IMHO there
is no place for devmem sticking out of every bit single bit of it, with
structures that are devmem specific and can even be not defined without
devmem TCP enabled (fwiw, which is not an actual problem for
compilation, juts oddness).
This patch is one way to do it. The other way assumed is to
convert that binding pointer field to a type-less / void *
context / private pointer, but that seems worse. The difference
starts and the chunk owners, i.e. io_uring's area has to extend
the structure, and we'd still need to cast both that private filed
and the chunk owner / area (with container_of), + a couple more
reasons on top.
--
Pavel Begunkov
