Re: [PATCH net-next v3 06/12] net: skbuff: don't include <net/page_pool.h> into <linux/skbuff.h>

[Alexander Lobakin]

From: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
Date: Wed, 31 May 2023 17:28:30 +0200

> From: Alexander H Duyck <alexander.duyck@xxxxxxxxx>
> Date: Wed, 31 May 2023 08:21:03 -0700
> 
>> On Tue, 2023-05-30 at 17:00 +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>.
>>>
>>> In 6a5bcd84e886 ("page_pool: Allow drivers to hint on SKB recycling"),
>>> Matteo included it to be able to call a couple functions defined there.
>>> Then, in 57f05bc2ab24 ("page_pool: keep pp info as long as page pool
>>> owns the page") one of the calls was removed, so only one left.
>>> It's call to page_pool_return_skb_page() in napi_frag_unref(). The
>>> function is external and doesn't have any dependencies. Having include
>>> of very niche page_pool.h only for that looks like an overkill.
>>> Instead, move the declaration of that function to skbuff.h itself, with
>>> a small comment that it's a special guest and should not be touched.
>>> Now, after a few include fixes in the drivers, touching page_pool.h
>>> only triggers rebuilding of the drivers using it and a couple core
>>> networking files.
>>>
>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
>>> ---
>>>  drivers/net/ethernet/engleder/tsnep_main.c               | 1 +
>>>  drivers/net/ethernet/freescale/fec_main.c                | 1 +
>>>  drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 1 +
>>>  drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c     | 1 +
>>>  drivers/net/ethernet/mellanox/mlx5/core/en/params.c      | 1 +
>>>  drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c         | 1 +
>>>  drivers/net/wireless/mediatek/mt76/mt76.h                | 1 +
>>>  include/linux/skbuff.h                                   | 4 +++-
>>>  include/net/page_pool.h                                  | 2 --
>>>  9 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>>
>>
>> <...>
>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 5951904413ab..6d5eee932b95 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -32,7 +32,6 @@
>>>  #include <linux/if_packet.h>
>>>  #include <linux/llist.h>
>>>  #include <net/flow.h>
>>> -#include <net/page_pool.h>
>>>  #if IS_ENABLED(CONFIG_NF_CONNTRACK)
>>>  #include <linux/netfilter/nf_conntrack_common.h>
>>>  #endif
>>> @@ -3422,6 +3421,9 @@ static inline void skb_frag_ref(struct sk_buff *skb, int f)
>>>  	__skb_frag_ref(&skb_shinfo(skb)->frags[f]);
>>>  }
>>>  
>>> +/* Internal from net/core/page_pool.c, do not use in drivers directly */
>>> +bool page_pool_return_skb_page(struct page *page, bool napi_safe);
>>> +
>>>  static inline void
>>>  napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
>>>  {
>>> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>>> index 126f9e294389..2a9ce2aa6eb2 100644
>>> --- a/include/net/page_pool.h
>>> +++ b/include/net/page_pool.h
>>> @@ -240,8 +240,6 @@ inline enum dma_data_direction page_pool_get_dma_dir(struct page_pool *pool)
>>>  	return pool->p.dma_dir;
>>>  }
>>>  
>>> -bool page_pool_return_skb_page(struct page *page, bool napi_safe);
>>> -
>>>  struct page_pool *page_pool_create(const struct page_pool_params *params);
>>>  
>>>  struct xdp_mem_info;
>>
>> So the code as-is works, so I am providing my "Reviewed-by".
>> Reviewed-by: Alexander Duyck <alexanderduyck@xxxxxx>
>>
>> Consider the rest of this a suggestion or a "nice to have".
>>
>> I wonder if we shouldn't also look at restructuring the function and
>> just moving it to net/core/skbuff.c somewhere next to skb_pp_recycle.
>>
>> 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.
> 
> I've just noticed that this function is exported with no modular users ._.

^^^ pls ignore this nonsense lol

> Anyway, I feel like it's a good way to go. The entire function, apart
> from the magic check, can be moved and made static. And the magic can be
> moved one level up, right...
> v4 will happen either way I guess, so maybe I'll replace this patch with
> that kinda change.
> 
> Thanks,
> Olek