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

Yunsheng Lin
2023-05-31T12:19:17Z

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