Re: [PATCH v5 RFC 1/6] page_pool: frag API support for 32-bit arch with 64-bit DMA

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Jakub Kicinski <kuba@kernel.org>
Date: Mon, 10 Jul 2023 11:38:41 -0700

> On Sun, 9 Jul 2023 20:54:12 +0800 Yunsheng Lin wrote:
>>> And the include is still here, too, eh..  
>>
>> In V4, it has:
>>
>> --- a/include/net/page_pool.h
>> +++ b/include/net/page_pool.h
>> @@ -33,6 +33,7 @@ 
>>  #include <linux/mm.h> /* Needed by ptr_ring */
>>  #include <linux/ptr_ring.h>
>>  #include <linux/dma-direction.h>
>> +#include <linux/dma-mapping.h>
>>
>> As dma_get_cache_alignment() defined in dma-mapping.h is used
>> here, so we need to include dma-mapping.h.
>>
>> I though the agreement is that this patch only remove the
>> "#include <linux/dma-direction.h>" as we dma-mapping.h has included
>> dma-direction.h.
>>
>> And Alexander will work on excluding page_pool.h from skbuff.h
>> https://lore.kernel.org/all/09842498-b3ba-320d-be8d-348b85e8d525@intel.com/
>>
>> Did I miss something obvious here？ Or there is better way to do it
>> than the method discussed in the above thread?
> 
> We're adding a ton of static inline functions to what is a fairly core
> header for networking, that's what re-triggered by complaint:
> 
>  include/net/page_pool.h                       | 179 ++++++++++++++----
> 
> Maybe we should revisit the idea of creating a new header file for
> inline helpers... Olek, WDYT?

I'm fine with that, although ain't really able to work on this myself
now :s (BTW I almost finished Netlink bigints, just some more libie/IAVF
crap).
It just needs to be carefully designed, because if we want move ALL the
inlines to a new header, we may end up including 2 PP's headers in each
file. That's why I'd prefer "core/driver" separation. Let's say skbuff.c
doesn't need page_pool_create(), page_pool_alloc(), and so on, while
drivers don't need some of its internal functions.
OTOH after my patch it's included in only around 20-30 files on
allmodconfig. That is literally nothing comparing to e.g. kernel.h
(w/includes) :D

Thanks,
Olek