diff mbox series

[net-next,v6,8/8] page_pool: use list instead of array for alloc cache

Message ID 20250106130116.457938-9-linyunsheng@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series fix two bugs related to page_pool | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 42 this patch: 42
netdev/build_tools success Errors and warnings before: 0 (+23) this patch: 0 (+23)
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 7129 this patch: 7129
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4949 this patch: 4949
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 160 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2025-01-06--21-00 (tests: 886)

Commit Message

Yunsheng Lin Jan. 6, 2025, 1:01 p.m. UTC 
As the alloc cache is always protected by NAPI context
protection, use encoded_next as a pointer to a next item
to avoid the using the array.

Testing shows there is about 3ns improvement for the
performance of 'time_bench_page_pool01_fast_path' test
case.

CC: Robin Murphy <robin.murphy@arm.com>
CC: Alexander Duyck <alexander.duyck@gmail.com>
CC: IOMMU <iommu@lists.linux.dev>
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 include/net/page_pool/types.h |  2 +-
 net/core/page_pool.c          | 59 +++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 18 deletions(-)

Comments

Simon Horman Jan. 7, 2025, 12:03 p.m. UTC | #1 
On Mon, Jan 06, 2025 at 09:01:16PM +0800, Yunsheng Lin wrote:
> As the alloc cache is always protected by NAPI context
> protection, use encoded_next as a pointer to a next item
> to avoid the using the array.
> 
> Testing shows there is about 3ns improvement for the
> performance of 'time_bench_page_pool01_fast_path' test
> case.
> 
> CC: Robin Murphy <robin.murphy@arm.com>
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> CC: IOMMU <iommu@lists.linux.dev>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>

...

> diff --git a/net/core/page_pool.c b/net/core/page_pool.c

...

