On Tue, May 27, 2025 at 08:39:43PM -0700, Mina Almasry wrote:
> On Tue, May 27, 2025 at 7:29 PM Byungchul Park <byungchul@xxxxxx> wrote:
> >
> > To simplify struct page, the page pool members of struct page should be
> > moved to other, allowing these members to be removed from struct page.
> >
> > Introduce a network memory descriptor to store the members, struct
> > netmem_desc, reusing struct net_iov that already mirrored struct page.
> >
> > While at it, add a static assert to prevent the size of struct
> > netmem_desc from getting bigger that might conflict with other members
> > within struct page.
> >
> > Signed-off-by: Byungchul Park <byungchul@xxxxxx>
>
> This patch looks fine to me, but as of this series this change seems
> unnecessary, no? You're not using netmem_desc for anything in this
> series AFAICT. It may make sense to keep this series as
> straightforward renames, and have the series that introduces
> netmem_desc be the same one that is removing the page_pool fields from
> struct page or does something useful with it?
I didn't document well nor even work quite well for my purpose. I have
to admit I was also confused because the network code already had a
struct mirroring struct page, net_iov, so I thought it could be the
descriptor along with struct netmeme_desc until the day for page pool.
However, thanks to you, Pavel, and all, I understand better what net_iov
is for. I posted v3 with all the feedbacks applied, I guess the changes
in v3 are going to meet what you guys asked me.
On the top of v3, I think we can work on organizing struct net_iov to
make it look like:
struct net_iov {
struct netmem_desc desc;
net_iov_field1;
net_iov_field2;
net_iov_field3;
...
};
In order to make it, we should convert all the references to the
existing fields in struct net_iov, to access them indirectly using desc
field like e.g. 'net_iov->pp' to 'net_iov->desc.pp'. Once the
conversion is completed, we can make net_iov look better, which is not
urgent and I will not include the work in this series.
Please check v3:
https://lore.kernel.org/all/20250529031047.7587-1-byungchul@xxxxxx/
https://lore.kernel.org/all/20250529031047.7587-2-byungchul@xxxxxx/
Thanks to all.
Byungchul
> > ---
> > include/net/netmem.h | 41 +++++++++++++++++++++++++++++++++++------
> > 1 file changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > index 386164fb9c18..a721f9e060a2 100644
> > --- a/include/net/netmem.h
> > +++ b/include/net/netmem.h
> > @@ -31,12 +31,34 @@ enum net_iov_type {
> > };
> >
> > 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;
> > + /*
> > + * XXX: Now that struct netmem_desc overlays on struct page,
>
> This starting statement doesn't make sense to me TBH. netmem_desc
> doesn't seem to overlay struct page? For example, enum net_iov_type
> overlays unsigned long page->flags. Both have very different semantics
> and usage, no?
>
> > + * struct_group_tagged() should cover all of them. However,
> > + * a separate struct netmem_desc should be declared and embedded,
> > + * once struct netmem_desc is no longer overlayed but it has its
> > + * own instance from slab. The final form should be:
> > + *
>
> Honestly I'm not sure about putting future plans that aren't
> completely agreed upon in code comments. May be better to keep these
> future plans to the series that actually introduces them?
>
> > + * struct netmem_desc {
> > + * unsigned long pp_magic;
> > + * struct page_pool *pp;
> > + * unsigned long dma_addr;
> > + * atomic_long_t pp_ref_count;
> > + * };
> > + *
> > + * struct net_iov {
> > + * enum net_iov_type type;
> > + * struct net_iov_area *owner;
> > + * struct netmem_desc;
> > + * };
> > + */
> > + struct_group_tagged(netmem_desc, desc,
> > + 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;
> > + );
> > };
> >
> > struct net_iov_area {
> > @@ -73,6 +95,13 @@ NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
> > NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
> > #undef NET_IOV_ASSERT_OFFSET
> >
> > +/*
> > + * Since struct netmem_desc uses the space in struct page, the size
> > + * should be checked, until struct netmem_desc has its own instance from
> > + * slab, to avoid conflicting with other members within struct page.
> > + */
> > +static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
> > +
> > static inline struct net_iov_area *net_iov_owner(const struct net_iov *niov)
> > {
> > return niov->owner;
> > --
> > 2.17.1
> >
>
>
> --
> Thanks,
> Mina
