Re: [PATCH net-next v6 09/10] page_pool: allow mixing PPs within one bulk

[Alexander Lobakin]

From: Jakub Kicinski <kuba@xxxxxxxxxx>
Date: Thu, 5 Dec 2024 18:40:16 -0800

> Very nice in general, I'll apply the previous 8 but I'd like to offer
> some alternatives here..

Great suggestions, I addressed most of them already and the function
looks much better now.
One note below.

> 
> On Tue,  3 Dec 2024 18:37:32 +0100 Alexander Lobakin wrote:
>> +void page_pool_put_netmem_bulk(netmem_ref *data, u32 count)
>>  {
>> -	int i, bulk_len = 0;
>> -	bool allow_direct;
>> -	bool in_softirq;
>> +	bool allow_direct, in_softirq, again = false;
>> +	netmem_ref bulk[XDP_BULK_QUEUE_SIZE];
>> +	u32 i, bulk_len, foreign;
>> +	struct page_pool *pool;
>>  
>> -	allow_direct = page_pool_napi_local(pool);
>> +again:
>> +	pool = NULL;
>> +	bulk_len = 0;
>> +	foreign = 0;
>>  
>>  	for (i = 0; i < count; i++) {
>> -		netmem_ref netmem = netmem_compound_head(data[i]);
>> +		struct page_pool *netmem_pp;
>> +		netmem_ref netmem;
>> +
>> +		if (!again) {
>> +			netmem = netmem_compound_head(data[i]);
>>  
>> -		/* It is not the last user for the page frag case */
>> -		if (!page_pool_is_last_ref(netmem))
>> +			/* It is not the last user for the page frag case */
>> +			if (!page_pool_is_last_ref(netmem))
>> +				continue;
> 
> We check the "again" condition potentially n^2 times, is it written
> this way because we expect no mixing? Would it not be fewer cycles
> to do a first pass, convert all buffers to heads, filter out all
> non-last refs, and delete the "again" check?
> 
> Minor benefit is that it removes a few of the long lines so it'd be
> feasible to drop the "goto again" as well and just turn this function
> into a while (count) loop.
> 
>> +		} else {
>> +			netmem = data[i];
>> +		}
>> +
>> +		netmem_pp = netmem_get_pp(netmem);
> 
> nit: netmem_pp is not a great name. Ain't nothing especially netmem
> about it, it's just the _current_ page pool.

It's the page_pool of the @netmem we're processing on this iteration.
"This netmem's PP" => netmem_pp.
Current page_pool which we'll use for recycling is @pool.

> 
>> +		if (unlikely(!pool)) {
>> +			pool = netmem_pp;
>> +			allow_direct = page_pool_napi_local(pool);
>> +		} else if (netmem_pp != pool) {
>> +			/*
>> +			 * If the netmem belongs to a different page_pool, save
>> +			 * it for another round after the main loop.
>> +			 */
>> +			data[foreign++] = netmem;
>>  			continue;
>> +		}
>>  
>>  		netmem = __page_pool_put_page(pool, netmem, -1, allow_direct);
>>  		/* Approved for bulk recycling in ptr_ring cache */
>>  		if (netmem)
>> -			data[bulk_len++] = netmem;
>> +			bulk[bulk_len++] = netmem;
>>  	}

Thanks,
Olek