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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 18 Feb 2024 11:49:50 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
CC: Lorenzo Bianconi <lorenzo@...nel.org>, <netdev@...r.kernel.org>,
	<kuba@...nel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<pabeni@...hat.com>, <hawk@...nel.org>, <ilias.apalodimas@...aro.org>,
	<toke@...hat.com>, <aleksander.lobakin@...el.com>
Subject: Re: [PATCH net-next] net: page_pool: fix recycle stats for system
 page_pool allocator

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?