Re: [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/2/17 19:00, Lorenzo Bianconi wrote:
>>>  #ifdef CONFIG_PAGE_POOL_STATS
>>> -	pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
>>> -	if (!pool->recycle_stats)
>>> -		return -ENOMEM;
>>> +	if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL)) {
>>> +		pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
>>> +		if (!pool->recycle_stats)
>>> +			return -ENOMEM;
>>> +	} else {
>>> +		/* For system page pool instance we use a singular stats object
>>> +		 * instead of allocating a separate percpu variable for each
>>> +		 * (also percpu) page pool instance.
>>> +		 */
>>> +		pool->recycle_stats = &pp_system_recycle_stats;
>>
>> Do we need to return -EINVAL here if page_pool_init() is called with
>> pool->p.flags & PP_FLAG_SYSTEM_POOL being true and cpuid being a valid
>> cpu?

My fault, the above "cpuid being a valid cpu" should be "cpuid not being a
valid cpu".
In other word, do we need to protect user from calling page_pool_init()
with PP_FLAG_SYSTEM_POOL flag and cpuid being -1?


>> If yes, it seems we may be able to use the cpuid to decide if we need
>> to allocate a new pool->recycle_stats without adding a new flag.
> 
> for the current use-cases cpuid is set to a valid core id just for system
> page_pool but in the future probably there will not be a 1:1 relation (e.g.
> we would want to pin a per-cpu page_pool instance to a particular CPU?)

if it is a per-cpu page_pool instance, doesn't it run into the similar
problem this patch is trying to fix?

> 
>>
>> If no, the API for page_pool_create_percpu() seems a litte weird as it
>> relies on the user calling it correctly.
>>
>> Also, do we need to enforce that page_pool_create_percpu() is only called
>> once for the same cpu? if no, we may have two page_pool instance sharing
>> the same stats.
> 
> do you mean for the same pp instance? I guess it is not allowed by the APIs.

As above comment, if the user is passing a valid cpuid, the PP_FLAG_SYSTEM_POOL
flag need to be set too? if yes, doesn't the new flag seems somewhat redundant
here?