Re: [PATCH rfc v6 2/4] page_pool: add interface to manipulate frag count in page pool

[Yunsheng Lin]

On 2021/7/20 23:43, Alexander Duyck wrote:
> On Mon, Jul 19, 2021 at 8:36 PM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>>
>> For 32 bit systems with 64 bit dma, dma_addr[1] is used to
>> store the upper 32 bit dma addr, those system should be rare
>> those days.
>>
>> For normal system, the dma_addr[1] in 'struct page' is not
>> used, so we can reuse dma_addr[1] for storing frag count,
>> which means how many frags this page might be splited to.
>>
>> In order to simplify the page frag support in the page pool,
>> the PAGE_POOL_DMA_USE_PP_FRAG_COUNT macro is added to indicate
>> the 32 bit systems with 64 bit dma, and the page frag support
>> in page pool is disabled for such system.
>>
>> The newly added page_pool_set_frag_count() is called to reserve
>> the maximum frag count before any page frag is passed to the
>> user. The page_pool_atomic_sub_frag_count_return() is called
>> when user is done with the page frag.
>>
>> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
>> ---
>>  include/linux/mm_types.h | 18 +++++++++++++-----
>>  include/net/page_pool.h  | 41 ++++++++++++++++++++++++++++++++++-------
>>  net/core/page_pool.c     |  4 ++++
>>  3 files changed, 51 insertions(+), 12 deletions(-)
>>
> 
> <snip>
> 
>> +static inline long page_pool_atomic_sub_frag_count_return(struct page *page,
>> +                                                         long nr)
>> +{
>> +       long frag_count = atomic_long_read(&page->pp_frag_count);
>> +       long ret;
>> +
>> +       if (frag_count == nr)
>> +               return 0;
>> +
>> +       ret = atomic_long_sub_return(nr, &page->pp_frag_count);
>> +       WARN_ON(ret < 0);
>> +       return ret;
>>  }
>>
> 
> So this should just be an atomic_long_sub_return call. You should get
> rid of the atomic_long_read portion of this as it can cover up
> reference count errors.

atomic_long_sub_return() is used to avoid one possible cache bouncing and
barrrier caused by the last user.

You are right that that may cover up the reference count errors. How about
something like below:

static inline long page_pool_atomic_sub_frag_count_return(struct page *page,
							  long nr)
{
#ifdef CONFIG_DEBUG_PAGE_REF
	long ret = atomic_long_sub_return(nr, &page->pp_frag_count);

	WARN_ON(ret < 0);

	return ret;
#else
	if (atomic_long_read(&page->pp_frag_count) == nr)
		return 0;

	return atomic_long_sub_return(nr, &page->pp_frag_count);
#end
}

Or any better suggestion?


> .
>