Re: [PATCH net-next v6 9/9] page_pool: access ->pp_magic through struct netmem_desc in page_pool_page_is_pp()

[Byungchul Park]

On Mon, Jun 23, 2025 at 07:14:41PM +0100, Pavel Begunkov wrote:
> On 6/23/25 18:28, Mina Almasry wrote:
> > On Mon, Jun 23, 2025 at 10:05 AM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote:
> ...>> As you said, it's just a sanity check, all page pool pages should
> > > be freed by the networking code. It checks the ownership with
> > > netmem_is_pp(), which is basically the same as page_pool_page_is_pp()
> > > but done though some aliasing.
> > > 
> > > static inline bool netmem_is_pp(netmem_ref netmem)
> > > {
> > >          return (netmem_get_pp_magic(netmem) & PP_MAGIC_MASK) == PP_SIGNATURE;
> > > }
> > > 
> > > I assume there is no point in moving the check to skbuff.c as it
> > > already does exactly same test, but we can probably just kill it.
> > > 
> > 
> > Even if we do kill it, maybe lets do that in a separate patch, and
> > maybe a separate series. I would recommend not complicating this one?
> FWIW, the discussion somewhat mentioned "long term", but I'm not
> suggesting actually removing it, it serves the purpose. And in
> long term the helper will be converted to use page->type / etc.
> without touching pp fields, that should reduce the degree of
> ugliness and make it more acceptable for keeping in mm.

Agree.

	Byungchul
> 
> > Also, AFAIU, this is about removing/moving the checks in
> > bad_page_reason() and page_expected_state()? I think this check does
> > fire sometimes. I saw at least 1 report in the last year of a
> > bad_page_reason() check firing because the page_pool got its
> > accounting wrong and released a page to the buddy allocator early, so
> > maybe that new patch that removes that check should explain why this
> > check is no longer necessary.
> 
> --
> Pavel Begunkov