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

From: Yunsheng Lin
Date: Tue Jan 07 2025 - 07:55:54 EST


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@xxxxxxx>
>> CC: Alexander Duyck <alexander.duyck@xxxxxxxxx>
>> CC: IOMMU <iommu@xxxxxxxxxxxxxxx>
>> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
>
> ...
>
>> 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.

>
> ...
>