sched/psi: Create the psimon kthread outside of cgroup_mutex

a5b98009f16d ("sched/psi: fix race between file release and pressure write") made pressure_write() hold cgroup_mutex across psi_trigger_create(), which forks the psimon kthread for the first rtpoll trigger. As kthread creation depends on the whole fork path, the commit inadvertently created a lot of unwanted locking dependencies from cgroup_mutex. sched_ext got hit by one: its enable path blocks forks and then grabs cgroup_mutex, so a pressure write racing a scheduler enable deadlocks, with every other fork piling up behind. Fix it by splitting trigger creation so that the worker is forked with cgroup_mutex dropped and the kernfs active reference left broken. The latter matters because rmdir and cgroup.pressure writes drain active references under cgroup_mutex. Publishing the trigger last keeps error reporting synchronous and preserves the of->priv lifetime rules. The trigger registered in the first stage pins the group's rtpoll machinery across the unlocked window, leaving only creation races to resolve. The catch-up poll on installation covers scheduling attempts dropped while there was no worker. v2: Retagged sched/psi (was cgroup).

Tejun Heo committed 4 months ago

Commit fadeedd1 parent 97fef603 files · +78 −18Browse files

Showing the changes to kernel (2 of 3 files). Show all

Unified · Split

2 files

kernel/cgroup/cgroup.c +22 −1
kernel/sched/psi.c +53 −16

kernel/cgroup/cgroup.c+22−1

@@ -3996,6 +3996,7 @@ static ssize_t pressure_write(struct kernfs_open_file of, char buf, · 3995 unmodified lines
3996	struct psi_trigger *new;	3996	struct psi_trigger *new;
3997	struct cgroup *cgrp;	3997	struct cgroup *cgrp;
3998	struct psi_group *psi;	3998	struct psi_group *psi;
		3999	bool need_rtpoll_worker;
3999	ssize_t ret = 0;	4000	ssize_t ret = 0;
4000		4001
4001	cgrp = cgroup_kn_lock_live(of->kn, false);	4002	cgrp = cgroup_kn_lock_live(of->kn, false);
@@ -4015,12 +4016,32 @@ static ssize_t pressure_write(struct kernfs_open_file of, char buf, · 13 unmodified lines
4015	}	4016	}
4016		4017
4017	psi = cgroup_psi(cgrp);	4018	psi = cgroup_psi(cgrp);
4018	new = psi_trigger_create(psi, buf, res, of->file, of);	4019	new = psi_trigger_create(psi, buf, res, of->file, of,
		4020	&need_rtpoll_worker);
4019	if (IS_ERR(new)) {	4021	if (IS_ERR(new)) {
4020	ret = PTR_ERR(new);	4022	ret = PTR_ERR(new);
4021	goto out_unlock;	4023	goto out_unlock;
4022	}	4024	}
4023		4025
		4026	/*
		4027	* The worker fork must run with neither cgroup_mutex nor the file's
		4028	* kernfs active reference held. The latter is broken since
		4029	* cgroup_kn_lock_live(). @of->priv may be released while unlocked, so
		4030	* recheck before publishing @new.
		4031	*/
		4032	if (need_rtpoll_worker) {
		4033	cgroup_unlock();
		4034	ret = psi_trigger_create_rtpoll_worker(psi);
		4035	cgroup_lock();
		4036
		4037	if (!ret && !of->priv)
		4038	ret = -ENODEV;
		4039	if (ret) {
		4040	psi_trigger_destroy(new);
		4041	goto out_unlock;
		4042	}
		4043	}
		4044
4024	smp_store_release(&ctx->psi.trigger, new);	4045	smp_store_release(&ctx->psi.trigger, new);
4025		4046
4026	out_unlock:	4047	out_unlock:

kernel/sched/psi.c+53−16

 	return 0;
 }
+/*
+ * Create @group's rtpoll worker after psi_trigger_create() reported the need
+ * for one. kthread creation depends on the whole fork path and we don't want
+ * all of that nested inside cgroup_mutex, so the caller must drop it and any
+ * other lock that forks can wait behind. If two callers race, the loser stops
+ * its never-woken kthread.
+ */
+int psi_trigger_create_rtpoll_worker(struct psi_group *group)
+{
+	struct task_struct *task;
+	task = kthread_create(psi_rtpoll_worker, group, "psimon");
+	if (IS_ERR(task))
+		return PTR_ERR(task);
+	scoped_guard(mutex, &group->rtpoll_trigger_lock) {
+		if (!rcu_access_pointer(group->rtpoll_task)) {
+			atomic_set(&group->rtpoll_wakeup, 0);
+			wake_up_process(task);
+			rcu_assign_pointer(group->rtpoll_task, task);
+			/*
+			 * Poll once to catch up on scheduling attempts dropped
+			 * while there was no rtpoll worker.
+			 */
+			psi_schedule_rtpoll_work(group, 1, true);
+			return 0;
+		}
+	}
+	kthread_stop(task);
+	return 0;
+}
 struct psi_trigger *psi_trigger_create(struct psi_group *group, char *buf,
 				       enum psi_res res, struct file *file,
-				       struct kernfs_open_file *of)
+				       struct kernfs_open_file *of,
+				       bool *need_rtpoll_worker)
 {
 	struct psi_trigger *t;
 	enum psi_states state;
 	bool privileged;
 	u32 window_us;
+	*need_rtpoll_worker = false;
 	if (static_branch_likely(&psi_disabled))
 		return ERR_PTR(-EOPNOTSUPP);
 	if (privileged) {
 		mutex_lock(&group->rtpoll_trigger_lock);
-		if (!rcu_access_pointer(group->rtpoll_task)) {
-			struct task_struct *task;
-			task = kthread_create(psi_rtpoll_worker, group, "psimon");
-			if (IS_ERR(task)) {
-				kfree(t);
-				mutex_unlock(&group->rtpoll_trigger_lock);
-				return ERR_CAST(task);
-			}
-			atomic_set(&group->rtpoll_wakeup, 0);
-			wake_up_process(task);
-			rcu_assign_pointer(group->rtpoll_task, task);
-		}
 		list_add(&t->node, &group->rtpoll_triggers);
 		group->rtpoll_min_period = min(group->rtpoll_min_period,
 			div_u64(t->win.size, UPDATES_PER_WINDOW));
 		group->rtpoll_nr_triggers[t->state]++;
 		group->rtpoll_states |= (1 << t->state);
+		*need_rtpoll_worker = !rcu_access_pointer(group->rtpoll_task);
 		mutex_unlock(&group->rtpoll_trigger_lock);
 	} else {
 		mutex_lock(&group->avgs_lock);
 	size_t buf_size;
 	struct seq_file *seq;
 	struct psi_trigger *new;
+	bool need_rtpoll_worker;
+	int ret;
 	if (static_branch_likely(&psi_disabled))
 		return -EOPNOTSUPP;
 		return -EBUSY;
 	}
-	new = psi_trigger_create(&psi_system, buf, res, file, NULL);
+	new = psi_trigger_create(&psi_system, buf, res, file, NULL,
+				 &need_rtpoll_worker);
 	if (IS_ERR(new)) {
 		mutex_unlock(&seq->lock);
 		return PTR_ERR(new);
 	}
+	if (need_rtpoll_worker) {
+		ret = psi_trigger_create_rtpoll_worker(&psi_system);
+		if (ret) {
+			psi_trigger_destroy(new);
+			mutex_unlock(&seq->lock);
+			return ret;
+		}
+	}
 	smp_store_release(&seq->private, new);
 	mutex_unlock(&seq->lock);