Re: [RFC] net: esp: fix bad handling of pages from page_pool

[Mina Almasry]

On Wed, Mar 6, 2024 at 9:10 AM Dragos Tatulea <dtatulea@xxxxxxxxxx> wrote:
>
> On Wed, 2024-03-06 at 08:40 -0800, Mina Almasry wrote:
> > On Wed, Mar 6, 2024 at 8:09 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > >
> > > On Wed, 6 Mar 2024 16:00:46 +0000 Dragos Tatulea wrote:
> > > > > Hm, that's a judgment call.
> > > > > Part of me wants to put it next to napi_frag_unref(), since we
> > > > > basically need to factor out the insides of this function.
> > > > > When you post the patch the page pool crowd will give us
> > > > > their opinions.
> > > >
> > > > Why not have napi_pp_put_page simply return false if CONFIG_PAGE_POOL is not
> > > > set?
> > >
> > > Without LTO it may still be a function call.
> > > Plus, subjectively, I think that it's a bit too much logic to encode in
> > > the caller (you must also check skb->pp_recycle, AFAIU)
> > > Maybe we should make skb_pp_recycle() take struct page and move it to
> > > skbuff.h ? Rename it to skb_page_unref() ?
> > >
> >
> > Does the caller need to check skb->pp_recycle? pp_recycle seems like a
> > redundant bit. We can tell whether the page is pp by checking
> > is_pp_page(page). the pages in the frag must be pp pages when
> > skb->pp_recycle is set and must be non pp pages when the
> > skb->pp_recycle is not set, so it all seems redundant to me.
> >
> AFAIU we don't have to check for pp_recycle, at least not in this specific case.
>
> > My fix would be something like:
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index d577e0bee18d..cc737b7b9860 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -3507,17 +3507,25 @@ int skb_cow_data_for_xdp(struct page_pool
> > *pool, struct sk_buff **pskb,
> >  bool napi_pp_put_page(struct page *page, bool napi_safe);
> >
> >  static inline void
> > -napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> > +napi_page_unref(struct page *page, bool napi_safe)
> >  {
> > -       struct page *page = skb_frag_page(frag);
> > -
> >  #ifdef CONFIG_PAGE_POOL
> > -       if (recycle && napi_pp_put_page(page, napi_safe))
> > +       if (napi_pp_put_page(page, napi_safe))
> >                 return;
> >  #endif
> >         put_page(page);
> >  }
> >
> > +static inline void
> > +napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> > +{
> > +       struct page *page = skb_frag_page(frag);
> > +
> > +       DEBUG_NET_WARN_ON(recycle != is_pp_page(page));
> > +
> > +       napi_page_unref(page);
> > +}
> > +
> >
> > And then use napi_page_unref() in the callers to handle page pool &
> > non-page pool gracefully without leaking page pool internals to the
> > callers.
> >
> We'd also need to add is_pp_page() in the header with the changes above...
>

Gah, I did not realize skbuff.h doesn't have is_pp_page(). Sorry!

> On that line of thought, unless these new APIs are useful for other use-cases,
> why not keep it simple:
> - Move is_pp_page() to skbuff.h.

I prefer this. is_pp_page() is a one-liner anyway.

> - Do a simple is_pp_page(page) ? page_pool_put_full_page(page):put_page(page) in
> the caller? Checking skb->pp_recycle would not be needed.
>

IMHO page pool internals should not leak to the callers like this.
There should be a helper that does the right thing for pp & non-pp
pages so that the caller can use it without worrying about subtle pp
concepts that they may miss at some callsites, but I'm fine either way
if there is strong disagreement from others.

> Thanks,
> Dragos
>
> > > > Regarding stable would I need to send a separate fix that does the raw pp page
> > > > check without the API?
> > >
> > > You can put them in one patch, I reckon.
> >
>


-- 
Thanks,
Mina