[PATCH v5 RFC 0/6] introduce page_pool_alloc() API

[PATCH v5 RFC 0/6] introduce page_pool_alloc() API

[Yunsheng Lin]

In [1] & [2] & [3], there are usecases for veth and virtio_net
to use frag support in page pool to reduce memory usage, and it
may request different frag size depending on the head/tail
room space for xdp_frame/shinfo and mtu/packet size. When the
requested frag size is large enough that a single page can not
be split into more than one frag, using frag support only have
performance penalty because of the extra frag count handling
for frag support.

So this patchset provides a page pool API for the driver to
allocate memory with least memory utilization and performance
penalty when it doesn't know the size of memory it need
beforehand.

1. https://patchwork.kernel.org/project/netdevbpf/patch/d3ae6bd3537fbce379382ac6a42f67e22f27ece2.1683896626.git.lorenzo@kernel.org/
2. https://patchwork.kernel.org/project/netdevbpf/patch/20230526054621.18371-3-liangchen.linux@gmail.com/
3. https://github.com/alobakin/linux/tree/iavf-pp-frag

v5 RFC: add a new page_pool_cache_alloc() API, and other minor
        change as discussed in v4. As there seems to be three
        comsumers that might be made use of the new API, so
        repost it as RFC and CC the relevant authors to see
        if the new API fits their need.

V4. Fix a typo and add a patch to update document about frag
    API, PAGE_POOL_DMA_USE_PP_FRAG_COUNT is not renamed yet
    as we may need a different thread to discuss that.

V3: Incorporate changes from the disscusion with Alexander,
    mostly the inline wraper, PAGE_POOL_DMA_USE_PP_FRAG_COUNT
    change split to separate patch and comment change.
V2: Add patch to remove PP_FLAG_PAGE_FRAG flags and mention
    virtio_net usecase in the cover letter.
V1: Drop RFC tag and page_pool_frag patch.

Yunsheng Lin (6):
  page_pool: frag API support for 32-bit arch with 64-bit DMA
  page_pool: unify frag_count handling in page_pool_is_last_frag()
  page_pool: introduce page_pool[_cache]_alloc() API
  page_pool: remove PP_FLAG_PAGE_FRAG flag
  page_pool: update document about frag API
  net: veth: use newly added page pool API for veth with xdp

 Documentation/networking/page_pool.rst        |  34 +++-
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |   3 +-
 .../marvell/octeontx2/nic/otx2_common.c       |   2 +-
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  12 +-
 drivers/net/veth.c                            |  24 ++-
 drivers/net/wireless/mediatek/mt76/mac80211.c |   2 +-
 include/net/page_pool.h                       | 179 ++++++++++++++----
 net/core/page_pool.c                          |  30 ++-
 net/core/skbuff.c                             |   2 +-
 9 files changed, 212 insertions(+), 76 deletions(-)

-- 
2.33.0

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

[Yunsheng Lin]

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.0

[PATCH v5 RFC 2/6] page_pool: unify frag_count handling in page_pool_is_last_frag()

[Yunsheng Lin]

Currently when page_pool_create() is called with
PP_FLAG_PAGE_FRAG flag, page_pool_alloc_pages() is only
allowed to be called under the below constraints:
1. page_pool_fragment_page() need to be called to setup
   page->pp_frag_count immediately.
2. page_pool_defrag_page() often need to be called to drain
   the page->pp_frag_count when there is no more user will
   be holding on to that page.

Those constraints exist in order to support a page to be
split into multi frags.

And those constraints have some overhead because of the
cache line dirtying/bouncing and atomic update.

Those constraints are unavoidable for case when we need a
page to be split into more than one frag, but there is also
case that we want to avoid the above constraints and their
overhead when a page can't be split as it can only hold a big
frag as requested by user, depending on different use cases:
use case 1: allocate page without page splitting.
use case 2: allocate page with page splitting.
use case 3: allocate page with or without page splitting
            depending on the frag size.

Currently page pool only provide page_pool_alloc_pages() and
page_pool_alloc_frag() API to enable the 1 & 2 separately,
so we can not use a combination of 1 & 2 to enable 3, it is
not possible yet because of the per page_pool flag
PP_FLAG_PAGE_FRAG.

So in order to allow allocating unsplit page without the
overhead of split page while still allow allocating split
page we need to remove the per page_pool flag in
page_pool_is_last_frag(), as best as I can think of, it seems
there are two methods as below:
1. Add per page flag/bit to indicate a page is split or
   not, which means we might need to update that flag/bit
   everytime the page is recycled, dirtying the cache line
   of 'struct page' for use case 1.
