[v3,bpf-next,03/11] cgroup: Eliminate the need for cgroup_mutex in proc_cgroup_show()

Commit Message

Yafang Shao Oct. 29, 2023, 6:14 a.m. UTC

The cgroup root_list is already RCU-safe. Therefore, we can replace the
cgroup_mutex with the RCU read lock in some particular paths. This change
will be particularly beneficial for frequent operations, such as
`cat /proc/self/cgroup`, in a cgroup1-based container environment.

I did stress tests with this change, as outlined below
(with CONFIG_PROVE_RCU_LIST enabled):

- Continuously mounting and unmounting named cgroups in some tasks,
  for example:

  cgrp_name=$1
  while true
  do
      mount -t cgroup -o none,name=$cgrp_name none /$cgrp_name
      umount /$cgrp_name
  done

- Continuously triggering proc_cgroup_show() in some tasks concurrently,
  for example:
  while true; do cat /proc/self/cgroup > /dev/null; done

They can ran successfully after implementing this change, with no RCU
warnings in dmesg.

Signed-off-by: Yafang Shao <[e-mail]>
---
 kernel/cgroup/cgroup.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Tejun Heo Nov. 9, 2023, 9:20 p.m. UTC | #1

On Sun, Oct 29, 2023 at 06:14:30AM +0000, Yafang Shao wrote:
> The cgroup root_list is already RCU-safe. Therefore, we can replace the
> cgroup_mutex with the RCU read lock in some particular paths. This change
> will be particularly beneficial for frequent operations, such as
> `cat /proc/self/cgroup`, in a cgroup1-based container environment.
> 
> I did stress tests with this change, as outlined below
> (with CONFIG_PROVE_RCU_LIST enabled):
> 
> - Continuously mounting and unmounting named cgroups in some tasks,
>   for example:
> 
>   cgrp_name=$1
>   while true
>   do
>       mount -t cgroup -o none,name=$cgrp_name none /$cgrp_name
>       umount /$cgrp_name
>   done
> 
> - Continuously triggering proc_cgroup_show() in some tasks concurrently,
>   for example:
>   while true; do cat /proc/self/cgroup > /dev/null; done
> 
> They can ran successfully after implementing this change, with no RCU
> warnings in dmesg.
> 
> Signed-off-by: Yafang Shao <[e-mail]>

Acked-by: Tejun Heo <[e-mail]>

Thanks.

Patch

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 28b8ccc..cc6a6d9 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6261,7 +6261,7 @@  int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
 	if (!buf)
 		goto out;
 
-	cgroup_lock();
+	rcu_read_lock();
 	spin_lock_irq(&css_set_lock);
 
 	for_each_root(root) {
@@ -6272,6 +6272,11 @@  int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
 		if (root == &cgrp_dfl_root && !READ_ONCE(cgrp_dfl_visible))
 			continue;
 
+		cgrp = task_cgroup_from_root(tsk, root);
+		/* The root has already been unmounted. */
+		if (!cgrp)
+			continue;
+
 		seq_printf(m, "%d:", root->hierarchy_id);
 		if (root != &cgrp_dfl_root)
 			for_each_subsys(ss, ssid)
@@ -6282,9 +6287,6 @@  int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
 			seq_printf(m, "%sname=%s", count ? "," : "",
 				   root->name);
 		seq_putc(m, ':');
-
-		cgrp = task_cgroup_from_root(tsk, root);
-
 		/*
 		 * On traditional hierarchies, all zombie tasks show up as
 		 * belonging to the root cgroup.  On the default hierarchy,
@@ -6316,7 +6318,7 @@  int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
 	retval = 0;
 out_unlock:
 	spin_unlock_irq(&css_set_lock);
-	cgroup_unlock();
+	rcu_read_unlock();
 	kfree(buf);
 out:
 	return retval;