Messages in this thread | | | From | Alexander Duyck <> | Date | Fri, 30 Jun 2023 08:11:02 -0700 | Subject | Re: [PATCH RFC net-next 1/4] net: skbuff: don't include <net/page_pool.h> to <linux/skbuff.h> |
| |
On Fri, Jun 30, 2023 at 5:39 AM Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > > From: Alexander H Duyck <alexander.duyck@gmail.com> > Date: Thu, 29 Jun 2023 09:55:15 -0700 > > > On Thu, 2023-06-29 at 17:23 +0200, Alexander Lobakin wrote: > >> Currently, touching <net/page_pool.h> triggers a rebuild of more than > >> a half of the kernel. That's because it's included in <linux/skbuff.h>. > >> And each new include to page_pool.h adds more [useless] data for the > >> toolchain to process per each source file from that pile. > > [...] > > >> +bool page_pool_return_skb_page(struct page *page, bool napi_safe) > >> +{ > >> + struct napi_struct *napi; > >> + struct page_pool *pp; > >> + bool allow_direct; > >> + > >> + page = compound_head(page); > >> + pp = page->pp; > > > > So this is just assuming that any page we pass thru is a page pool > > page. The problem is there may be some other pointer stored here that > > could cause issues. > > But that is exactly what you suggested in the previous revision's > thread... Hey! :D > > "I suspect we could look at pulling parts of it out as well. The > pp_magic check should always be succeeding unless we have pages getting > routed the wrong way somewhere. So maybe we should look at pulling it > out and moving it to another part of the path such as > __page_pool_put_page() and making it a bit more visible to catch those > cases".
Yeah, I have had a few off days recently, amazing what happens when you are running on only a few hours of sleep.. :-/
Anyway, as I was saying it might make sense to wrap the whole thing up as a page pool accessor that would return NULL if the MAGIC value isn't present. Alternatively one other possibility would be to look at creating an inline function here that could check to see if the skb_frag is page pool and then just keep that in sk_buff.h since it looks like it should only need to rely on the page struct and PP_SIGNATURE which is a poison.h value. With that napi_frag_unref could avoid an unnecessary trip into the page_pool_return_skb_page function entirely if it isn't a page pool page and we could look at dropping the return value from page_pool_return_skb_page entirely.
> Anyway, since some drivers still mix PP pages with non-PP ones (mt76 > IIRC, maybe more), I feel the check for magic is still relevant. I just > believed you and forgot about that T.T
Yeah, sorry about that, my bad. I was a bit too focused on the main drivers we use and not thinking outside the box enough.
| |