cgroup: Elide write-locking threadgroup_rwsem when updating csses on an empty subtree

kernel/cgroup/cgroup.c +3 -0

+void cgroup_favor_dynmods(struct cgroup_root *root, bool favor)
+{
+	bool favoring = root->flags & CGRP_ROOT_FAVOR_DYNMODS;

I see no problems in this patch. But just for record, we do not need synchronize_rcu() in the "favor && !favoring" case, so we cab probably do something like

	@@ -146,13 +146,20 @@ void rcu_sync_enter(struct rcu_sync *rsp)
			 * See the comment above, this simply does the "synchronous"
			 * call_rcu(rcu_sync_func) which does GP_ENTER -> GP_PASSED.
			 */
	+		if (wait) {
	+			synchronize_rcu();
	+			rcu_sync_func(&rsp->cb_head);
	+		} else {
	+			rcu_sync_call(rsp);
	+		}
	+	} else if (wait) {
	+		wait_event(rsp->gp_wait, READ_ONCE(rsp->gp_state) >= GP_PASSED);

later.

__rcu_sync_enter(rsp, false) works just like rcu_sync_enter_start() but it can be safely called at any moment.

And can't resist, off-topic question... Say, cgroup_attach_task_all() does

	mutex_lock(&cgroup_mutex);
	percpu_down_write(&cgroup_threadgroup_rwsem);

and this means that synchronize_rcu() can be called with cgroup_mutex held. Perhaps it makes sense to change this code to do

	rcu_sync_enter(&cgroup_threadgroup_rwsem.rss);
	mutex_lock(&cgroup_mutex);
	percpu_down_write(&cgroup_threadgroup_rwsem);
	...
	percpu_up_write(&cgroup_threadgroup_rwsem);
	mutex_unlock(&cgroup_mutex);
	rcu_sync_exit(&cgroup_threadgroup_rwsem.rss);

? Just curious.

kernel/cgroup/cgroup.c int __init cgroup_init(void) · line 5861

Quoted 5 lines

kernel/cgroup/cgroup.c +0 -5

-	/*
-	 * The latency of the synchronize_rcu() is too high for cgroups,
-	 * avoid it at the cost of forcing all readers into the slow path.
-	 */
-	rcu_sync_enter_start(&cgroup_threadgroup_rwsem.rss);

Note that it doesn't have other users, probably you can kill it.

Quoted 3 lines

__rcu_sync_enter(rsp, false) works just like rcu_sync_enter_start() but it can be safely called at any moment.

Yeah, I originally used rcu_sync_enter_start() but quickly found out that it can't be reverted reliably. Given how cold the option switching path is, I think it's fine to pay an extra synchronize_rcu() there rather than adding more complexity to rcu_sync_enter() unless this will be useful somewhere else too.

Quoted 16 lines

And can't resist, off-topic question... Say, cgroup_attach_task_all() does

	mutex_lock(&cgroup_mutex);
	percpu_down_write(&cgroup_threadgroup_rwsem);

and this means that synchronize_rcu() can be called with cgroup_mutex held. Perhaps it makes sense to change this code to do

	rcu_sync_enter(&cgroup_threadgroup_rwsem.rss);
	mutex_lock(&cgroup_mutex);
	percpu_down_write(&cgroup_threadgroup_rwsem);
	...
	percpu_up_write(&cgroup_threadgroup_rwsem);
	mutex_unlock(&cgroup_mutex);
	rcu_sync_exit(&cgroup_threadgroup_rwsem.rss);

Quoted 1 line

? Just curious.

I'm not quite following.

Are you saying that if we switching the rwsem into
slow mode before grabbing the locks, we can avoid inducing latencies on other users?

Hmm... assuming that I'm understanding you correctly, one problem with that approach is that everyone would be doing synchronize_rcu() whether they want to change favoring state.

3942a9bd7b58 ("locking, rcu, cgroup: Avoid synchronize_sched() in __cgroup_procs_write()") disabled percpu operations on threadgroup_rwsem because the impiled synchronize_rcu() on write locking was pushing up the latencies too much for android which constantly moves processes between cgroups.

This makes the hotter paths - fork and exit - slower as they're always forced into the slow path. There is no reason to force this on everyone especially given that more common static usage pattern can now completely avoid write-locking the rwsem. Write-locking is elided when turning on and off controllers on empty sub-trees and CLONE_INTO_CGROUP enables seeding a cgroup without grabbing the rwsem.

Restore the default percpu operations and introduce the mount option "favordynmods" and config option CGROUP_FAVOR_DYNMODS for users who need lower latencies for the dynamic operations.

Signed-off-by Tejun Heo <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Michal Koutný <mkoutny-IBi9RG/[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc John Stultz <[email protected]>

Cc Dmitry Shmidt <[email protected]>

Cc Oleg Nesterov <[email protected]>

---

This makes the hotter paths - fork and exit - slower as they're always forced into the slow path. There is no reason to force this on everyone especially given that more common static usage pattern can now completely avoid write-locking the rwsem. Write-locking is elided when turning on and off controllers on empty sub-trees and CLONE_INTO_CGROUP enables seeding a cgroup without grabbing the rwsem.

include/linux/cgroup-defs.h +4 -1

+	CGRP_ROOT_FAVOR_DYNMODS = (1 << 4),
+
+	/*
 	 * Enable cpuset controller in v1 cgroup to use v2 behavior.
 	 */
-	CGRP_ROOT_CPUSET_V2_MODE = (1 << 4),
+	CGRP_ROOT_CPUSET_V2_MODE = (1 << 16),
 
 	/*

Quoted 6 lines · Tejun Heo

This makes the hotter paths - fork and exit - slower as they're always forced into the slow path. There is no reason to force this on everyone especially given that more common static usage pattern can now completely avoid write-locking the rwsem. Write-locking is elided when turning on and off controllers on empty sub-trees and CLONE_INTO_CGROUP enables seeding a cgroup without grabbing the rwsem.

Just a practical note that CLONE_INTO_CGROUP may not be so widespread yet [1][2].
But generally, the change makes sense to me.

Quoted 14 lines

include/linux/cgroup-defs.h +5 -2

+	CGRP_ROOT_FAVOR_DYNMODS = (1 << 4),
+
+	/*
 	 * Enable cpuset controller in v1 cgroup to use v2 behavior.
 	 */
-	CGRP_ROOT_CPUSET_V2_MODE = (1 << 4),
+	CGRP_ROOT_CPUSET_V2_MODE = (1 << 16),
 
 	/*
 	 * Enable legacy local memory.events.
 	 */
-	CGRP_ROOT_MEMORY_LOCAL_EVENTS = (1 << 5),
+	CGRP_ROOT_MEMORY_LOCAL_EVENTS = (1 << 17),

include/linux/cgroup-defs.h enum { · line 117

Quoted 5 lines

include/linux/cgroup-defs.h +1 -1

 	/*
 	 * Enable recursive subtree protection
 	 */
-	CGRP_ROOT_MEMORY_RECURSIVE_PROT = (1 << 6),
+	CGRP_ROOT_MEMORY_RECURSIVE_PROT = (1 << 18),

Why this new gap in flag bits?

We allow modifying these mount options via remount. Let's add "no" prefixed variants so that they can be turned off too.

// on v5.19-rc?
:~ # grep cg /proc/mounts
cgroup2 /sys/fs/cgroup cgroup2 rw,nosuid,nodev,noexec,relatime,nsdelegate 0 0 :~ # mount -t cgroup2 cgroup2 /sys/fs/cgroup/ -oremount
:~ # grep cg /proc/mounts
cgroup2 /sys/fs/cgroup cgroup2 rw,relatime 0 0

The mountflags and data arguments should match the values used in the original mount() call, except for those parameters that are being deliberately changed.

Quoted 2 lines · Tejun Heo

We allow modifying these mount options via remount. Let's add "no" prefixed variants so that they can be turned off too.

They can be turned off:

Quoted 6 lines

// on v5.19-rc?
:~ # grep cg /proc/mounts
cgroup2 /sys/fs/cgroup cgroup2 rw,nosuid,nodev,noexec,relatime,nsdelegate 0 0 :~ # mount -t cgroup2 cgroup2 /sys/fs/cgroup/ -oremount
:~ # grep cg /proc/mounts
cgroup2 /sys/fs/cgroup cgroup2 rw,relatime 0 0

The mount(2) says about remounting:

Quoted 3 lines

The mountflags and data arguments should match the values used in the original mount() call, except for those parameters that are being deliberately changed.

Or is this a provision for the fsconfig(2) API?

kernel/cgroup/cgroup.c int cgroup_show_path(struct seq_file *sf · line 1872

Quoted 2 lines

kernel/cgroup/cgroup.c +2 -0

+	fsparam_flag("memory_nolocalevents",	Opt_memory_nolocalevents),
+	fsparam_flag("memory_norecursiveprot",	Opt_memory_norecursiveprot),

These are not 'no' prefixes of the option :-)

I.e. it seem more consistent to prefix whole boolean option name (in accordance with other FS options but I know limited subset of them). In the end, this should be handled generically for boolean options in the VFS and not via custom options.

Also, this allows both
'nsdelegate,nonsdelegate'
and
'nonsdelegate,nsdelegate'
(nsdelegate is just an example) where the 'no' always overrides being a hidden implementation detail.

I find this patch a bit weird.

Quoted 4 lines · Michal Koutný

Quoted 2 lines · Tejun Heo

We allow modifying these mount options via remount. Let's add "no" prefixed variants so that they can be turned off too.

Quoted 8 lines

They can be turned off:

Quoted 6 lines

// on v5.19-rc?
:~ # grep cg /proc/mounts
cgroup2 /sys/fs/cgroup cgroup2 rw,nosuid,nodev,noexec,relatime,nsdelegate 0 0 :~ # mount -t cgroup2 cgroup2 /sys/fs/cgroup/ -oremount
:~ # grep cg /proc/mounts
cgroup2 /sys/fs/cgroup cgroup2 rw,relatime 0 0

While a bit confusing, given that there is a way to turn off the options, there's no reason to have the explicit "no" prefixed options. Let's remove them.

 Documentation/admin-guide/cgroup-v2.rst |    8 ++++----
 kernel/cgroup/cgroup.c                  |   24 ++++--------------------
 2 files changed, 8 insertions(+), 24 deletions(-)

On Tue, Jul 26, 2022 at 01:48:17PM -1000, Tejun Heo <[email protected]> wrote:
Thanks.

Quoted 3 lines

While a bit confusing, given that there is a way to turn off the options, there's no reason to have the explicit "no" prefixed options. Let's remove them.

This is sensible...

Quoted 3 lines

 Documentation/admin-guide/cgroup-v2.rst |    8 ++++----
 kernel/cgroup/cgroup.c                  |   24 ++++--------------------
 2 files changed, 8 insertions(+), 24 deletions(-)

...and cleaner.

However, if the subtree doesn't contain any tasks, there aren't gonna be any cgroup migrations.

This condition can be trivially detected by testing whether mgctx.preloaded_src_csets is empty. Elide write-locking threadgroup_rwsem if the subtree is empty.

 kernel/cgroup/cgroup.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)