[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAHS8izNPSHR7B24Y3RZiBeZHkPyzKAKdZbQgXwqwgs01HzxDTw@mail.gmail.com>
Date: Fri, 30 May 2025 10:50:22 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Byungchul Park <byungchul@...com>
Cc: willy@...radead.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, kernel_team@...ynix.com, kuba@...nel.org,
ilias.apalodimas@...aro.org, harry.yoo@...cle.com, hawk@...nel.org,
akpm@...ux-foundation.org, davem@...emloft.net, john.fastabend@...il.com,
andrew+netdev@...n.ch, asml.silence@...il.com, toke@...hat.com,
tariqt@...dia.com, edumazet@...gle.com, pabeni@...hat.com, saeedm@...dia.com,
leon@...nel.org, ast@...nel.org, daniel@...earbox.net, david@...hat.com,
lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com, vbabka@...e.cz,
rppt@...nel.org, surenb@...gle.com, mhocko@...e.com, horms@...nel.org,
linux-rdma@...r.kernel.org, bpf@...r.kernel.org, vishal.moola@...il.com
Subject: Re: [RFC v3 01/18] netmem: introduce struct netmem_desc mirroring
struct page
On Thu, May 29, 2025 at 6:10 PM Byungchul Park <byungchul@...com> wrote:
>
> On Thu, May 29, 2025 at 09:31:40AM -0700, Mina Almasry wrote:
> > On Wed, May 28, 2025 at 8:11 PM Byungchul Park <byungchul@...com> wrote:
> > > struct net_iov {
> > > - enum net_iov_type type;
> > > - unsigned long pp_magic;
> > > - struct page_pool *pp;
> > > - struct net_iov_area *owner;
> > > - unsigned long dma_addr;
> > > - atomic_long_t pp_ref_count;
> > > + union {
> > > + struct netmem_desc desc;
> > > +
> > > + /* XXX: The following part should be removed once all
> > > + * the references to them are converted so as to be
> > > + * accessed via netmem_desc e.g. niov->desc.pp instead
> > > + * of niov->pp.
> > > + *
> > > + * Plus, once struct netmem_desc has it own instance
> > > + * from slab, network's fields of the following can be
> > > + * moved out of struct netmem_desc like:
> > > + *
> > > + * struct net_iov {
> > > + * struct netmem_desc desc;
> > > + * struct net_iov_area *owner;
> > > + * ...
> > > + * };
> > > + */
> >
> > We do not need to wait until netmem_desc has its own instance from
> > slab to move the net_iov-specific fields out of netmem_desc. We can do
> > that now, because there are no size restrictions on net_iov.
>
> Got it. Thanks for explanation.
>
> > So, I recommend change this to:
> >
> > struct net_iov {
> > /* Union for anonymous aliasing: */
> > union {
> > struct netmem_desc desc;
> > struct {
> > unsigned long _flags;
> > unsigned long pp_magic;
> > struct page_pool *pp;
> > unsigned long _pp_mapping_pad;
> > unsigned long dma_addr;
> > atomic_long_t pp_ref_count;
> > };
> > struct net_iov_area *owner;
> > enum net_iov_type type;
> > };
>
> Do you mean?
>
> struct net_iov {
> /* Union for anonymous aliasing: */
> union {
> struct netmem_desc desc;
> struct {
> unsigned long _flags;
> unsigned long pp_magic;
> struct page_pool *pp;
> unsigned long _pp_mapping_pad;
> unsigned long dma_addr;
> atomic_long_t pp_ref_count;
> };
> };
> struct net_iov_area *owner;
> enum net_iov_type type;
> };
>
> Right? If so, I will.
>
Yes, sounds good.
Also, maybe having a union with the same fields for anonymous aliasing
can be error prone if someone updates netmem_desc and forgets to
update the mirror in struct net_iov. If you can think of a way to deal
with that, great, if not lets maybe put a comment on top of struct
netmem_desc:
/* Do not update the fields in netmem_desc without also updating the
anonymous aliasing union in struct net_iov */.
Or something like that.
--
Thanks,
Mina
Powered by blists - more mailing lists