Re: [RFC] shrinking struct page (part of page pool)

Re: [RFC] shrinking struct page (part of page pool)

[Byungchul Park]

On Mon, Apr 14, 2025 at 10:36:27AM +0900, Byungchul Park wrote:
> Hi guys,

+cc hawk@kernel.org
+cc netdev@vger.kernel.org

	Byungchul

> 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:
> 
>    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'.
> 
> 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?
> 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.
> 
> 	Byungchul

Re: [RFC] shrinking struct page (part of page pool)

[Jakub Kicinski]

On Mon, 14 Apr 2025 10:52:07 +0900 Byungchul Park wrote:
> > 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'.
> > 
> > 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.

I don't understand what the question is but FWIW from my perspective
the ZC APIs are fairly contained, or at least we tried to make sure
that net_iov pages cannot reach random parts of the stack.

Replacing all uses of struct page would require converting much more
of the stack, AFAIU. But that's best discussed over posted patches.

> >    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?
> > 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.

I don't see any benefit from moving page pool to MM. It is quite
networking specific. But we can discuss this later. Moving code
is trivial, it should not be the initial focus.

Re: [RFC] shrinking struct page (part of page pool)

[Byungchul Park]

On Mon, Apr 14, 2025 at 04:30:02PM -0700, Jakub Kicinski wrote:
> On Mon, 14 Apr 2025 10:52:07 +0900 Byungchul Park wrote:
> > > 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'.
> > > 
> > > 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.
> 
> I don't understand what the question is but FWIW from my perspective
> the ZC APIs are fairly contained, or at least we tried to make sure
> that net_iov pages cannot reach random parts of the stack.
> 
> Replacing all uses of struct page would require converting much more
> of the stack, AFAIU. But that's best discussed over posted patches.

Okay.  Let's discuss it once posting patches.

> > >    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?
> > > 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.
> 
> I don't see any benefit from moving page pool to MM. It is quite
> networking specific. But we can discuss this later. Moving code
> is trivial, it should not be the initial focus.

I think so.

	Byungchul

Re: [RFC] shrinking struct page (part of page pool)

[Ilias Apalodimas]

Hi Jakub

[...]

> > >
> > >    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?
> > > 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.
>
> I don't see any benefit from moving page pool to MM. It is quite
> networking specific. But we can discuss this later. Moving code
> is trivial, it should not be the initial focus.

Random thoughts here until I look at the patches.
The concept of devices doing DMA + recycling the used buffer
transcends networking. But I agree with you, that's something we can
discuss on the reviews.

Thanks
/Ilias

Re: [RFC] shrinking struct page (part of page pool)

[Andrew Lunn]

On Sat, May 10, 2025 at 10:02:59AM +0300, Ilias Apalodimas wrote:
> Hi Jakub
> 
> [...]
> 
> > > >
> > > >    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?
> > > > 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.
> >
> > I don't see any benefit from moving page pool to MM. It is quite
> > networking specific. But we can discuss this later. Moving code
> > is trivial, it should not be the initial focus.
> 
> Random thoughts here until I look at the patches.
> The concept of devices doing DMA + recycling the used buffer
> transcends networking.

Do you know of any other subsystem which takes a page, splits it into
two, and then uses each half independently for DMA and recycling. A
typical packet is 1514 octets, so you can get two in a page.

	Andrew

Re: [RFC] shrinking struct page (part of page pool)

[Christoph Hellwig]

On Sat, May 10, 2025 at 03:53:47PM +0200, Andrew Lunn wrote:
> > Random thoughts here until I look at the patches.
> > The concept of devices doing DMA + recycling the used buffer
> > transcends networking.
> 
> Do you know of any other subsystem which takes a page, splits it into
> two, and then uses each half independently for DMA and recycling. A
> typical packet is 1514 octets, so you can get two in a page.

The mm/dmapool.c code is all about this.

Re: [RFC] shrinking struct page (part of page pool)

[Ilias Apalodimas]

Hi Andrew

Apologies for the late reply,

On Sat, 10 May 2025 at 16:53, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, May 10, 2025 at 10:02:59AM +0300, Ilias Apalodimas wrote:
> > Hi Jakub
> >
> > [...]
> >
> > > > >
> > > > >    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?
> > > > > 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.
> > >
> > > I don't see any benefit from moving page pool to MM. It is quite
> > > networking specific. But we can discuss this later. Moving code
> > > is trivial, it should not be the initial focus.
> >
> > Random thoughts here until I look at the patches.
> > The concept of devices doing DMA + recycling the used buffer
> > transcends networking.
>
> Do you know of any other subsystem which takes a page, splits it into
> two, and then uses each half independently for DMA and recycling. A
> typical packet is 1514 octets, so you can get two in a page.

No, but OTOH the recycling is not somehow bound to having multiple
fragments of a page. So I assumed more subsystems would benefit from
not constantly re-allocating and re-mapping pages

Thanks
/Ilias
>
>         Andrew

Re: [RFC] shrinking struct page (part of page pool)

[Mina Almasry]

On Sun, Apr 13, 2025 at 6:36 PM Byungchul Park <byungchul@sk.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
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
somehow removed entirely?

>    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?

> 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
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.

> 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?

-- 
Thanks,
Mina

Re: [RFC] shrinking struct page (part of page pool)

[Byungchul Park]

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@sk.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.

> >    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.

> > 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.

	Byungchul

> 
> -- 
> Thanks,
> Mina

Re: [RFC] shrinking struct page (part of page pool)

[Mina Almasry]

On Tue, Apr 15, 2025 at 10:24 PM Byungchul Park <byungchul@sk.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@sk.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