2. Unify the page->pp_frag_count handling for both split and
   unsplit page by assuming all pages in the page pool is split
   into a big frag initially.

As page pool already supports use case 1 without dirtying the
cache line of 'struct page' whenever a page is recyclable, we
need to support the above use case 3 with minimal overhead,
especially not adding any noticeable overhead for use case 1,
and we are already doing an optimization by not updating
pp_frag_count in page_pool_defrag_page() for the last frag
user, this patch chooses to unify the pp_frag_count handling
to support the above use case 3.

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>
---
 include/net/page_pool.h | 49 ++++++++++++++++++++++++++++++-----------
 net/core/page_pool.c    |  8 +++++++
 2 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 83bd13491105..bbbdd584cb7f 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -323,7 +323,8 @@ void page_pool_put_defragged_page(struct page_pool *pool, struct page *page,
  */
 static inline void page_pool_fragment_page(struct page *page, long nr)
 {
-	atomic_long_set(&page->pp_frag_count, nr);
+	if (!PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
+		atomic_long_set(&page->pp_frag_count, nr);
 }
 
 static inline long page_pool_defrag_page(struct page *page, long nr)
@@ -331,19 +332,43 @@ static inline long page_pool_defrag_page(struct page *page, long nr)
 	long ret;
 
 	/* If nr == pp_frag_count then we have cleared all remaining
-	 * references to the page. No need to actually overwrite it, instead
-	 * we can leave this to be overwritten by the calling function.
+	 * references to the page:
+	 * 1. 'n == 1': no need to actually overwrite it.
+	 * 2. 'n != 1': overwrite it with one, which is the rare case
+	 *              for frag draining.
 	 *
-	 * The main advantage to doing this is that an atomic_read is
-	 * generally a much cheaper operation than an atomic update,
-	 * especially when dealing with a page that may be partitioned
-	 * into only 2 or 3 pieces.
+	 * The main advantage to doing this is that not only we avoid a
+	 * atomic update, as an atomic_read is generally a much cheaper
+	 * operation than an atomic update, especially when dealing with
+	 * a page that may be partitioned into only 2 or 3 pieces; but
+	 * also unify the frag and non-frag handling by ensuring all
+	 * pages have been split into one big frag initially, and only
+	 * overwrite it when the page is split into more than one frag.
 	 */
-	if (atomic_long_read(&page->pp_frag_count) == nr)
+	if (atomic_long_read(&page->pp_frag_count) == nr) {
+		/* As we have ensured nr is always one for constant case
+		 * using the BUILD_BUG_ON(), only need to handle the
+		 * non-constant case here for frag count draining, which
+		 * is a rare case.
+		 */
+		BUILD_BUG_ON(__builtin_constant_p(nr) && nr != 1);
+		if (!__builtin_constant_p(nr))
+			atomic_long_set(&page->pp_frag_count, 1);
+
 		return 0;
+	}
 
 	ret = atomic_long_sub_return(nr, &page->pp_frag_count);
 	WARN_ON(ret < 0);
+
+	/* We are the last user here too, reset frag count back to 1 to
+	 * ensure all pages have been split into one big frag initially,
+	 * this should be the rare case when the last two frag users call
+	 * page_pool_defrag_page() currently.
+	 */
+	if (unlikely(!ret))
+		atomic_long_set(&page->pp_frag_count, 1);
+
 	return ret;
 }
 
@@ -352,12 +377,10 @@ static inline bool page_pool_is_last_frag(struct page_pool *pool,
 {
 	/* 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.
+	 * 1. We are running in 32-bit arch with 64-bit DMA.
+	 * 2. 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 ||
+	return PAGE_POOL_DMA_USE_PP_FRAG_COUNT ||
 	       (page_pool_defrag_page(page, 1) == 0);
 }
 
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 9c4118c62997..69e3c5175236 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -352,6 +352,14 @@ static void page_pool_set_pp_info(struct page_pool *pool,
 {
 	page->pp = pool;
 	page->pp_magic |= PP_SIGNATURE;
+
+	/* Ensuring all pages have been split into one big frag initially:
+	 * page_pool_set_pp_info() is only called once for every page when it
+	 * is allocated from the page allocator and page_pool_fragment_page()
+	 * is dirtying the same cache line as the page->pp_magic above, so
+	 * the overhead is negligible.
+	 */
+	page_pool_fragment_page(page, 1);
 	if (pool->p.init_callback)
 		pool->p.init_callback(page, pool->p.init_arg);
 }
-- 
2.33.0

[PATCH v5 RFC 3/6] page_pool: introduce page_pool[_cache]_alloc() API

[Yunsheng Lin]

Currently page pool supports the below use cases:
use case 1: allocate page without page splitting using
            page_pool_alloc_pages() API if the driver knows
            that the memory it need is always bigger than
            half of the page allocated from page pool.
use case 2: allocate page frag with page splitting using
            page_pool_alloc_frag() API if the driver knows
            that the memory it need is always smaller than
            or equal to the half of the page allocated from
            page pool.

There is emerging use case [1] & [2] that is a mix of the
above two case: the driver doesn't know the size of memory it
need beforehand, so the driver may use something like below to
allocate memory with least memory utilization and performance
penalty:

if (size << 1 > max_size)
	page = page_pool_alloc_pages();
else
	page = page_pool_alloc_frag();

To avoid the driver doing something like above, add the
page_pool[_cache]_alloc() API to support the above use case,
and update the true size of memory that is acctually allocated
by updating '*size' back to the driver in order to avoid
exacerbating truesize underestimate problem.

Rename page_pool_free() which is used in the destroy process to
__page_pool_destroy() to avoid confusion with the newly added
API.

1. https://lore.kernel.org/all/d3ae6bd3537fbce379382ac6a42f67e22f27ece2.1683896626.git.lorenzo@kernel.org/
2. https://lore.kernel.org/all/20230526054621.18371-3-liangchen.linux@gmail.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>
---
 include/net/page_pool.h | 70 +++++++++++++++++++++++++++++++++++++++++
 net/core/page_pool.c    |  4 +--
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index bbbdd584cb7f..53486ef9074d 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -259,6 +259,70 @@ static inline struct page *page_pool_dev_alloc_frag(struct page_pool *pool,
 	return page_pool_alloc_frag(pool, offset, size, gfp);
 }
 
+static inline struct page *page_pool_alloc(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;
+
+	*size = ALIGN(*size, dma_get_cache_alignment());
+
+	if (WARN_ON(*size > max_size))
+		return NULL;
+
+	if ((*size << 1) > max_size || PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
+		*size = max_size;
+		*offset = 0;
+		return page_pool_alloc_pages(pool, gfp);
+	}
+
+	page = __page_pool_alloc_frag(pool, offset, *size, gfp);
+	if (unlikely(!page))
+		return NULL;
+
+	/* There is very likely not enough space for another frag, so append the
+	 * remaining size to the current frag to avoid truesize underestimate
+	 * problem.
+	 */
+	if (pool->frag_offset + *size > max_size) {
+		*size = max_size - *offset;
+		pool->frag_offset = max_size;
+	}
+
+	return page;
+}
+
+static inline struct page *page_pool_dev_alloc(struct page_pool *pool,
+					       unsigned int *offset,
+					       unsigned int *size)
+{
+	gfp_t gfp = (GFP_ATOMIC | __GFP_NOWARN);
+
+	return page_pool_alloc(pool, offset, size, gfp);
+}
+
+static inline void *page_pool_cache_alloc(struct page_pool *pool,
+					  unsigned int *size, gfp_t gfp)
+{
+	unsigned int offset;
+	struct page *page;
+
+	page = page_pool_alloc(pool, &offset, size, gfp);
+	if (unlikely(!page))
+		return NULL;
+
+	return page_address(page) + offset;
+}
+
+static inline void *page_pool_dev_cache_alloc(struct page_pool *pool,
+					      unsigned int *size)
+{
+	gfp_t gfp = (GFP_ATOMIC | __GFP_NOWARN);
+
+	return page_pool_cache_alloc(pool, size, gfp);
+}
+
 /* get the stored dma direction. A driver might decide to treat this locally and
  * avoid the extra cache line from page_pool to determine the direction
  */
@@ -414,6 +478,12 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
 	page_pool_put_full_page(pool, page, true);
 }
 
+static inline void page_pool_cache_free(struct page_pool *pool, void *data,
+					bool allow_direct)
+{
+	page_pool_put_page(pool, virt_to_head_page(data), -1, allow_direct);
+}
+
 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 69e3c5175236..985ccaffc06a 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -774,7 +774,7 @@ static void page_pool_empty_ring(struct page_pool *pool)
 	}
 }
 