> @@ -677,10 +698,12 @@ static void __page_pool_return_page(struct page_pool *pool, netmem_ref netmem,
>  
>  static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
>  {
> -	struct page_pool_item *refill;
> +	struct page_pool_item *refill, *alloc, *curr;
>  	netmem_ref netmem;
>  	int pref_nid; /* preferred NUMA node */
>  
> +	DEBUG_NET_WARN_ON_ONCE(pool->alloc.count || pool->alloc.list);
> +
>  	/* Quicker fallback, avoid locks when ring is empty */
>  	refill = pool->alloc.refill;
>  	if (unlikely(!refill && !READ_ONCE(pool->ring.list))) {
> @@ -698,6 +721,7 @@ static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
>  	pref_nid = numa_mem_id(); /* will be zero like page_to_nid() */
>  #endif
>  
> +	alloc = NULL;
>  	/* Refill alloc array, but only if NUMA match */
>  	do {
>  		if (unlikely(!refill)) {
> @@ -706,10 +730,13 @@ static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
>  				break;
>  		}
>  
> +		curr = refill;
>  		netmem = refill->pp_netmem;
>  		refill = page_pool_item_get_next(refill);
>  		if (likely(netmem_is_pref_nid(netmem, pref_nid))) {
> -			pool->alloc.cache[pool->alloc.count++] = netmem;
> +			page_pool_item_set_next(curr, alloc);
> +			pool->alloc.count++;
> +			alloc = curr;
>  		} else {
>  			/* NUMA mismatch;
>  			 * (1) release 1 page to page-allocator and
> @@ -729,7 +756,8 @@ static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
>  	/* Return last page */
>  	if (likely(pool->alloc.count > 0)) {
>  		atomic_sub(pool->alloc.count, &pool->ring.count);
> -		netmem = pool->alloc.cache[--pool->alloc.count];
> +		pool->alloc.list = page_pool_item_get_next(alloc);
> +		pool->alloc.count--;
>  		alloc_stat_inc(pool, refill);
>  	}
>  

Hi Yunsheng Lin,

The following line of the code looks like this:

	return netmem;

And, with this patch applied, Smatch warns that netmem may be used
uninitialised here. I assume this is because it is no longer conditionally
initialised above.

...
Yunsheng Lin Jan. 7, 2025, 12:55 p.m. UTC | #2 
On 2025/1/7 20:03, Simon Horman wrote:
> On Mon, Jan 06, 2025 at 09:01:16PM +0800, Yunsheng Lin wrote:
>> As the alloc cache is always protected by NAPI context
>> protection, use encoded_next as a pointer to a next item
>> to avoid the using the array.
>>
>> Testing shows there is about 3ns improvement for the
>> performance of 'time_bench_page_pool01_fast_path' test
>> case.
>>
>> CC: Robin Murphy <robin.murphy@arm.com>
>> CC: Alexander Duyck <alexander.duyck@gmail.com>
>> CC: IOMMU <iommu@lists.linux.dev>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> 
> ...
> 
>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> 
> ...
> 
>> @@ -677,10 +698,12 @@ static void __page_pool_return_page(struct page_pool *pool, netmem_ref netmem,
>>  
>>  static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
>>  {
>> -	struct page_pool_item *refill;
>> +	struct page_pool_item *refill, *alloc, *curr;
>>  	netmem_ref netmem;
>>  	int pref_nid; /* preferred NUMA node */
>>  
>> +	DEBUG_NET_WARN_ON_ONCE(pool->alloc.count || pool->alloc.list);
>> +
>>  	/* Quicker fallback, avoid locks when ring is empty */
>>  	refill = pool->alloc.refill;
>>  	if (unlikely(!refill && !READ_ONCE(pool->ring.list))) {

The checking has ensured that netmem will be initialised below.

>> @@ -698,6 +721,7 @@ static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
>>  	pref_nid = numa_mem_id(); /* will be zero like page_to_nid() */
>>  #endif
>>  
>> +	alloc = NULL;
>>  	/* Refill alloc array, but only if NUMA match */
>>  	do {
>>  		if (unlikely(!refill)) {
>> @@ -706,10 +730,13 @@ static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
>>  				break;
>>  		}
>>  
>> +		curr = refill;
>>  		netmem = refill->pp_netmem;

initialised here.

>>  		refill = page_pool_item_get_next(refill);
>>  		if (likely(netmem_is_pref_nid(netmem, pref_nid))) {
>> -			pool->alloc.cache[pool->alloc.count++] = netmem;
>> +			page_pool_item_set_next(curr, alloc);
>> +			pool->alloc.count++;
>> +			alloc = curr;
>>  		} else {
>>  			/* NUMA mismatch;
>>  			 * (1) release 1 page to page-allocator and

And set to zero here.

>> @@ -729,7 +756,8 @@ static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
>>  	/* Return last page */
>>  	if (likely(pool->alloc.count > 0)) {
>>  		atomic_sub(pool->alloc.count, &pool->ring.count);
>> -		netmem = pool->alloc.cache[--pool->alloc.count];

So we still need to set netmem to something meaningful here if netmem
is set to zero in the above 'else' branch.

>> +		pool->alloc.list = page_pool_item_get_next(alloc);
>> +		pool->alloc.count--;
>>  		alloc_stat_inc(pool, refill);
>>  	}
>>  
> 
> Hi Yunsheng Lin,
> 
> The following line of the code looks like this:
> 
> 	return netmem;
> 
> And, with this patch applied, Smatch warns that netmem may be used
> uninitialised here. I assume this is because it is no longer conditionally
> initialised above.

Thanks for reminding, the tool does seem to catch some bug here.

> 
> ...
>
diff mbox series

Patch

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index ac83abea24b0..97f548c79e22 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -50,7 +50,7 @@ 
 #define PP_ALLOC_CACHE_REFILL	64
 struct pp_alloc_cache {
 	u32 count;
-	netmem_ref cache[PP_ALLOC_CACHE_SIZE];
+	struct page_pool_item *list;
 
 	/* Keep batched refilled pages here to avoid doing the atomic operation
 	 * for each page.
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 429d44ede074..59aea3abf0eb 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -386,6 +386,27 @@  static netmem_ref page_pool_consume_ring(struct page_pool *pool)
 	return list->pp_netmem;
 }
 
+static netmem_ref __page_pool_consume_alloc(struct page_pool *pool)
+{
+	struct page_pool_item *item = pool->alloc.list;
+
+	pool->alloc.list = page_pool_item_get_next(item);
+	pool->alloc.count--;
+
+	return item->pp_netmem;
+}
+
+static void __page_pool_recycle_in_alloc(struct page_pool *pool,
+					 netmem_ref netmem)
+{
+	struct page_pool_item *item;
+
+	item = netmem_get_pp_item(netmem);
+	page_pool_item_set_next(item, pool->alloc.list);
+	pool->alloc.list = item;
+	pool->alloc.count++;
+}
+
 static __always_inline void __page_pool_release_page_dma(struct page_pool *pool,
 							 netmem_ref netmem,
 							 bool destroyed)
@@ -677,10 +698,12 @@  static void __page_pool_return_page(struct page_pool *pool, netmem_ref netmem,
 
 static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
 {
-	struct page_pool_item *refill;
+	struct page_pool_item *refill, *alloc, *curr;
 	netmem_ref netmem;
 	int pref_nid; /* preferred NUMA node */
 
+	DEBUG_NET_WARN_ON_ONCE(pool->alloc.count || pool->alloc.list);
+
 	/* Quicker fallback, avoid locks when ring is empty */
 	refill = pool->alloc.refill;
 	if (unlikely(!refill && !READ_ONCE(pool->ring.list))) {
@@ -698,6 +721,7 @@  static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
 	pref_nid = numa_mem_id(); /* will be zero like page_to_nid() */
 #endif
 
+	alloc = NULL;
 	/* Refill alloc array, but only if NUMA match */
 	do {
 		if (unlikely(!refill)) {
@@ -706,10 +730,13 @@  static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
 				break;
 		}
 
+		curr = refill;
 		netmem = refill->pp_netmem;
 		refill = page_pool_item_get_next(refill);
 		if (likely(netmem_is_pref_nid(netmem, pref_nid))) {
-			pool->alloc.cache[pool->alloc.count++] = netmem;
+			page_pool_item_set_next(curr, alloc);
+			pool->alloc.count++;
+			alloc = curr;
 		} else {
 			/* NUMA mismatch;
 			 * (1) release 1 page to page-allocator and
@@ -729,7 +756,8 @@  static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool)
 	/* Return last page */
 	if (likely(pool->alloc.count > 0)) {
 		atomic_sub(pool->alloc.count, &pool->ring.count);
-		netmem = pool->alloc.cache[--pool->alloc.count];
+		pool->alloc.list = page_pool_item_get_next(alloc);
+		pool->alloc.count--;
 		alloc_stat_inc(pool, refill);
 	}
 
@@ -744,7 +772,7 @@  static netmem_ref __page_pool_get_cached(struct page_pool *pool)
 	/* Caller MUST guarantee safe non-concurrent access, e.g. softirq */
 	if (likely(pool->alloc.count)) {
 		/* Fast-path */
-		netmem = pool->alloc.cache[--pool->alloc.count];
+		netmem = __page_pool_consume_alloc(pool);
 		alloc_stat_inc(pool, fast);
 	} else {
 		netmem = page_pool_refill_alloc_cache(pool);
@@ -863,6 +891,7 @@  static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
 static noinline netmem_ref __page_pool_alloc_pages_slow(struct page_pool *pool,
 							gfp_t gfp)
 {
+	netmem_ref netmems[PP_ALLOC_CACHE_REFILL] = {0};
 	const int bulk = PP_ALLOC_CACHE_REFILL;
 	unsigned int pp_order = pool->p.order;
 	bool dma_map = pool->dma_map;
@@ -873,16 +902,12 @@  static noinline netmem_ref __page_pool_alloc_pages_slow(struct page_pool *pool,
 	if (unlikely(pp_order))
 		return page_to_netmem(__page_pool_alloc_page_order(pool, gfp));
 
-	/* Unnecessary as alloc cache is empty, but guarantees zero count */
-	if (unlikely(pool->alloc.count > 0))
-		return pool->alloc.cache[--pool->alloc.count];
-
-	/* Mark empty alloc.cache slots "empty" for alloc_pages_bulk_array */
-	memset(&pool->alloc.cache, 0, sizeof(void *) * bulk);
+	/* alloc cache should be empty */
+	DEBUG_NET_WARN_ON_ONCE(pool->alloc.count || pool->alloc.list);
 
 	nr_pages = alloc_pages_bulk_array_node(gfp,
 					       pool->p.nid, bulk,
-					       (struct page **)pool->alloc.cache);
+					       (struct page **)netmems);
 	if (unlikely(!nr_pages))
 		return 0;
 
@@ -890,7 +915,7 @@  static noinline netmem_ref __page_pool_alloc_pages_slow(struct page_pool *pool,
 	 * page element have not been (possibly) DMA mapped.
 	 */
 	for (i = 0; i < nr_pages; i++) {
-		netmem = pool->alloc.cache[i];
+		netmem = netmems[i];
 
 		if (unlikely(!page_pool_set_pp_info(pool, netmem))) {
 			put_page(netmem_to_page(netmem));
@@ -903,7 +928,7 @@  static noinline netmem_ref __page_pool_alloc_pages_slow(struct page_pool *pool,
 			continue;
 		}
 
-		pool->alloc.cache[pool->alloc.count++] = netmem;
+		__page_pool_recycle_in_alloc(pool, netmem);
 		/* Track how many pages are held 'in-flight' */
 		pool->pages_state_hold_cnt++;
 		trace_page_pool_state_hold(pool, netmem,
@@ -912,7 +937,7 @@  static noinline netmem_ref __page_pool_alloc_pages_slow(struct page_pool *pool,
 
 	/* Return last page */
 	if (likely(pool->alloc.count > 0)) {
-		netmem = pool->alloc.cache[--pool->alloc.count];
+		netmem = __page_pool_consume_alloc(pool);
 		alloc_stat_inc(pool, slow);
 	} else {
 		netmem = 0;
@@ -1082,7 +1107,7 @@  static bool page_pool_recycle_in_cache(netmem_ref netmem,
 	}
 
 	/* Caller MUST have verified/know (page_ref_count(page) == 1) */
-	pool->alloc.cache[pool->alloc.count++] = netmem;
+	__page_pool_recycle_in_alloc(pool, netmem);
 	recycle_stat_inc(pool, cached);
 	return true;
 }
@@ -1427,7 +1452,7 @@  static void page_pool_empty_alloc_cache_once(struct page_pool *pool)
 	 * call concurrently.
 	 */
 	while (pool->alloc.count) {
-		netmem = pool->alloc.cache[--pool->alloc.count];
+		netmem = __page_pool_consume_alloc(pool);
 		page_pool_return_page(pool, netmem);
 	}
 
@@ -1567,7 +1592,7 @@  void page_pool_update_nid(struct page_pool *pool, int new_nid)
 
 	/* Flush pool alloc cache, as refill will check NUMA node */
 	while (pool->alloc.count) {
-		netmem = pool->alloc.cache[--pool->alloc.count];
+		netmem = __page_pool_consume_alloc(pool);
 		__page_pool_return_page(pool, netmem, false);
 	}
 }