Currently page_pool_alloc_frag() is not supported in 32-bit
arch with 64-bit DMA because of the overlap issue between
pp_frag_count and dma_addr_upper in 'struct page' for those
arches, which seems to be quite common, see [1], which means
driver may need to handle it when using frag API.
In order to simplify the driver's work when using frag API
this patch allows page_pool_alloc_frag() to call
page_pool_alloc_pages() to return pages for those arches.
mlx5 calls page_pool_create() with PP_FLAG_PAGE_FRAG and is not
using the frag API, and the PP_FLAG_PAGE_FRAG checking will now
be removed in this patch, a new checking for the mlx5 driver is
needed to retain the old behavior. As we have not come up with a
better name yet, so PAGE_POOL_DMA_USE_PP_FRAG_COUNT is reused
here.
Note that it may aggravate truesize underestimate problem for
skb as there is no page splitting for those pages, if driver
need a accurate truesize, it may calculate that according to
frag size, page order and PAGE_POOL_DMA_USE_PP_FRAG_COUNT
being true or not. And we may provide a helper for that if it
turns out to be helpful.
1. https://lore.kernel.org/all/20211117075652.58299-1-linyunsheng@huawei.com/
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
CC: Lorenzo Bianconi <lorenzo@kernel.org>
CC: Alexander Duyck <alexander.duyck@gmail.com>
CC: Liang Chen <liangchen.linux@gmail.com>
CC: Alexander Lobakin <aleksander.lobakin@intel.com>
---
.../net/ethernet/mellanox/mlx5/core/en_main.c | 10 +++++
include/net/page_pool.h | 45 ++++++++++++++++---
net/core/page_pool.c | 18 ++------
3 files changed, 52 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index a7c526ee5024..42ccb77a1b6c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -832,6 +832,16 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
/* Create a page_pool and register it with rxq */
struct page_pool_params pp_params = { 0 };
+ /* Return error here to avoid mlx5e_page_release_fragmented()
+ * calling page_pool_defrag_page() to write to pp_frag_count
+ * which is overlapped with dma_addr_upper in 'struct page' for
+ * arch with PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
+ */
+ if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
+ err = -EINVAL;
+ goto err_free_by_rq_type;
+ }
+
pp_params.order = 0;
pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV | PP_FLAG_PAGE_FRAG;
pp_params.pool_size = pool_size;
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 126f9e294389..83bd13491105 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -32,7 +32,7 @@
#include <linux/mm.h> /* Needed by ptr_ring */
#include <linux/ptr_ring.h>
-#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
#define PP_FLAG_DMA_MAP BIT(0) /* Should page_pool do the DMA
* map/unmap
@@ -50,6 +50,9 @@
PP_FLAG_DMA_SYNC_DEV |\
PP_FLAG_PAGE_FRAG)
+#define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \
+ (sizeof(dma_addr_t) > sizeof(unsigned long))
+
/*
* Fast allocation side cache array/stack
*
@@ -219,8 +222,33 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
return page_pool_alloc_pages(pool, gfp);
}
-struct page *page_pool_alloc_frag(struct page_pool *pool, unsigned int *offset,
- unsigned int size, gfp_t gfp);
+struct page *__page_pool_alloc_frag(struct page_pool *pool,
+ unsigned int *offset, unsigned int size,
+ gfp_t gfp);
+
+static inline struct page *page_pool_alloc_frag(struct page_pool *pool,
+ unsigned int *offset,
+ unsigned int size, gfp_t gfp)
+{
+ unsigned int max_size = PAGE_SIZE << pool->p.order;
+
+ size = ALIGN(size, dma_get_cache_alignment());
+
+ if (WARN_ON(!(pool->p.flags & PP_FLAG_PAGE_FRAG) ||
+ size > max_size))
+ return NULL;
+
+ /* Don't allow page splitting and allocate one big frag
+ * for 32-bit arch with 64-bit DMA, corresponding to
+ * the checking in page_pool_is_last_frag().
+ */
+ if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
+ *offset = 0;
+ return page_pool_alloc_pages(pool, gfp);
+ }
+
+ return __page_pool_alloc_frag(pool, offset, size, gfp);
+}
static inline struct page *page_pool_dev_alloc_frag(struct page_pool *pool,
unsigned int *offset,
@@ -322,8 +350,14 @@ static inline long page_pool_defrag_page(struct page *page, long nr)
static inline bool page_pool_is_last_frag(struct page_pool *pool,
struct page *page)
{
- /* If fragments aren't enabled or count is 0 we were the last user */
+ /* We assume we are the last frag user that is still holding
+ * on to the page if:
+ * 1. Fragments aren't enabled.
+ * 2. We are running in 32-bit arch with 64-bit DMA.
+ * 3. page_pool_defrag_page() indicate we are the last user.
+ */
return !(pool->p.flags & PP_FLAG_PAGE_FRAG) ||
+ PAGE_POOL_DMA_USE_PP_FRAG_COUNT ||
(page_pool_defrag_page(page, 1) == 0);
}
@@ -357,9 +391,6 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
page_pool_put_full_page(pool, page, true);
}
-#define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \
- (sizeof(dma_addr_t) > sizeof(unsigned long))
-
static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
{
dma_addr_t ret = page->dma_addr;
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a3e12a61d456..9c4118c62997 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -14,7 +14,6 @@
#include <net/xdp.h>
#include <linux/dma-direction.h>
-#include <linux/dma-mapping.h>
#include <linux/page-flags.h>
#include <linux/mm.h> /* for put_page() */
#include <linux/poison.h>
@@ -200,10 +199,6 @@ static int page_pool_init(struct page_pool *pool,
*/
}
- if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
- pool->p.flags & PP_FLAG_PAGE_FRAG)
- return -EINVAL;
-
#ifdef CONFIG_PAGE_POOL_STATS
pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
if (!pool->recycle_stats)
@@ -715,18 +710,13 @@ static void page_pool_free_frag(struct page_pool *pool)
page_pool_return_page(pool, page);
}
-struct page *page_pool_alloc_frag(struct page_pool *pool,
- unsigned int *offset,
- unsigned int size, gfp_t gfp)
+struct page *__page_pool_alloc_frag(struct page_pool *pool,
+ unsigned int *offset,
+ unsigned int size, gfp_t gfp)
{
unsigned int max_size = PAGE_SIZE << pool->p.order;
struct page *page = pool->frag_page;
- if (WARN_ON(!(pool->p.flags & PP_FLAG_PAGE_FRAG) ||
- size > max_size))
- return NULL;
-
- size = ALIGN(size, dma_get_cache_alignment());
*offset = pool->frag_offset;
if (page && *offset + size > max_size) {
@@ -759,7 +749,7 @@ struct page *page_pool_alloc_frag(struct page_pool *pool,
alloc_stat_inc(pool, fast);
return page;
}
-EXPORT_SYMBOL(page_pool_alloc_frag);
+EXPORT_SYMBOL(__page_pool_alloc_frag);
static void page_pool_empty_ring(struct page_pool *pool)
{
--
2.33.0On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:
> + /* Return error here to avoid mlx5e_page_release_fragmented()
> + * calling page_pool_defrag_page() to write to pp_frag_count
> + * which is overlapped with dma_addr_upper in 'struct page' for
> + * arch with PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
> + */
> + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
> + err = -EINVAL;
> + goto err_free_by_rq_type;
> + }
I told you not to do this in a comment on v4.
Keep the flag in page pool params and let the creation fail.
On 2023/7/8 7:59, Jakub Kicinski wrote:
> On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:
>> + /* Return error here to avoid mlx5e_page_release_fragmented()
>> + * calling page_pool_defrag_page() to write to pp_frag_count
>> + * which is overlapped with dma_addr_upper in 'struct page' for
>> + * arch with PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
>> + */
>> + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
>> + err = -EINVAL;
>> + goto err_free_by_rq_type;
>> + }
>
> I told you not to do this in a comment on v4.
> Keep the flag in page pool params and let the creation fail.
There seems to be naming disagreement in the previous discussion,
The simplest way seems to be reuse the
PAGE_POOL_DMA_USE_PP_FRAG_COUNT and do the checking in the driver
without introducing new macro or changing macro name.
Let's be more specific about what is your suggestion here:
Do you mean keep the PP_FLAG_PAGE_FRAG flag and keep the below
checking in page_pool_init(), right?
if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
pool->p.flags & PP_FLAG_PAGE_FRAG)
return -EINVAL;
Isn't it confusing to still say page frag is not supported
for PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true case when this
patch will now add support for it, at least from API POV, the
page_pool_alloc_frag() is always supported now.
Maybe remove the PP_FLAG_PAGE_FRAG and add a new macro named
PP_FLAG_PAGE_SPLIT_IN_DRIVER, and do the checking as before in
page_pool_init() like below?
if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
pool->p.flags & PP_FLAG_PAGE_SPLIT_IN_DRIVER)
return -EINVAL;
Or any better suggestion?
On Sun, 9 Jul 2023 20:39:45 +0800 Yunsheng Lin wrote:
> On 2023/7/8 7:59, Jakub Kicinski wrote:
> > On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:
> >> + /* Return error here to avoid mlx5e_page_release_fragmented()
> >> + * calling page_pool_defrag_page() to write to pp_frag_count
> >> + * which is overlapped with dma_addr_upper in 'struct page' for
> >> + * arch with PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
> >> + */
> >> + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
> >> + err = -EINVAL;
> >> + goto err_free_by_rq_type;
> >> + }
> >
> > I told you not to do this in a comment on v4.
> > Keep the flag in page pool params and let the creation fail.
>
> There seems to be naming disagreement in the previous discussion,
> The simplest way seems to be reuse the
> PAGE_POOL_DMA_USE_PP_FRAG_COUNT and do the checking in the driver
> without introducing new macro or changing macro name.
>
> Let's be more specific about what is your suggestion here:
> Do you mean keep the PP_FLAG_PAGE_FRAG flag and keep the below
> checking in page_pool_init(), right?
> if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
> pool->p.flags & PP_FLAG_PAGE_FRAG)
> return -EINVAL;
>
> Isn't it confusing to still say page frag is not supported
> for PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true case when this
> patch will now add support for it, at least from API POV, the
> page_pool_alloc_frag() is always supported now.
I don't mind what the flag is called, I just want the check to stay
inside the page_pool code, acting on driver info passed inside
pp_params.
> Maybe remove the PP_FLAG_PAGE_FRAG and add a new macro named
> PP_FLAG_PAGE_SPLIT_IN_DRIVER, and do the checking as before in
> page_pool_init() like below?
> if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
> pool->p.flags & PP_FLAG_PAGE_SPLIT_IN_DRIVER)
> return -EINVAL;
Yup, that sound good.
On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote: > -#include <linux/dma-direction.h> > +#include <linux/dma-mapping.h> And the include is still here, too, eh..
On 2023/7/8 8:01, Jakub Kicinski wrote: > On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote: >> -#include <linux/dma-direction.h> >> +#include <linux/dma-mapping.h> > > 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? >
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?
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
On Tue, 11 Jul 2023 12:59:00 +0200 Alexander Lobakin wrote: > 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). FWIW I was thinking about the bigints recently, and from ynl perspective I think we may want two flavors :( One which is at most the length of platform's long long, and another which is always a bigint. The latter will be more work for user space to handle, so given 99% of use cases don't need more than 64b we should make its life easier? > 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 Well, once you have to rebuilding 100+ files it gets pretty hard to clean things up ;) I think I described the preferred setup, previously: $path/page_pool.h: #include <$path/page_pool/types.h> #include <$path/page_pool/helpers.h> $path/page_pool/types.h - has types $path/page_pool/helpers.h - has all the inlines C sources can include $path/page_pool.h, headers should generally only include $path/page_pool/types.h.
From: Jakub Kicinski <kuba@kernel.org> Date: Tue, 11 Jul 2023 09:37:05 -0700 > On Tue, 11 Jul 2023 12:59:00 +0200 Alexander Lobakin wrote: >> 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). > > FWIW I was thinking about the bigints recently, and from ynl > perspective I think we may want two flavors :( One which is at > most the length of platform's long long, and another which is (not sure we shouldn't split a separate thread off this one at this point :D) `long long` or `long`? `long long` is always 64-bit unless I'm missing something. On my 32-bit MIPS they were :D If `long long`, what's the point then if we have %NLA_U64 and would still have to add dumb padding attrs? :D I thought the idea was to carry 64+ bits encapsulated in 32-bit primitives. > always a bigint. The latter will be more work for user space > to handle, so given 99% of use cases don't need more than 64b > we should make its life easier? > >> 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 > > Well, once you have to rebuilding 100+ files it gets pretty hard to > clean things up ;) > > I think I described the preferred setup, previously: > > $path/page_pool.h: > > #include <$path/page_pool/types.h> > #include <$path/page_pool/helpers.h> > > $path/page_pool/types.h - has types > $path/page_pool/helpers.h - has all the inlines > > C sources can include $path/page_pool.h, headers should generally only > include $path/page_pool/types.h. Aaah okay, I did read it backwards ._. Moreover, generic stack barely uses PP's inlines, it needs externals mostly. Thanks, Olek
On Tue, 11 Jul 2023 18:59:51 +0200 Alexander Lobakin wrote: > From: Jakub Kicinski <kuba@kernel.org> > Date: Tue, 11 Jul 2023 09:37:05 -0700 > > > On Tue, 11 Jul 2023 12:59:00 +0200 Alexander Lobakin wrote: > >> 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). > > > > FWIW I was thinking about the bigints recently, and from ynl > > perspective I think we may want two flavors :( One which is at > > most the length of platform's long long, and another which is > > `long long` or `long`? `long long` is always 64-bit unless I'm missing > something. On my 32-bit MIPS they were :D > If `long long`, what's the point then if we have %NLA_U64 and would > still have to add dumb padding attrs? :D I thought the idea was to carry > 64+ bits encapsulated in 32-bit primitives. Sorry I confused things. Keep in mind we're only talking about what the generated YNL code ends up looking like, not the "wire" format. So we still "transport" things as multiple 32b chunks at netlink level. No padding. The question is how to render the C / C++ code on the YNL side (or any practical library). Are we storing all those values as bigints and require users to coerce them to a more natural type on each access? That'd defeat the goal of the new int type becoming the default / "don't overthink the sizing" type. If we have a subtype with a max size of 64b, it can be 32b or 64b on the wire, as needed, but user space can feel assured that u64 will always be able to store the result. The long long is my misguided attempt to be platform dependent. I think a better way of putting it would actually be 2 * sizeof(long). That way we can use u128 as max, which seems to only be defined on 64b platforms. But that's just a random thought, I'm not sure how useful it would be. Perhaps we need two types, one "basic" which tops out at 64b and one "really bigint" which can be used as bitmaps as well?
On 2023/7/12 0:59, Alexander Lobakin wrote: > From: Jakub Kicinski <kuba@kernel.org> > Date: Tue, 11 Jul 2023 09:37:05 -0700 > >> On Tue, 11 Jul 2023 12:59:00 +0200 Alexander Lobakin wrote: >>> 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). >> >> FWIW I was thinking about the bigints recently, and from ynl >> perspective I think we may want two flavors :( One which is at >> most the length of platform's long long, and another which is > > (not sure we shouldn't split a separate thread off this one at this > point :D) > > `long long` or `long`? `long long` is always 64-bit unless I'm missing > something. On my 32-bit MIPS they were :D > If `long long`, what's the point then if we have %NLA_U64 and would > still have to add dumb padding attrs? :D I thought the idea was to carry > 64+ bits encapsulated in 32-bit primitives. > >> always a bigint. The latter will be more work for user space >> to handle, so given 99% of use cases don't need more than 64b >> we should make its life easier? >> >>> 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 >> >> Well, once you have to rebuilding 100+ files it gets pretty hard to >> clean things up ;) >> >> I think I described the preferred setup, previously: >> >> $path/page_pool.h: >> >> #include <$path/page_pool/types.h> >> #include <$path/page_pool/helpers.h> >> >> $path/page_pool/types.h - has types >> $path/page_pool/helpers.h - has all the inlines >> >> C sources can include $path/page_pool.h, headers should generally only >> include $path/page_pool/types.h. Does spliting the page_pool.h as above fix the problem about including a ton of static inline functions from "linux/dma-mapping.h" in skbuff.c? As the $path/page_pool/helpers.h which uses dma_get_cache_alignment() must include the "linux/dma-mapping.h" which has dma_get_cache_alignment() defining as a static inline function. and if skbuff.c include $path/page_pool.h or $path/page_pool/helpers.h, doesn't we still have the same problem? Or do I misunderstand something here? > > Aaah okay, I did read it backwards ._. Moreover, generic stack barely > uses PP's inlines, it needs externals mostly. > > Thanks, > Olek > > . >
On Wed, 12 Jul 2023 20:34:12 +0800 Yunsheng Lin wrote: > >> C sources can include $path/page_pool.h, headers should generally only > >> include $path/page_pool/types.h. > > Does spliting the page_pool.h as above fix the problem about including > a ton of static inline functions from "linux/dma-mapping.h" in skbuff.c? > > As the $path/page_pool/helpers.h which uses dma_get_cache_alignment() > must include the "linux/dma-mapping.h" which has dma_get_cache_alignment() > defining as a static inline function. > and if skbuff.c include $path/page_pool.h or $path/page_pool/helpers.h, > doesn't we still have the same problem? Or do I misunderstand something > here? I should have clarified that "types.h" should also include pure function declarations (and possibly static line wrappers like pure get/set functions which only need locally defined types). The skbuff.h only needs to include $path/page_pool/types.h, right? I know that Olek has a plan to remove the skbuff dependency completely but functionally / for any future dependencies - this should work?
On 2023/7/13 1:26, Jakub Kicinski wrote: > On Wed, 12 Jul 2023 20:34:12 +0800 Yunsheng Lin wrote: >>>> C sources can include $path/page_pool.h, headers should generally only >>>> include $path/page_pool/types.h. >> >> Does spliting the page_pool.h as above fix the problem about including >> a ton of static inline functions from "linux/dma-mapping.h" in skbuff.c? >> >> As the $path/page_pool/helpers.h which uses dma_get_cache_alignment() >> must include the "linux/dma-mapping.h" which has dma_get_cache_alignment() >> defining as a static inline function. >> and if skbuff.c include $path/page_pool.h or $path/page_pool/helpers.h, >> doesn't we still have the same problem? Or do I misunderstand something >> here? > > I should have clarified that "types.h" should also include pure > function declarations (and possibly static line wrappers like > pure get/set functions which only need locally defined types). So "types.h" is not supposed/allowed to include any header and it can include any function declarations and static line wrappers which do not depend on any other header? It means we need to forward declaring a lot of 'struct' type for function declarations, right? If it is the case, the "types.h" does not seems to match it's naming when we can not really define most of the 'struct' in "types.h", such as 'struct page_pool' need to include some header in order to have definition of 'struct delayed_work'. Similar issue for 'helpers.h', as it will include most of the definition of 'struct', which are not really helpers, right? > > The skbuff.h only needs to include $path/page_pool/types.h, right? It seems doable, it need trying to prove it is indeed that case. > > I know that Olek has a plan to remove the skbuff dependency completely > but functionally / for any future dependencies - this should work? I am not experienced and confident enough to say about this for now.
On 14/07/2023 14.16, Yunsheng Lin wrote: >> I know that Olek has a plan to remove the skbuff dependency completely >> but functionally / for any future dependencies - this should work? > > I am not experienced and confident enough to say about this for now. > A trick Eric once shared with me is this make command: make net/core/page_pool.i It will generate a file "net/core/page_pool.i" that kind of shows how the includes gets processed, I believe it is the C preprocess output. --Jesper
On Fri, 14 Jul 2023 20:16:34 +0800 Yunsheng Lin wrote: > > I should have clarified that "types.h" should also include pure > > function declarations (and possibly static line wrappers like > > pure get/set functions which only need locally defined types). > > So "types.h" is not supposed/allowed to include any header and > it can include any function declarations and static line wrappers > which do not depend on any other header? It means we need to forward > declaring a lot of 'struct' type for function declarations, right? Only those used in function prototypes. Pointers in structures are somewhat special and don't require fwd declaration. > If it is the case, the "types.h" does not seems to match it's > naming when we can not really define most of the 'struct' in "types.h", > such as 'struct page_pool' need to include some header in order to > have definition of 'struct delayed_work'. Obviously. And refcount.h, and types.h. > Similar issue for 'helpers.h', as it will include most of the > definition of 'struct', which are not really helpers, right?
On 2023/7/15 1:52, Jakub Kicinski wrote: > On Fri, 14 Jul 2023 20:16:34 +0800 Yunsheng Lin wrote: >>> I should have clarified that "types.h" should also include pure >>> function declarations (and possibly static line wrappers like >>> pure get/set functions which only need locally defined types). >> >> So "types.h" is not supposed/allowed to include any header and >> it can include any function declarations and static line wrappers >> which do not depend on any other header? It means we need to forward >> declaring a lot of 'struct' type for function declarations, right? > > Only those used in function prototypes. Pointers in structures > are somewhat special and don't require fwd declaration. I gave it a try to split it, and something as below come out: https://github.com/gestionlin/linux/commit/11ac8c1959f7eda06a7b987903f37212b490b292 As the 'helpers.h' is not really useful when splitting, so only 'page_pool_types.h' is added, and include 'page_pool_types.h' in 'page_pool.h', does it make sense? As Alexander is sending a new RFC for the similar problem, I think we need to align on which is the better way to solve the problem.
On Mon, 17 Jul 2023 20:33:05 +0800 Yunsheng Lin wrote: > > Only those used in function prototypes. Pointers in structures > > are somewhat special and don't require fwd declaration. > > I gave it a try to split it, and something as below come out: > > https://github.com/gestionlin/linux/commit/11ac8c1959f7eda06a7b987903f37212b490b292 > > As the 'helpers.h' is not really useful when splitting, so only > 'page_pool_types.h' is added, and include 'page_pool_types.h' in > 'page_pool.h', does it make sense? > > As Alexander is sending a new RFC for the similar problem, I think > we need to align on which is the better way to solve the problem. LGTM, thanks!
From: Jakub Kicinski <kuba@kernel.org> Date: Tue, 18 Jul 2023 11:16:21 -0700 > On Mon, 17 Jul 2023 20:33:05 +0800 Yunsheng Lin wrote: >>> Only those used in function prototypes. Pointers in structures >>> are somewhat special and don't require fwd declaration. >> >> I gave it a try to split it, and something as below come out: >> >> https://github.com/gestionlin/linux/commit/11ac8c1959f7eda06a7b987903f37212b490b292 >> >> As the 'helpers.h' is not really useful when splitting, so only >> 'page_pool_types.h' is added, and include 'page_pool_types.h' in >> 'page_pool.h', does it make sense? >> >> As Alexander is sending a new RFC for the similar problem, I think >> we need to align on which is the better way to solve the problem. > > LGTM, thanks! Looks nice to me as well. Re "which way is better" -- they can coexist actually. skbuff.h won't lose anything if doesn't include any PP headers at all after my commit, while yours also adds some future-proofing, as you never know when the same story happens to some other header file. (BTW it would be nice to inspect page_pool.h users whether each of them needs the full-blown header) Thanks, Olek
On 2023/7/19 2:28, Alexander Lobakin wrote: > > (BTW it would be nice to inspect page_pool.h users whether each of them > needs the full-blown header) It seems other page_pool.h users are all drivers, so the full-blown header is needed. > > Thanks, > Olek > . >
© 2016 - 2024 Red Hat, Inc.