-static void page_pool_free(struct page_pool *pool)
+static void __page_pool_destroy(struct page_pool *pool)
 {
 	if (pool->disconnect)
 		pool->disconnect(pool);
@@ -825,7 +825,7 @@ static int page_pool_release(struct page_pool *pool)
 	page_pool_scrub(pool);
 	inflight = page_pool_inflight(pool);
 	if (!inflight)
-		page_pool_free(pool);
+		__page_pool_destroy(pool);
 
 	return inflight;
 }
-- 
2.33.0

[PATCH v5 RFC 4/6] page_pool: remove PP_FLAG_PAGE_FRAG flag

[Yunsheng Lin]

PP_FLAG_PAGE_FRAG is not really needed after pp_frag_count
handling is unified and page_pool_alloc_frag() is supported
in 32-bit arch with 64-bit DMA, so remove it.

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>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c          | 3 +--
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c        | 2 +-
 drivers/net/wireless/mediatek/mt76/mac80211.c            | 2 +-
 include/net/page_pool.h                                  | 7 ++-----
 net/core/skbuff.c                                        | 2 +-
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index b676496ec6d7..4e613d5bf1fd 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -4925,8 +4925,7 @@ static void hns3_put_ring_config(struct hns3_nic_priv *priv)
 static void hns3_alloc_page_pool(struct hns3_enet_ring *ring)
 {
 	struct page_pool_params pp_params = {
-		.flags = PP_FLAG_DMA_MAP | PP_FLAG_PAGE_FRAG |
-				PP_FLAG_DMA_SYNC_DEV,
+		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
 		.order = hns3_page_order(ring),
 		.pool_size = ring->desc_num * hns3_buf_size(ring) /
 				(PAGE_SIZE << hns3_page_order(ring)),
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index a79cb680bb23..404caec467af 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -1426,7 +1426,7 @@ int otx2_pool_init(struct otx2_nic *pfvf, u16 pool_id,
 		return 0;
 	}
 
-	pp_params.flags = PP_FLAG_PAGE_FRAG | PP_FLAG_DMA_MAP;
+	pp_params.flags = PP_FLAG_DMA_MAP;
 	pp_params.pool_size = numptrs;
 	pp_params.nid = NUMA_NO_NODE;
 	pp_params.dev = pfvf->dev;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 42ccb77a1b6c..cdd00848c0cb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -843,7 +843,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
 		}
 
 		pp_params.order     = 0;
-		pp_params.flags     = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV | PP_FLAG_PAGE_FRAG;
+		pp_params.flags     = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
 		pp_params.pool_size = pool_size;
 		pp_params.nid       = node;
 		pp_params.dev       = rq->pdev;
diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index 467afef98ba2..ee72869e5572 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -566,7 +566,7 @@ int mt76_create_page_pool(struct mt76_dev *dev, struct mt76_queue *q)
 {
 	struct page_pool_params pp_params = {
 		.order = 0,
-		.flags = PP_FLAG_PAGE_FRAG,
+		.flags = 0,
 		.nid = NUMA_NO_NODE,
 		.dev = dev->dma_dev,
 	};
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 53486ef9074d..e9fb95d62ed5 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -45,10 +45,8 @@
 					* Please note DMA-sync-for-CPU is still
 					* device driver responsibility
 					*/
-#define PP_FLAG_PAGE_FRAG	BIT(2) /* for page frag feature */
 #define PP_FLAG_ALL		(PP_FLAG_DMA_MAP |\
-				 PP_FLAG_DMA_SYNC_DEV |\
-				 PP_FLAG_PAGE_FRAG)
+				 PP_FLAG_DMA_SYNC_DEV)
 
 #define PAGE_POOL_DMA_USE_PP_FRAG_COUNT	\
 		(sizeof(dma_addr_t) > sizeof(unsigned long))
