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/31 20:19, Yunsheng Lin wrote:
> On 2023/5/30 23:07, Alexander H Duyck wrote:

Hi, Alexander
    Any more comment or concern?
    I feel like we are circling back to v1 about whether it is better
add a new wrapper/API or not and where to do the "(size << 1 > max_size)"
checking. I really like to continue the discussion here instead of in the
new thread again when I post a v3, thanks.

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