Message-ID: <CAHS8izOBkDdMZhKYegMWYsmEEkzFBrBf6Ds3nenL5AZeKq7-UA@mail.gmail.com>
Date: Wed, 16 Apr 2025 09:02:02 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Byungchul Park <byungchul@...com>
Cc: Jesper Dangaard Brouer <hawk@...nel.org>, netdev <netdev@...r.kernel.org>, willy@...radead.org, 
	ilias.apalodimas@...aro.org, kernel_team@...ynix.com, 42.hyeyoo@...il.com, 
	linux-mm@...ck.org
Subject: Re: [RFC] shrinking struct page (part of page pool)

On Tue, Apr 15, 2025 at 10:24 PM Byungchul Park <byungchul@...com> wrote:
>
> On Tue, Apr 15, 2025 at 08:39:47AM -0700, Mina Almasry wrote:
> > On Sun, Apr 13, 2025 at 6:36 PM Byungchul Park <byungchul@...com> wrote:
> > >
> > > Hi guys,
> > >
> > > I'm looking at network's page pool code to help 'shrinking struct page'
> > > project by Matthew Wilcox.  See the following link:
> > >
> > >    https://kernelnewbies.org/MatthewWilcox/Memdescs/Path
> > >
> > > My first goal is to remove fields for page pool from struct page like:
> > >
> >
> > Remove them, but put them where? The page above specificies "Split the
>
> We need to introduce a new struct that will be used as a new descriptor
> e.g. bump, instead of struct page, similar to net_iov, overlaying struct
> page for now.
>
> > pagepool bump allocator out of struct page, as has been done for, eg,
> > slab and ptdesc.", but I'm not familiar what happened with slab and
> > ptdesc. Are these fields moving to a different location? Or being
>
> Move to the newly introduced struct e.g. bump and temporarily let it
> overlay struct page for now.
>
> > somehow removed entirely?
>
> And then we can remove the fields from struct page.
>

OK, IIUC, what you're trying to do is fairly straightforward actually.

We already have struct net_iov which overlays the page_pool entries in
struct page, and we use it to represent non-paged memory.

You can create struct bump which overlays the page_pool entries in
struct page (just like net_iov does), and modify all the places in the
net stack and page_pool where we query these entries to query the
entries from the struct bump instead of from the struct page.

> > >    struct {     /* page_pool used by netstack */
> > >         /**
> > >          * @pp_magic: magic value to avoid recycling non
> > >          * page_pool allocated pages.
> > >          */
> > >         unsigned long pp_magic;
> > >         struct page_pool *pp;
> > >         unsigned long _pp_mapping_pad;
> > >         unsigned long dma_addr;
> > >         atomic_long_t pp_ref_count;
> > >    };
> > >
> > > Fortunately, many prerequisite works have been done by Mina but I guess
> > > he or she has done it for other purpose than 'shrinking struct page'.
> > >
> >
> > Yeah, we did it to support non-page memory in the net stack, which is
> > quite orthogonal to what you're trying to do AFAICT so far. Looks like
> > maybe some implementation details are shared by luck?
>
> Oh.
>
> > > I'd like to just finalize the work so that the fields above can be
> > > removed from struct page.  However, I need to resolve a curiousity
> > > before starting.
> > >
> > >    Network guys already introduced a sperate strcut, struct net_iov,
> > >    to overlay the interesting fields.  However, another separate struct
> > >    for system memory might be also needed e.g. struct bump so that
> > >    struct net_iov and struct bump can be overlayed depending on the
> > >    source:
> > >
> > >    struct bump {
> > >         unsigned long _page_flags;
> > >         unsigned long bump_magic;
> > >         struct page_pool *bump_pp;
> > >         unsigned long _pp_mapping_pad;
> > >         unsigned long dma_addr;
> > >         atomic_long_t bump_ref_count;
> > >         unsigned int _page_type;
> > >         atomic_t _refcount;
> > >    };
> > >
> > > To netwrok guys, any thoughts on it?
> >
> > Need more details. What does struct bump represent? If it's meant to
>
> 'bump' comes from how page pool works.  See the following link:
>
>    https://en.wikipedia.org/wiki/Region-based_memory_management
>
> However, any better name suggestion from network guys should be
> appreciated.
>
> > replace the fields used by the page_pool referenced above, then it
> > should not have _page_flags, bump_ref_count should be pp_ref_count,
> > and should not have _page_type or _refcount.
>
> These are place holders that might be needed for now but should be
> removed later.
>

I think they need to not be added at all, rather than removed later.
It makes little sense to me to have a _page_type or _refcount entries
in this bump struct when the original page_pool entries in struct page
don't have a _page_flags or _page_type or _refcount, but maybe I
misunderstood and looking at patches would make this clearer.

> > > To Willy, do I understand correctly your direction?
> > >
> > > Plus, it's a quite another issue but I'm curious, that is, what do you
> > > guys think about moving the bump allocator(= page pool) code from
> > > network to mm?  I'd like to start on the work once gathering opinion
> > > from both Willy and network guys.
> > >
> >
> > What is the terminology "bump"? Are you wanting to rename page_pool to
> > "bump"? What does the new name mean?
>
> I hope the link above explain it.
>

To be honest I would drop renaming the page_pool and moving the
page_pool to mm as part of your changes. Those seem to have very
little benefit for what you're trying to do, and what you're doing
seems straightforward enough while keeping the code in place, but
that's just my 2 cents.

-- 
Thanks,
Mina

Powered by blists - more mailing lists