@@ -234,8 +232,7 @@ static inline struct page *page_pool_alloc_frag(struct page_pool *pool,
 
 	size = ALIGN(size, dma_get_cache_alignment());
 
-	if (WARN_ON(!(pool->p.flags & PP_FLAG_PAGE_FRAG) ||
-		    size > max_size))
+	if (WARN_ON(size > max_size))
 		return NULL;
 
 	/* Don't allow page splitting and allocate one big frag
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7c4338221b17..ca2316cc1e7e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5652,7 +5652,7 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 	/* In general, avoid mixing page_pool and non-page_pool allocated
 	 * pages within the same SKB. Additionally avoid dealing with clones
 	 * with page_pool pages, in case the SKB is using page_pool fragment
-	 * references (PP_FLAG_PAGE_FRAG). Since we only take full page
+	 * references (page_pool_alloc_frag()). Since we only take full page
 	 * references for cloned SKBs at the moment that would result in
 	 * inconsistent reference counts.
 	 * In theory we could take full references if @from is cloned and
-- 
2.33.0

[PATCH v5 RFC 5/6] page_pool: update document about frag API

[Yunsheng Lin]

As more drivers begin to use the frag API, update the
document about how to decide which API to use for the
driver author.

Also it seems there is a similar document in page_pool.h,
so remove it to avoid the duplication.

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>
---
 Documentation/networking/page_pool.rst | 34 ++++++++++++++++++++++----
 include/net/page_pool.h                | 22 -----------------
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/Documentation/networking/page_pool.rst b/Documentation/networking/page_pool.rst
index 873efd97f822..18b13d659c98 100644
--- a/Documentation/networking/page_pool.rst
+++ b/Documentation/networking/page_pool.rst
@@ -4,12 +4,27 @@
 Page Pool API
 =============
 
-The page_pool allocator is optimized for the XDP mode that uses one frame
-per-page, but it can fallback on the regular page allocator APIs.
+The page_pool allocator is optimized for recycling page or page frag used by skb
+packet and xdp frame.
 
-Basic use involves replacing alloc_pages() calls with the
-page_pool_alloc_pages() call.  Drivers should use page_pool_dev_alloc_pages()
-replacing dev_alloc_pages().
+Basic use involves replacing napi_alloc_frag() and alloc_pages() calls with
+page_pool_cache_alloc() and page_pool_alloc(), which allocate memory with or
+without page splitting depending on the requested memory size.
+
+If the driver knows that it always requires full pages or its allocates are
+always smaller than half a page, it can use one of the more specific API calls:
+
+1. page_pool_alloc_pages(): allocate memory without page splitting when driver
+   knows that the memory it need is always bigger than half of the page
+   allocated from page pool. There is no cache line dirtying for 'struct page'
+   when a page is recycled back to the page pool.
+
+2. page_pool_alloc_frag(): allocate memory with page splitting when driver knows
+   that the memory it need is always smaller than or equal to half of the page
+   allocated from page pool. Page splitting enables memory saving and thus avoid
+   TLB/cache miss for data access, but there also is some cost to implement page
+   splitting, mainly some cache line dirtying/bouncing for 'struct page' and
+   atomic operation for page->pp_frag_count.
 
 API keeps track of in-flight pages, in order to let API user know
 when it is safe to free a page_pool object.  Thus, API users
@@ -93,6 +108,15 @@ a page will cause no race conditions is enough.
 * page_pool_dev_alloc_pages(): Get a page from the page allocator or page_pool
   caches.
 
+* page_pool_dev_alloc_frag(): Get a page frag from the page allocator or
+  page_pool caches.
+
+* page_pool_dev_alloc(): Get a page or page frag from the page allocator or
+  page_pool caches.
+
+* page_pool_dev_cache_alloc(): Get a cache from the page allocator or page_pool
+  caches.
+
 * page_pool_get_dma_addr(): Retrieve the stored DMA address.
 
 * page_pool_get_dma_dir(): Retrieve the stored DMA direction.
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index e9fb95d62ed5..2b7db9992fc0 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -5,28 +5,6 @@
  *	Copyright (C) 2016 Red Hat, Inc.
  */
 
-/**
- * DOC: page_pool allocator
- *
- * This page_pool allocator is optimized for the XDP mode that
- * uses one-frame-per-page, but have fallbacks that act like the
- * regular page allocator APIs.
- *
- * Basic use involve replacing alloc_pages() calls with the
- * page_pool_alloc_pages() call.  Drivers should likely use
- * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
- *
- * API keeps track of in-flight pages, in-order to let API user know
- * when it is safe to dealloactor page_pool object.  Thus, API users
- * must make sure to call page_pool_release_page() when a page is
- * "leaving" the page_pool.  Or call page_pool_put_page() where
- * appropiate.  For maintaining correct accounting.
- *
- * API user must only call page_pool_put_page() once on a page, as it
- * will either recycle the page, or in case of elevated refcnt, it
- * will release the DMA mapping and in-flight state accounting.  We
- * hope to lift this requirement in the future.
- */
 #ifndef _NET_PAGE_POOL_H
 #define _NET_PAGE_POOL_H
 
-- 
2.33.0

[PATCH v5 RFC 6/6] net: veth: use newly added page pool API for veth with xdp

[Yunsheng Lin]

Use page_pool[_cache]_alloc() API to allocate memory with
least memory utilization and performance penalty.

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>
---
 drivers/net/veth.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 614f3e3efab0..d3dc754ba7f8 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -736,10 +736,11 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 	if (skb_shared(skb) || skb_head_is_locked(skb) ||
 	    skb_shinfo(skb)->nr_frags ||
 	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
-		u32 size, len, max_head_size, off;
+		u32 size, len, max_head_size, off, truesize, page_offset;
 		struct sk_buff *nskb;
 		struct page *page;
 		int i, head_off;
+		void *data;
 
 		/* We need a private copy of the skb and data buffers since
 		 * the ebpf program can modify it. We segment the original skb
@@ -752,14 +753,17 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 		if (skb->len > PAGE_SIZE * MAX_SKB_FRAGS + max_head_size)
 			goto drop;
 
+		size = min_t(u32, skb->len, max_head_size);
+		truesize = size;
+
 		/* Allocate skb head */
-		page = page_pool_dev_alloc_pages(rq->page_pool);
-		if (!page)
+		data = page_pool_dev_cache_alloc(rq->page_pool, &truesize);
+		if (!data)
 			goto drop;
 
-		nskb = napi_build_skb(page_address(page), PAGE_SIZE);
+		nskb = napi_build_skb(data, truesize);
 		if (!nskb) {
-			page_pool_put_full_page(rq->page_pool, page, true);
+			page_pool_cache_free(rq->page_pool, data, true);
 			goto drop;
 		}
 
@@ -767,7 +771,6 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 		skb_copy_header(nskb, skb);
 		skb_mark_for_recycle(nskb);
 
-		size = min_t(u32, skb->len, max_head_size);
 		if (skb_copy_bits(skb, 0, nskb->data, size)) {
 			consume_skb(nskb);
 			goto drop;
@@ -782,14 +785,17 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 		len = skb->len - off;
 
 		for (i = 0; i < MAX_SKB_FRAGS && off < skb->len; i++) {
-			page = page_pool_dev_alloc_pages(rq->page_pool);
+			size = min_t(u32, len, PAGE_SIZE);
+			truesize = size;
+
+			page = page_pool_dev_alloc(rq->page_pool, &page_offset,
+						   &truesize);
 			if (!page) {
 				consume_skb(nskb);
 				goto drop;
 			}
 
-			size = min_t(u32, len, PAGE_SIZE);
-			skb_add_rx_frag(nskb, i, page, 0, size, PAGE_SIZE);
+			skb_add_rx_frag(nskb, i, page, page_offset, size, truesize);
 			if (skb_copy_bits(skb, off, page_address(page),
 					  size)) {
 				consume_skb(nskb);
-- 
2.33.0

Re: [PATCH v5 RFC 0/6] introduce page_pool_alloc() API

[Alexander Lobakin]

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Thu, 29 Jun 2023 20:02:20 +0800

> In [1] & [2] & [3], there are usecases for veth and virtio_net
> to use frag support in page pool to reduce memory usage, and it
> may request different frag size depending on the head/tail
> room space for xdp_frame/shinfo and mtu/packet size. When the
> requested frag size is large enough that a single page can not
> be split into more than one frag, using frag support only have
> performance penalty because of the extra frag count handling
> for frag support.
> 
> So this patchset provides a page pool API for the driver to
> allocate memory with least memory utilization and performance
> penalty when it doesn't know the size of memory it need
> beforehand.
> 
> 1. https://patchwork.kernel.org/project/netdevbpf/patch/d3ae6bd3537fbce379382ac6a42f67e22f27ece2.1683896626.git.lorenzo@kernel.org/
> 2. https://patchwork.kernel.org/project/netdevbpf/patch/20230526054621.18371-3-liangchen.linux@gmail.com/
> 3. https://github.com/alobakin/linux/tree/iavf-pp-frag

Thanks for sharing the link :D

> 
> v5 RFC: add a new page_pool_cache_alloc() API, and other minor
>         change as discussed in v4. As there seems to be three
>         comsumers that might be made use of the new API, so
>         repost it as RFC and CC the relevant authors to see
>         if the new API fits their need.

Tested v5 against my latest tree, no regressions, perf is even a bit
better than it was. That also might've come from that net-next pulled
Linus' tree with a good bunch of PRs already merged, or from v4 -> v5
update.

Re consumers, I'm planning to send the RFC series with IAVF as a
consumer on Monday (and a couple generic Page Pool improvements today,
will see).

> 
> V4. Fix a typo and add a patch to update document about frag
>     API, PAGE_POOL_DMA_USE_PP_FRAG_COUNT is not renamed yet
>     as we may need a different thread to discuss that.
> 
> V3: Incorporate changes from the disscusion with Alexander,
>     mostly the inline wraper, PAGE_POOL_DMA_USE_PP_FRAG_COUNT
>     change split to separate patch and comment change.
> V2: Add patch to remove PP_FLAG_PAGE_FRAG flags and mention
>     virtio_net usecase in the cover letter.
> V1: Drop RFC tag and page_pool_frag patch.
Thanks,
Olek

Re: [PATCH v5 RFC 5/6] page_pool: update document about frag API

[Randy Dunlap]

Hi--

On 6/29/23 05:02, Yunsheng Lin wrote:
> As more drivers begin to use the frag API, update the
> document about how to decide which API to use for the
> driver author.
> 
> Also it seems there is a similar document in page_pool.h,
> so remove it to avoid the duplication.
> 
> 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>
> ---
>  Documentation/networking/page_pool.rst | 34 ++++++++++++++++++++++----
>  include/net/page_pool.h                | 22 -----------------
>  2 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/networking/page_pool.rst b/Documentation/networking/page_pool.rst
> index 873efd97f822..18b13d659c98 100644
> --- a/Documentation/networking/page_pool.rst
> +++ b/Documentation/networking/page_pool.rst
> @@ -4,12 +4,27 @@
>  Page Pool API
>  =============
>  
> -The page_pool allocator is optimized for the XDP mode that uses one frame
> -per-page, but it can fallback on the regular page allocator APIs.
> +The page_pool allocator is optimized for recycling page or page frag used by skb
> +packet and xdp frame.

That sentence could use some adjectives. Choose singular or plural:

> +The page_pool allocator is optimized for recycling a page or page frag used by an skb
> +packet or xdp frame.

or

> +The page_pool allocator is optimized for recycling pages or page frags used by skb
> +packets or xdp frames.

Now that I have written them, I prefer the latter one (plural). FWIW.

>  
> -Basic use involves replacing alloc_pages() calls with the
> -page_pool_alloc_pages() call.  Drivers should use page_pool_dev_alloc_pages()
> -replacing dev_alloc_pages().
> +Basic use involves replacing napi_alloc_frag() and alloc_pages() calls with
> +page_pool_cache_alloc() and page_pool_alloc(), which allocate memory with or
> +without page splitting depending on the requested memory size.
> +
> +If the driver knows that it always requires full pages or its allocates are

                                                                 allocations are

> +always smaller than half a page, it can use one of the more specific API calls:
> +
> +1. page_pool_alloc_pages(): allocate memory without page splitting when driver
> +   knows that the memory it need is always bigger than half of the page
> +   allocated from page pool. There is no cache line dirtying for 'struct page'
> +   when a page is recycled back to the page pool.
> +
> +2. page_pool_alloc_frag(): allocate memory with page splitting when driver knows
> +   that the memory it need is always smaller than or equal to half of the page
> +   allocated from page pool. Page splitting enables memory saving and thus avoid

                                                                     and thus avoids

> +   TLB/cache miss for data access, but there also is some cost to implement page
> +   splitting, mainly some cache line dirtying/bouncing for 'struct page' and
> +   atomic operation for page->pp_frag_count.
>  
>  API keeps track of in-flight pages, in order to let API user know
>  when it is safe to free a page_pool object.  Thus, API users
> @@ -93,6 +108,15 @@ a page will cause no race conditions is enough.
>  * page_pool_dev_alloc_pages(): Get a page from the page allocator or page_pool
>    caches.
>  
> +* page_pool_dev_alloc_frag(): Get a page frag from the page allocator or
> +  page_pool caches.
> +
> +* page_pool_dev_alloc(): Get a page or page frag from the page allocator or
> +  page_pool caches.
> +
> +* page_pool_dev_cache_alloc(): Get a cache from the page allocator or page_pool
> +  caches.
> +
>  * page_pool_get_dma_addr(): Retrieve the stored DMA address.
>  
>  * page_pool_get_dma_dir(): Retrieve the stored DMA direction.

Thanks for adding the documentation.

-- 
~Randy

Re: [PATCH v5 RFC 0/6] introduce page_pool_alloc() API

[Yunsheng Lin]

On 2023/6/29 22:26, Alexander Lobakin wrote:
>> v5 RFC: add a new page_pool_cache_alloc() API, and other minor
>>         change as discussed in v4. As there seems to be three
>>         comsumers that might be made use of the new API, so
>>         repost it as RFC and CC the relevant authors to see
>>         if the new API fits their need.
> 
> Tested v5 against my latest tree, no regressions, perf is even a bit
> better than it was. That also might've come from that net-next pulled
> Linus' tree with a good bunch of PRs already merged, or from v4 -> v5
> update.

v4 -> v5 is mostly about adding the page pool cache API, so I believe the
perf improvement is from the net-next pull:)

> 
> Re consumers, I'm planning to send the RFC series with IAVF as a
> consumer on Monday (and a couple generic Page Pool improvements today,
> will see).

Thanks.

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

[Jakub Kicinski]

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.

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

[Jakub Kicinski]

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..

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

[Yunsheng Lin]

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? 

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

[Yunsheng Lin]

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?

> 

Re: [PATCH v5 RFC 2/6] page_pool: unify frag_count handling in page_pool_is_last_frag()

[Alexander Lobakin]

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Thu, 29 Jun 2023 20:02:22 +0800

> Currently when page_pool_create() is called with
> PP_FLAG_PAGE_FRAG flag, page_pool_alloc_pages() is only
> allowed to be called under the below constraints:
> 1. page_pool_fragment_page() need to be called to setup
>    page->pp_frag_count immediately.
> 2. page_pool_defrag_page() often need to be called to drain
>    the page->pp_frag_count when there is no more user will
>    be holding on to that page.

[...]

> @@ -352,12 +377,10 @@ static inline bool page_pool_is_last_frag(struct page_pool *pool,
>  {
>  	/* 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.
> +	 * 1. We are running in 32-bit arch with 64-bit DMA.
> +	 * 2. 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 ||
> +	return PAGE_POOL_DMA_USE_PP_FRAG_COUNT ||
>  	       (page_pool_defrag_page(page, 1) == 0);

Just noticed while developing: after this change, the first function
argument, i.e. @pool, is not needed anymore and can be removed.

[...]

Thanks,
Olek

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

[Jakub Kicinski]

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.

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

[Jakub Kicinski]

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?

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

[Alexander Lobakin]

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

Re: [PATCH v5 RFC 2/6] page_pool: unify frag_count handling in page_pool_is_last_frag()

[Yunsheng Lin]

On 2023/7/10 22:39, Alexander Lobakin wrote:
> From: Yunsheng Lin <linyunsheng@huawei.com>
> Date: Thu, 29 Jun 2023 20:02:22 +0800
> 
>> Currently when page_pool_create() is called with
>> PP_FLAG_PAGE_FRAG flag, page_pool_alloc_pages() is only
>> allowed to be called under the below constraints:
>> 1. page_pool_fragment_page() need to be called to setup
>>    page->pp_frag_count immediately.
>> 2. page_pool_defrag_page() often need to be called to drain
>>    the page->pp_frag_count when there is no more user will
>>    be holding on to that page.
> 
> [...]
> 
>> @@ -352,12 +377,10 @@ static inline bool page_pool_is_last_frag(struct page_pool *pool,
>>  {
>>  	/* 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.
>> +	 * 1. We are running in 32-bit arch with 64-bit DMA.
>> +	 * 2. 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 ||
>> +	return PAGE_POOL_DMA_USE_PP_FRAG_COUNT ||
>>  	       (page_pool_defrag_page(page, 1) == 0);
> 
> Just noticed while developing: after this change, the first function
> argument, i.e. @pool, is not needed anymore and can be removed.

Yes, thanks.

> 

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

[Jakub Kicinski]

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.

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

[Alexander Lobakin]

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

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

[Jakub Kicinski]

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?

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

[Yunsheng Lin]

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
> 
> .
> 

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

[Jakub Kicinski]

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?

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

[Yunsheng Lin]

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.

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

[Jesper Dangaard Brouer]

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

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

[Jakub Kicinski]

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?

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

[Yunsheng Lin]

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.

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

[Jakub Kicinski]

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!

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

[Alexander Lobakin]

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

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

[Yunsheng Lin]

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
> .
>