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

[Waiman Long <longman@redhat.com>]

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