On Fri, Aug 01, 2025 at 04:08:05PM -0700, Jakub Kicinski wrote:
> On Tue, 29 Jul 2025 20:02:10 +0900 Byungchul Park wrote:
> > [PATCH linux-next v3] mm, page_pool: introduce a new page type for page pool in page type
>
> linux-next does not accept patches. This has to go either via networking or MM.
Yeah. That's what I found. Thanks for the confirmation.
> > - if (unlikely(page_has_type(page)))
> > + if (unlikely(page_has_type(page))) {
>
> Maybe add :
>
> /* networking expects to clear its page type before releasing */
I will.
> > + WARN_ON_ONCE(PageNetpp(page));
> > /* Reset the page_type (which overlays _mapcount) */
> > page->page_type = UINT_MAX;
> > + }
>
> > static inline bool netmem_is_pp(netmem_ref netmem)
> > {
> > - return (netmem_get_pp_magic(netmem) & PP_MAGIC_MASK) == PP_SIGNATURE;
> > + /* Use ->pp for net_iov to identify if it's pp,
>
> Please try to use precise language, this code is confusing as is.
> net_iov may _belong_ to a page pool.
>
> * which requires that non-pp net_iov should have ->pp NULL'd.
Thank you. I will.
> I don't think this adds any information.
>
> > + */
> > + if (netmem_is_net_iov(netmem))
> > + return !!__netmem_clear_lsb(netmem)->pp;
> > +
> > + /* For system memory, page type bit in struct page can be used
>
> "page type bit" -> "page type", it's not a bit.
>
> > + * to identify if it's pp.
>
> ... to identify pages which belong to a page pool.
I will.
> > + */
> > + return PageNetpp(__netmem_to_page(netmem));
> > }
> >
> > static inline void netmem_set_pp(netmem_ref netmem, struct page_pool *pool)
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 05e2e22a8f7c..37eeab76c41c 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -654,7 +654,6 @@ s32 page_pool_inflight(const struct page_pool *pool, bool strict)
> > void page_pool_set_pp_info(struct page_pool *pool, netmem_ref netmem)
> > {
> > netmem_set_pp(netmem, pool);
> > - netmem_or_pp_magic(netmem, PP_SIGNATURE);
> >
> > /* Ensuring all pages have been split into one fragment initially:
> > * page_pool_set_pp_info() is only called once for every page when it
> > @@ -665,12 +664,19 @@ void page_pool_set_pp_info(struct page_pool *pool, netmem_ref netmem)
> > page_pool_fragment_netmem(netmem, 1);
> > if (pool->has_init_callback)
> > pool->slow.init_callback(netmem, pool->slow.init_arg);
> > +
> > + /* If it's page-backed */
>
> Please don't add obvious comments.
I added the comment since it's _not_ obvious. !net_iov means it's not
net_iov, not it's page-backed. However, since you, as the maintainer,
are okay, I will remove it. Thanks.
Byungchul
> > + if (!netmem_is_net_iov(netmem))
> > + __SetPageNetpp(__netmem_to_page(netmem));
