Re: [PATCH v1 -next 10/11] cgroup/cpuset: guard cpuset-v1 code under CONFIG_CPUSETS_V1

[Chen Ridong <chenridong@huaweicloud.com>]

On 2024/8/26 10:18, Waiman Long wrote:
> On 8/25/24 21:34, Chen Ridong wrote:
>>
>>
>> On 2024/8/26 1:25, Waiman Long wrote:
>>> On 8/23/24 06:01, Chen Ridong wrote:
>>>> This patch introduces CONFIG_CPUSETS_V1 and guard cpuset-v1 code under
>>>> CONFIG_CPUSETS_V1. The default value of CONFIG_CPUSETS_V1 is N, so that
>>>> user who adopted v2 don't have 'pay' for cpuset v1.
>>>> Besides, to make code succinct, rename 
>>>> '__cpuset_memory_pressure_bump()'
>>>> to 'cpuset_memory_pressure_bump()', and expose it to the world, which
>>>> takes place of the old mocro 'cpuset_memory_pressure_bump'.
>>>>
>>>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
>>>> ---
>>>>   include/linux/cpuset.h          |  8 +-------
>>>>   init/Kconfig                    | 13 +++++++++++++
>>>>   kernel/cgroup/Makefile          |  3 ++-
>>>>   kernel/cgroup/cpuset-internal.h | 15 +++++++++++++++
>>>>   kernel/cgroup/cpuset-v1.c       | 10 ++++++----
>>>>   kernel/cgroup/cpuset.c          |  2 ++
>>>>   6 files changed, 39 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
>>>> index 2a6981eeebf8..f91f1f61f482 100644
>>>> --- a/include/linux/cpuset.h
>>>> +++ b/include/linux/cpuset.h
>>>> @@ -99,13 +99,7 @@ static inline bool cpuset_zone_allowed(struct 
>>>> zone *z, gfp_t gfp_mask)
>>>>   extern int cpuset_mems_allowed_intersects(const struct task_struct 
>>>> *tsk1,
>>>>                         const struct task_struct *tsk2);
>>>> -#define cpuset_memory_pressure_bump()                 \
>>>> -    do {                            \
>>>> -        if (cpuset_memory_pressure_enabled)        \
>>>> -            __cpuset_memory_pressure_bump();    \
>>>> -    } while (0)
>>>> -extern int cpuset_memory_pressure_enabled;
>>>> -extern void __cpuset_memory_pressure_bump(void);
>>>> +extern void cpuset_memory_pressure_bump(void);
>>>>   extern void cpuset_task_status_allowed(struct seq_file *m,
>>>>                       struct task_struct *task);
>>>
>>> As you are introducing a new CONFIG_CPUSET_V1 kconfig option, you can 
>>> use it to eliminate a useless function call if cgroup v1 isn't 
>>> enabled. Not just this function, all the v1 specific functions should 
>>> have a !CONFIG_CPUSET_V1 version that can be optimized out.
>>>
>>> Cheers,
>>> Longman
>>>
>>>
>> Hi, Longman, currently, we have only added one place to manage all the 
>> functions whose are empty if !CONFIG_CPUSET_V1 is not defined , and 
>> the caller does not need to care about using cpuset v1 or v2, which 
>> makes it succinct.
>>
>> However, we have to add !CONFIG_CPUSET_V1 to many places to avoid 
>> useless function calls. I am not opposed to do this if you thick it 'a 
>> better implementation.
> 
> You don't need to touch any files that use these cgroup v1 only 
> functions. You only need to add some #ifdef code in the cpuset.h header 
> file like
> 
> #ifdef CONFIG_CPUSET_V1
> extern void cpuset_memory_pressure_bump(void);
> #else
> static inline void cpuset_memory_pressure_bump(void) { }
> #endif
> 
> BTW, try not to introduce unnecessary performance regression. The reason 
> cpuset_memory_pressure_bump() is a macro is to avoid a function call if 
> cpuset_memory_pressure_enabled isn't set which is most likely case. It 
> is cheaper to read a variable than do a function call.
> 
> Cheers,
> Longman
> 
> 
> 
> 
Thank you for your patience, I just misunderstood.
I will do it in v2.

Best regards,
Ridong