cgroup, sched: restructure threadgroup locking and replace it with a percpu_rwsem

include/linux/sched.h +6 -11

+	cgroup_threadgroup_change_begin(tsk);
 }
 
 /**
- * threadgroup_unlock - unlock threadgroup
- * @tsk: member task of the threadgroup to unlock
+ * threadgroup_change_end - mark the end of changes to a threadgroup
+ * @tsk: task causing the changes
  *
- * Reverse threadgroup_lock().
+ * See threadgroup_change_begin().
  */
-static inline void threadgroup_unlock(struct task_struct *tsk)
+static inline void threadgroup_change_end(struct task_struct *tsk)
 {
-	up_write(&tsk->signal->group_rwsem);
+	cgroup_threadgroup_change_end(tsk);
 }
-#else
-static inline void threadgroup_change_begin(struct task_struct *tsk) {}
-static inline void threadgroup_change_end(struct task_struct *tsk) {}
-static inline void threadgroup_lock(struct task_struct *tsk) {}
-static inline void threadgroup_unlock(struct task_struct *tsk) {}
-#endif
 
 #ifndef __HAVE_THREAD_FUNCTIONS

kernel/cgroup.c +42 -0

index cfa27f9..9309452 100644
@@ -848,6 +848,48 @@ static struct css_set *find_css_set(struct css_set *old_cset,
 	return cset;
 }
 
