Re: [PATCH net-next v2 2/3] page_pool: support non-frag page for page_pool_alloc_frag()

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2023/5/30 23:07, Alexander H Duyck wrote:
...

>> +	if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
>> +		*offset = 0;
>> +		return page_pool_alloc_pages(pool, gfp);
>> +	}
>> +
> 
> This is a recipe for pain. Rather than doing this I would say we should
> stick with our existing behavior and not allow page pool fragments to
> be used when the DMA address is consuming the region. Otherwise we are
> going to make things very confusing.

Are there any other concern other than confusing? we could add a
big comment to make it clear.

The point of adding that is to avoid the driver handling the
PAGE_POOL_DMA_USE_PP_FRAG_COUNT when using page_pool_alloc_frag()
like something like below:

if (!PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
	page = page_pool_alloc_frag()
else
	page = XXXXX;

Or do you perfer the driver handling it? why?

> 
> If we have to have both version I would much rather just have some
> inline calls in the header wrapped in one #ifdef for
> PAGE_POOL_DMA_USE_PP_FRAG_COUNT that basically are a wrapper for
> page_pool pages treated as pp_frag.

Do you have a good name in mind for that wrapper.
In addition to the naming, which API should I use when I am a driver
author wanting to add page pool support?

> 
>>  	size = ALIGN(size, dma_get_cache_alignment());
>> -	*offset = pool->frag_offset;
>>  
> 
> If we are going to be allocating mono-frag pages they should be
> allocated here based on the size check. That way we aren't discrupting
> the performance for the smaller fragments and the code below could
> function undisturbed.

It is to allow possible optimization as below.

> 
>> -	if (page && *offset + size > max_size) {
>> +	if (page) {
>> +		*offset = pool->frag_offset;
>> +
>> +		if (*offset + size <= max_size) {
>> +			pool->frag_users++;
>> +			pool->frag_offset = *offset + size;
>> +			alloc_stat_inc(pool, fast);
>> +			return page;

Note that we still allow frag page here when '(size << 1 > max_size)'.

>> +		}
>> +
>> +		pool->frag_page = NULL;
>>  		page = page_pool_drain_frag(pool, page);
>>  		if (page) {
>>  			alloc_stat_inc(pool, fast);
>> @@ -714,26 +727,24 @@ struct page *page_pool_alloc_frag(struct page_pool *pool,
>>  		}
>>  	}
>>  
>> -	if (!page) {
>> -		page = page_pool_alloc_pages(pool, gfp);
>> -		if (unlikely(!page)) {
>> -			pool->frag_page = NULL;
>> -			return NULL;
>> -		}
>> -
>> -		pool->frag_page = page;
>> +	page = page_pool_alloc_pages(pool, gfp);
>> +	if (unlikely(!page))
>> +		return NULL;
>>  
>>  frag_reset:
>> -		pool->frag_users = 1;
>> +	/* return page as non-frag page if a page is not able to
>> +	 * hold two frags for the current requested size.
>> +	 */
> 
> This statement ins't exactly true since you make all page pool pages
> into fragmented pages.

Any suggestion to describe it more accurately?
I wrote that thinking frag_count being one as non-frag page.

> 
> 
>> +	if (unlikely(size << 1 > max_size)) {
> 
> This should happen much sooner so you aren't mixing these allocations
> with the smaller ones and forcing the fragmented page to be evicted.

As mentioned above, it is to allow a possible optimization