[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b9fba731-fe69-5eb6-8f9e-a477fe5cc124@huawei.com>
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?
Powered by blists - more mailing lists