+void cgroup_threadgroup_change_begin(struct task_struct *tsk)
+{
+	down_read(&tsk->signal->group_rwsem);
+}
+
+void cgroup_threadgroup_change_end(struct task_struct *tsk)
+{
+	up_read(&tsk->signal->group_rwsem);
+}
+
+/**
+ * threadgroup_lock - lock threadgroup
+ * @tsk: member task of the threadgroup to lock
+ *
+ * Lock the threadgroup @tsk belongs to.  No new task is allowed to enter
+ * and member tasks aren't allowed to exit (as indicated by PF_EXITING) or
+ * change ->group_leader/pid.  This is useful for cases where the threadgroup
+ * needs to stay stable across blockable operations.
+ *
+ * fork and exit explicitly call threadgroup_change_{begin|end}() for
+ * synchronization.  While held, no new task will be added to threadgroup
+ * and no existing live task will have its PF_EXITING set.
+ *
+ * de_thread() does threadgroup_change_{begin|end}() when a non-leader
+ * sub-thread becomes a new leader.
+ */
+static void threadgroup_lock(struct task_struct *tsk)
+{
+	down_write(&tsk->signal->group_rwsem);
+}
+
+/**
+ * threadgroup_unlock - unlock threadgroup
+ * @tsk: member task of the threadgroup to unlock
+ *
+ * Reverse threadgroup_lock().
+ */
+static inline void threadgroup_unlock(struct task_struct *tsk)
+{
+	up_write(&tsk->signal->group_rwsem);
+}
+
 static struct cgroup_root *cgroup_root_from_kf(struct kernfs_root *kf_root)
 {
 	struct cgroup *root_cgrp = kf_root->kn->priv;

Signature · 6 lines

2.1.0

--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

include/linux/sched.h static inline void unlock_task_sighand(struct task_struct *tsk, · line 2665

Quoted 4 lines

include/linux/sched.h +2 -1

+static inline void threadgroup_change_begin(struct task_struct *tsk)
 {
-	down_write(&tsk->signal->group_rwsem);
+	might_sleep();

I think cgroup_threadgroup_change_begin()->down_read() already does might_sleep() check.

kernel/cgroup.c +4 -4

@@ -2480,7 +2442,7 @@ retry_find_task:
 	get_task_struct(tsk);
 	rcu_read_unlock();
 
-	threadgroup_lock(tsk);
+	percpu_down_write(&cgroup_threadgroup_rwsem);
 	if (threadgroup) {
 		if (!thread_group_leader(tsk)) {
 			/*

@@ -2490,7 +2452,7 @@ retry_find_task:
 			 * try again; this is
 			 * "double-double-toil-and-trouble-check locking".
 			 */
-			threadgroup_unlock(tsk);
+			percpu_up_write(&cgroup_threadgroup_rwsem);
 			put_task_struct(tsk);
 			goto retry_find_task;
 		}

@@ -2703,17 +2665,17 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 				goto out_finish;
 			last_task = task;
 
-			threadgroup_lock(task);
+			percpu_down_write(&cgroup_threadgroup_rwsem);
 			/* raced against de_thread() from another thread? */
 			if (!thread_group_leader(task)) {
-				threadgroup_unlock(task);
+				percpu_up_write(&cgroup_threadgroup_rwsem);
 				put_task_struct(task);

.gitconfig:

[diff "default"]

        xfuncname = "^[[:alpha:]$_].*[^:]$"

Will avoid keying on labels like that and show us this is __cgroup_procs_write().

So my only worry with this patch-set is that these operations will be hugely expensive.

Now it looks like the cgroup_update_dfl_csses() thing is very rare, its when you change which controllers are active in a given subtree under the uber-l337-super-comount design.

The other one, __cgorup_procs_write() is every /procs, /tasks write to a cgroup, and that does worry me, this could be a somewhat common thing.

The Changelog states task migration is a cold path, but is tens of miliseconds per task really no problem?

Hello, Peter.

Quoted 7 lines · Peter Zijlstra

.gitconfig:

[diff "default"]

        xfuncname = "^[[:alpha:]$_].*[^:]$"

Will avoid keying on labels like that and show us this is __cgroup_procs_write().

Ah, nice trick.

Quoted 10 lines

So my only worry with this patch-set is that these operations will be hugely expensive.

Now it looks like the cgroup_update_dfl_csses() thing is very rare, its when you change which controllers are active in a given subtree under the uber-l337-super-comount design.

The other one, __cgorup_procs_write() is every /procs, /tasks write to a cgroup, and that does worry me, this could be a somewhat common thing.

Quoted 2 lines

The Changelog states task migration is a cold path, but is tens of miliseconds per task really no problem?

The latency is bound by synchronize_sched_expedited(). Given the way cgroups are used in majority of setups (process migration happening only during service / session setups), I think this should be okay.

I agree that something which is closer to lglock in characteristics would fit the workload better tho. If this actually becomes a problem, we can come up with a different percpu locking scheme which puts a bit more overhead on the reader side to reduce the latency / overhead on the writer side which shouldn't be that difficult but let's see whether we need to get there at all.

Thanks.

Quoted 3 lines

The latency is bound by synchronize_sched_expedited(). Given the way cgroups are used in majority of setups (process migration happening only during service / session setups), I think this should be okay.

Actually process migration can happen quite frequently, for example in Android phones, and that's why Google had an out-of-tree patch to remove the synchronize_rcu() in that path, which turned out to be buggy.

Hello, Li.

Quoted 7 lines · Zefan Li

Quoted 3 lines

The latency is bound by synchronize_sched_expedited(). Given the way cgroups are used in majority of setups (process migration happening only during service / session setups), I think this should be okay.

Actually process migration can happen quite frequently, for example in Android phones, and that's why Google had an out-of-tree patch to remove the synchronize_rcu() in that path, which turned out to be buggy.

It's still not a very frequent operation tho. We're talking about users switching fore/background jobs here and the expedited synchronization w/ preemption enabled doesn't take much time. In addition, as it currently stands, android is doing memory charge immigration on each fore/background switches. I'm pretty doubtful this would make any difference.

threadgroup locking was added because cgroup needs threadgroups to stay stable across attach operations. It was implemented as a per-signal_struct generic locking mechanism so that other users which require threadgroups stable across blocking operations can use it too; however, it hasn't grown any other use cases and still conditionalized on CONFIG_CGROUPS.

Quoted 6 lines · Tejun Heo

threadgroup locking was added because cgroup needs threadgroups to stay stable across attach operations. It was implemented as a per-signal_struct generic locking mechanism so that other users which require threadgroups stable across blocking operations can use it too; however, it hasn't grown any other use cases and still conditionalized on CONFIG_CGROUPS.

Ingo, Peter, what do you guys think?

threadgroup locking was added because cgroup needs threadgroups to stay stable across attach operations. It was implemented as a per-signal_struct generic locking mechanism so that other users which require threadgroups stable across blocking operations can use it too; however, it hasn't grown any other use cases and still conditionalized on CONFIG_CGROUPS.