lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZwavJuVI-6d9ZSuh@mini-arch>
Date: Wed, 9 Oct 2024 09:28:22 -0700
From: Stanislav Fomichev <stfomichev@...il.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>,
	Mina Almasry <almasrymina@...gle.com>
Subject: Re: [PATCH v1 03/15] net: generalise net_iov chunk owners

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@...il.com>
> > > 
> > > 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@...il.com>
> > > Signed-off-by: David Wei <dw@...idwei.uk>
> > > ---
> > >   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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