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

Chen Ridong posted 11 patches 1 year, 5 months ago
There is a newer version of this series
[PATCH v1 -next 10/11] cgroup/cpuset: guard cpuset-v1 code under CONFIG_CPUSETS_V1
Posted by Chen Ridong 1 year, 5 months ago
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);
diff --git a/init/Kconfig b/init/Kconfig
index b9ec6b6ef3a5..369941de9c2c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1144,6 +1144,19 @@ config CPUSETS
 
 	  Say N if unsure.
 
+config CPUSETS_V1
+	bool "Legacy cgroup v1 cpusets controller"
+	depends on CPUSETS
+	default n
+	help
+	  Legacy cgroup v1 cpusets controller which has been deprecated by
+	  cgroup v2 implementation. The v1 is there for legacy applications
+	  which haven't migrated to the new cgroup v2 interface yet. If you
+	  do not have any such application then you are completely fine leaving
+	  this option disabled.
+
+	  Say N if unsure.
+
 config PROC_PID_CPUSET
 	bool "Include legacy /proc/<pid>/cpuset file"
 	depends on CPUSETS
diff --git a/kernel/cgroup/Makefile b/kernel/cgroup/Makefile
index 005ac4c675cb..a5c9359d516f 100644
--- a/kernel/cgroup/Makefile
+++ b/kernel/cgroup/Makefile
@@ -4,6 +4,7 @@ obj-y := cgroup.o rstat.o namespace.o cgroup-v1.o freezer.o
 obj-$(CONFIG_CGROUP_FREEZER) += legacy_freezer.o
 obj-$(CONFIG_CGROUP_PIDS) += pids.o
 obj-$(CONFIG_CGROUP_RDMA) += rdma.o
-obj-$(CONFIG_CPUSETS) += cpuset.o cpuset-v1.o
+obj-$(CONFIG_CPUSETS) += cpuset.o
+obj-$(CONFIG_CPUSETS_V1) += cpuset-v1.o
 obj-$(CONFIG_CGROUP_MISC) += misc.o
 obj-$(CONFIG_CGROUP_DEBUG) += debug.o
diff --git a/kernel/cgroup/cpuset-internal.h b/kernel/cgroup/cpuset-internal.h
index 36e9686c7bba..9ccc4f54eefd 100644
--- a/kernel/cgroup/cpuset-internal.h
+++ b/kernel/cgroup/cpuset-internal.h
@@ -279,6 +279,7 @@ int cpuset_common_seq_show(struct seq_file *sf, void *v);
 /*
  * cpuset-v1.c
  */
+#ifdef CONFIG_CPUSETS_V1
 extern struct cftype legacy_files[];
 void fmeter_init(struct fmeter *fmp);
 void cpuset_update_task_spread_flags(struct cpuset *cs,
@@ -288,5 +289,19 @@ void hotplug_update_tasks_legacy(struct cpuset *cs,
 			    struct cpumask *new_cpus, nodemask_t *new_mems,
 			    bool cpus_updated, bool mems_updated);
 int validate_change_legacy(struct cpuset *cur, struct cpuset *trial);
+void cpuset_memory_pressure_bump(void);
+
+#else
+static inline void fmeter_init(struct fmeter *fmp) {}
+static inline void cpuset_update_task_spread_flags(struct cpuset *cs,
+					struct task_struct *tsk) {}
+static inline void update_tasks_flags(struct cpuset *cs) {}
+static inline void hotplug_update_tasks_legacy(struct cpuset *cs,
+			    struct cpumask *new_cpus, nodemask_t *new_mems,
+			    bool cpus_updated, bool mems_updated) {}
+static inline int validate_change_legacy(struct cpuset *cur,
+				struct cpuset *trial) { return 0; }
+void cpuset_memory_pressure_bump(void) {}
 
+#endif /* CONFIG_CPUSETS_V1 */
 #endif /* __CPUSET_INTERNAL_H */
diff --git a/kernel/cgroup/cpuset-v1.c b/kernel/cgroup/cpuset-v1.c
index ffb8711cc8fa..efa5ab0167b6 100644
--- a/kernel/cgroup/cpuset-v1.c
+++ b/kernel/cgroup/cpuset-v1.c
@@ -137,11 +137,13 @@ int cpuset_memory_pressure_enabled __read_mostly;
  * (direct) page reclaim by any task attached to the cpuset.
  */
 
-void __cpuset_memory_pressure_bump(void)
+void cpuset_memory_pressure_bump(void)
 {
-	rcu_read_lock();
-	fmeter_markevent(&task_cs(current)->fmeter);
-	rcu_read_unlock();
+	if (cpuset_memory_pressure_enabled) {
+		rcu_read_lock();
+		fmeter_markevent(&task_cs(current)->fmeter);
+		rcu_read_unlock();
+	}
 }
 
 static int update_relax_domain_level(struct cpuset *cs, s64 val)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 903e7bab399a..658a9e00671b 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -3617,7 +3617,9 @@ struct cgroup_subsys cpuset_cgrp_subsys = {
 	.can_fork	= cpuset_can_fork,
 	.cancel_fork	= cpuset_cancel_fork,
 	.fork		= cpuset_fork,
+#ifdef CONFIG_CPUSETS_V1
 	.legacy_cftypes	= legacy_files,
+#endif
 	.dfl_cftypes	= dfl_files,
 	.early_init	= true,
 	.threaded	= true,
-- 
2.34.1
Re: [PATCH v1 -next 10/11] cgroup/cpuset: guard cpuset-v1 code under CONFIG_CPUSETS_V1
Posted by Waiman Long 1 year, 5 months ago
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
Re: [PATCH v1 -next 10/11] cgroup/cpuset: guard cpuset-v1 code under CONFIG_CPUSETS_V1
Posted by Chen Ridong 1 year, 5 months ago

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.

Thanks,
Ridong

Re: [PATCH v1 -next 10/11] cgroup/cpuset: guard cpuset-v1 code under CONFIG_CPUSETS_V1
Posted by Waiman Long 1 year, 5 months ago
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



Re: [PATCH v1 -next 10/11] cgroup/cpuset: guard cpuset-v1 code under CONFIG_CPUSETS_V1
Posted by Chen Ridong 1 year, 5 months ago

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