[PATCHSET sched_ext/for-7.3] sched_ext: Sparse annotation cleanups

[PATCHSET sched_ext/for-7.3] sched_ext: Sparse annotation cleanups

[Tejun Heo]

Hello,

A sparse sweep of kernel/sched/ext flagged the __rcu pointers that are
read with plain loads. The reads are all protected by their calling
contexts but didn't state how. Add accessors that encode the protection
and convert the readers:

- cgrp->scx_sched readers go through the new scx_cgroup_sched() which
  states the locks that stabilize the association (0001).

- The naked scx_root accesses are resolved through the sched at hand or
  through new accessors that state the protection. One transitional
  marker remains (0002).

- The dsq->first_task identity comparison uses rcu_access_pointer()
  (0003).

What remains in the sparse output on kernel/sched/ext is the BPF kfunc
declaration noise shared with the rest of the tree, the rq->curr and
rq->donor accesses shared with the core scheduler, and the deliberately
naked scx_root access in touch_core_sched_dispatch() to be resolved
separately.

Based on sched_ext/for-7.3 (94ca9591108a).

Tejun Heo (3):
  sched_ext: Add scx_cgroup_sched() for cgrp->scx_sched reads
  sched_ext: Resolve most remaining scx_root accesses
  sched_ext: Use rcu_access_pointer() for the first_task comparison

Git tree: git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext.git scx-sparse-cleanups

 kernel/sched/ext/ext.c      | 20 ++++++++++----------
 kernel/sched/ext/idle.c     | 15 +++++++--------
 kernel/sched/ext/internal.h | 27 +++++++++++++++++++++++++++
 kernel/sched/ext/sub.c      | 33 +++++++++++++++++++--------------
 kernel/sched/ext/sub.h      | 14 ++++++++++++++
 5 files changed, 77 insertions(+), 32 deletions(-)

--
tejun

[PATCH 1/3] sched_ext: Add scx_cgroup_sched() for cgrp->scx_sched reads

[Tejun Heo]

cgrp->scx_sched is __rcu and published with rcu_assign_pointer() but every
reader loads it with a plain access, so sparse flags all of them. The reads
are lock-protected: enable/disable paths rewrite the field under all of
scx_enable_mutex, scx_fork_rwsem and cgroup_mutex, and cgroup creation
inherits the parent's sched under cgroup_mutex before the new cgroup is
reachable, so holding any one of the three locks makes the read stable.

Add scx_cgroup_sched() which states the protection with
rcu_dereference_check() and convert the readers. No functional changes.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/sched/ext/ext.c |  6 +++---
 kernel/sched/ext/sub.c | 16 +++++++++-------
 kernel/sched/ext/sub.h | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 81c2d8eeae41..d172ff5f7dc3 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -3795,7 +3795,7 @@ int scx_fork(struct task_struct *p, struct kernel_clone_args *kargs)
 
 	if (scx_init_task_enabled) {
 #ifdef CONFIG_EXT_SUB_SCHED
-		struct scx_sched *sch = kargs->cset->dfl_cgrp->scx_sched;
+		struct scx_sched *sch = scx_cgroup_sched(kargs->cset->dfl_cgrp);
 #else
 		struct scx_sched *sch = scx_root;
 #endif
@@ -4449,7 +4449,7 @@ int scx_tg_online(struct task_group *tg)
 		 * always belongs to the root sched.
 		 */
 		if (cgroup_on_dfl(tg->css.cgroup))
-			sch = tg->css.cgroup->scx_sched;
+			sch = scx_cgroup_sched(tg->css.cgroup);
 		else
 			sch = scx_tg_sched(&root_task_group);
 
@@ -4530,7 +4530,7 @@ int scx_cgroup_can_attach(struct cgroup_taskset *tset)
 		 * cgroup's sched and is reported through the
 		 * exit_task/init_task pair that the re-homing generates.
 		 */
-		if (!sch || sch != task_css_set(p)->mg_dst_cset->dfl_cgrp->scx_sched)
+		if (!sch || sch != scx_cgroup_sched(task_css_set(p)->mg_dst_cset->dfl_cgrp))
 			continue;
 
 		if (SCX_HAS_OP(sch, cgroup_prep_move)) {
diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
index 6da6c91e4287..2a8c979c7976 100644
--- a/kernel/sched/ext/sub.c
+++ b/kernel/sched/ext/sub.c
@@ -1204,7 +1204,7 @@ void scx_sub_disable(struct scx_sched *sch)
 /* verify that a scheduler can be attached to @cgrp and return the parent */
 static struct scx_sched *find_parent_sched(struct cgroup *cgrp)
 {
-	struct scx_sched *parent = cgrp->scx_sched;
+	struct scx_sched *parent = scx_cgroup_sched(cgrp);
 	struct scx_sched *pos;
 
 	lockdep_assert_held(&scx_sched_lock);
@@ -1578,7 +1578,7 @@ static s32 scx_cgroup_task_migrating(struct cgroup_task_migrate_ctx *ctx)
 	if (!scx_cgroup_enabled)
 		return NOTIFY_OK;
 
-	to = ctx->dst_dcgrp->scx_sched;
+	to = scx_cgroup_sched(ctx->dst_dcgrp);
 	if (scx_task_on_sched(to, p))
 		return NOTIFY_OK;
 
@@ -1612,7 +1612,7 @@ static void scx_cgroup_task_migrated(struct cgroup_task_migrate_ctx *ctx)
 	if (!scx_cgroup_enabled)
 		return;
 
-	to = ctx->dst_dcgrp->scx_sched;
+	to = scx_cgroup_sched(ctx->dst_dcgrp);
 	if (scx_task_on_sched(to, p))
 		return;
 
@@ -1639,7 +1639,7 @@ static void scx_cgroup_task_migrate_canceled(struct cgroup_task_migrate_ctx *ctx
 	if (!scx_cgroup_enabled)
 		return;
 
-	to = ctx->dst_dcgrp->scx_sched;
+	to = scx_cgroup_sched(ctx->dst_dcgrp);
 	if (scx_task_on_sched(to, p))
 		return;
 
@@ -1653,6 +1653,7 @@ static s32 scx_cgroup_lifetime_notify(struct notifier_block *nb,
 {
 	struct cgroup *cgrp = data;
 	struct cgroup *parent = cgroup_parent(cgrp);
+	struct scx_sched *sch;
 
 	if (!cgroup_on_dfl(cgrp))
 		return NOTIFY_OK;
@@ -1661,12 +1662,13 @@ static s32 scx_cgroup_lifetime_notify(struct notifier_block *nb,
 	case CGROUP_LIFETIME_ONLINE:
 		/* inherit ->scx_sched from $parent */
 		if (parent)
-			rcu_assign_pointer(cgrp->scx_sched, parent->scx_sched);
+			rcu_assign_pointer(cgrp->scx_sched, scx_cgroup_sched(parent));
 		break;
 	case CGROUP_LIFETIME_OFFLINE:
 		/* if there is a sched attached, shoot it down */
-		if (cgrp->scx_sched && cgrp->scx_sched->cgrp == cgrp)
-			scx_exit(cgrp->scx_sched, SCX_EXIT_UNREG_KERN,
+		sch = scx_cgroup_sched(cgrp);
+		if (sch && sch->cgrp == cgrp)
+			scx_exit(sch, SCX_EXIT_UNREG_KERN,
 				 SCX_ECODE_RSN_CGROUP_OFFLINE,
 				 "cgroup %llu going offline", cgroup_id(cgrp));
 		break;
diff --git a/kernel/sched/ext/sub.h b/kernel/sched/ext/sub.h
index 625d7ce334aa..0019b75a2560 100644
--- a/kernel/sched/ext/sub.h
+++ b/kernel/sched/ext/sub.h
@@ -39,6 +39,20 @@ struct scx_dispatch_q *scx_local_or_reject_dsq(struct scx_sched *sch, struct rq
 bool scx_task_reenq_on_cap_revoke(struct rq *rq, struct task_struct *p);
 void scx_reenq_reject(struct rq *rq);
 
+/*
+ * cgrp->scx_sched is written by root/sub enable/disable under all of
+ * scx_enable_mutex, scx_fork_rwsem and cgroup_mutex. A new cgroup inherits the
+ * parent's sched under just cgroup_mutex but is not yet reachable by the other
+ * two lock holders. Any one of the three locks stabilizes the association.
+ */
+static inline struct scx_sched *scx_cgroup_sched(struct cgroup *cgrp)
+{
+	return rcu_dereference_check(cgrp->scx_sched,
+				     lockdep_is_held(&cgroup_mutex) ||
+				     percpu_rwsem_is_held(&scx_fork_rwsem) ||
+				     lockdep_is_held(&scx_enable_mutex));
+}
+
 static inline const char *sch_cgrp_path(struct scx_sched *sch)
 {
 	return sch->cgrp_path;
-- 
2.55.0

[PATCH 2/3] sched_ext: Resolve most remaining scx_root accesses

[Tejun Heo]

scx_root is __rcu and naked accesses were left as transitional markers for
the multi-scheduler transition, to be converted to accesses through the
associated scheduler instances. Most accesses have since been converted to
resolve the sched from the program or task at hand. The remaining naked
sites divide into ones that semantically always want the root sched, which
this patch resolves, and one that is left to a later patch.

The resolved sites:

- The SCX_OPS_TID_TO_TASK validation and the ecaps sync kick already hold a
  sched whose ancestors[] pins the root as entry 0 with plain pointers
  stable for the sched's lifetime. Reach the root through the sched at hand.

- The dispatch entry, class switch, idle notification and fork init paths
  only execute while the scheduler is live and scx_root never changes inside
  the live window, so no update can race them. Add scx_root_protected_live()
  which documents that invariant and resolves with a plain load.

- The hotplug path, including the ecaps reseeds, runs with the hotplug lock
  held, which excludes the scx_root writers. Add scx_root_protected(), which
  accepts either the hotplug lock or scx_enable_mutex.

- Is-root tests use a zero level instead of comparing against the global.

touch_core_sched_dispatch() stays naked, to be resolved by a later patch.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/sched/ext/ext.c      | 12 ++++++------
 kernel/sched/ext/idle.c     | 15 +++++++--------
 kernel/sched/ext/internal.h | 27 +++++++++++++++++++++++++++
 kernel/sched/ext/sub.c      | 17 ++++++++++-------
 4 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index d172ff5f7dc3..95aadce7b104 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -2786,7 +2786,7 @@ static inline void maybe_queue_balance_callback(struct rq *rq)
 
 static int balance_one(struct rq *rq, struct task_struct *prev)
 {
-	struct scx_sched *sch = scx_root;
+	struct scx_sched *sch = scx_root_protected_live();
 	s32 cpu = cpu_of(rq);
 
 	lockdep_assert_rq_held(rq);
@@ -2942,7 +2942,7 @@ preempt_reason_from_class(const struct sched_class *class)
 
 static void switch_class(struct rq *rq, struct task_struct *next)
 {
-	struct scx_sched *sch = scx_root;
+	struct scx_sched *sch = scx_root_protected_live();
 	const struct sched_class *next_class = next->sched_class;
 
 	if (!(sch->ops.flags & SCX_OPS_HAS_CPU_PREEMPT))
@@ -3333,7 +3333,7 @@ static void set_cpus_allowed_scx(struct task_struct *p,
 
 static void handle_hotplug(struct rq *rq, bool online)
 {
-	struct scx_sched *sch = scx_root;
+	struct scx_sched *sch = scx_root_protected();
 	s32 cpu = cpu_of(rq);
 	s32 cpu_or_cid = cpu;
 
@@ -3797,7 +3797,7 @@ int scx_fork(struct task_struct *p, struct kernel_clone_args *kargs)
 #ifdef CONFIG_EXT_SUB_SCHED
 		struct scx_sched *sch = scx_cgroup_sched(kargs->cset->dfl_cgrp);
 #else
-		struct scx_sched *sch = scx_root;
+		struct scx_sched *sch = scx_root_protected_live();
 #endif
 		scx_set_task_state(p, SCX_TASK_INIT_BEGIN);
 		ret = __scx_init_task(sch, p, NULL, true);
@@ -5728,7 +5728,7 @@ void scx_disable_bypass_dsp(struct scx_sched *sch)
 static void unbypass_renotify_idle(struct rq *rq, struct scx_sched *pos,
 				   struct scx_sched_pcpu *pcpu)
 {
-	if (pos == scx_root) {
+	if (!pos->level) {
 		rq->scx.flags |= SCX_RQ_ROOT_IDLE_RENOTIFY;
 		return;
 	}
@@ -7096,7 +7096,7 @@ int scx_validate_ops(struct scx_sched *sch, const struct sched_ext_ops *ops)
 	 * enabled it.
 	 */
 	if ((ops->flags & SCX_OPS_TID_TO_TASK) && scx_parent(sch) &&
-	    !(scx_root->ops.flags & SCX_OPS_TID_TO_TASK)) {
+	    !(sch->ancestors[0]->ops.flags & SCX_OPS_TID_TO_TASK)) {
 		scx_error(sch, "SCX_OPS_TID_TO_TASK requires root scheduler to enable it");
 		return -EINVAL;
 	}
diff --git a/kernel/sched/ext/idle.c b/kernel/sched/ext/idle.c
index d08166de03d8..ccfc35b1bba9 100644
--- a/kernel/sched/ext/idle.c
+++ b/kernel/sched/ext/idle.c
@@ -742,30 +742,29 @@ static void scx_idle_notify(struct rq *rq, bool idle, bool do_notify, bool root_
 {
 	s32 cpu = cpu_of(rq);
 	s32 cid = scx_cpu_arg(cpu);
+	struct scx_sched *root = scx_root_protected_live();
 	struct scx_sched *pos;
 
 	lockdep_assert_rq_held(rq);
 
 	/* with no sub-sched, only the root can be owed a notification */
 	if (!scx_has_subs()) {
-		struct scx_sched *sch = scx_root;
-
 		if ((do_notify || root_renotify) &&
-		    SCX_HAS_OP(sch, update_idle) && !scx_bypassing(sch, cpu))
-			SCX_CALL_OP(sch, update_idle, rq, cid, idle);
+		    SCX_HAS_OP(root, update_idle) && !scx_bypassing(root, cpu))
+			SCX_CALL_OP(root, update_idle, rq, cid, idle);
 		return;
 	}
 
-	pos = scx_next_descendant_pre(NULL, scx_root);
+	pos = scx_next_descendant_pre(NULL, root);
 	while (pos) {
 		bool forced = false;
 
 		if (unlikely(scx_missing_caps(pos, cpu, SCX_CAP_BASE))) {
-			pos = scx_skip_subtree_pre(pos, scx_root);
+			pos = scx_skip_subtree_pre(pos, root);
 			continue;
 		}
 
-		if (pos == scx_root) {
+		if (!pos->level) {
 			forced = root_renotify;
 		}
 #ifdef CONFIG_EXT_SUB_SCHED
@@ -777,7 +776,7 @@ static void scx_idle_notify(struct rq *rq, bool idle, bool do_notify, bool root_
 		if ((do_notify || forced) && SCX_HAS_OP(pos, update_idle) &&
 		    !scx_bypassing(pos, cpu))
 			SCX_CALL_OP(pos, update_idle, rq, cid, idle);
-		pos = scx_next_descendant_pre(pos, scx_root);
+		pos = scx_next_descendant_pre(pos, root);
 	}
 }
 
diff --git a/kernel/sched/ext/internal.h b/kernel/sched/ext/internal.h
index a9a853a71061..abe1e7653e1d 100644
--- a/kernel/sched/ext/internal.h
+++ b/kernel/sched/ext/internal.h
@@ -1996,6 +1996,33 @@ extern struct scx_sched *scx_enabling_sub_sched;
 #define scx_error(sch, fmt, args...)						\
 	scx_exit((sch), SCX_EXIT_ERROR, 0, fmt, ##args)
 
+/**
+ * scx_root_protected_live - Root sched for paths that only run while live
+ *
+ * scx_root is published before the scheduler goes live and cleared only after
+ * it is fully drained, so a path that only executes while the scheduler is live
+ * can never race an update. Return the root sched with a plain load, never
+ * %NULL.
+ */
+static inline struct scx_sched *scx_root_protected_live(void)
+{
+	return rcu_dereference_protected(scx_root, true);
+}
+
+/**
+ * scx_root_protected - Root sched for contexts that exclude its updates
+ *
+ * Both scx_root updates run under the locks checked below, so holding one
+ * excludes them. Return the root sched with a plain load, %NULL if no scheduler
+ * is loaded.
+ */
+static inline struct scx_sched *scx_root_protected(void)
+{
+	return rcu_dereference_protected(scx_root,
+					 lockdep_is_cpus_held() ||
+					 lockdep_is_held(&scx_enable_mutex));
+}
+
 static inline struct scx_dispatch_q *scx_bypass_dsq(struct scx_sched *sch, s32 cpu)
 {
 	return &per_cpu_ptr(sch->pcpu, cpu)->bypass_dsq;
diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
index 2a8c979c7976..824fe35f00ee 100644
--- a/kernel/sched/ext/sub.c
+++ b/kernel/sched/ext/sub.c
@@ -477,7 +477,7 @@ static void queue_sync_ecaps(struct scx_sched *sch, s32 cid)
 	if (llist_on_list(&pcpu->ecaps_to_sync_node))
 		return;
 	if (llist_add(&pcpu->ecaps_to_sync_node, &cpu_rq(cpu)->scx.ecaps_to_sync))
-		scx_kick_cpu(scx_root, cpu, 0);
+		scx_kick_cpu(sch->ancestors[0], cpu, 0);
 }
 
 /* discard @rq's queued ecaps syncs */
@@ -638,7 +638,7 @@ void scx_unbypass_replay_ecaps(struct rq *rq, struct scx_sched *sch)
  */
 void scx_online_ecaps(struct rq *rq)
 {
-	struct scx_sched *pos;
+	struct scx_sched *root, *pos;
 	s32 cid, shard;
 
 	/*
@@ -652,14 +652,15 @@ void scx_online_ecaps(struct rq *rq)
 
 	guard(rq_lock_irqsave)(rq);
 
+	root = scx_root_protected();
 	cid = __scx_cpu_to_cid(cpu_of(rq));
 	shard = rcu_dereference_all(scx_cid_to_shard)[cid];
 
-	scx_for_each_descendant_pre(pos, scx_root) {
+	scx_for_each_descendant_pre(pos, root) {
 		struct scx_pshard *ps;
 
 		/* root holds every cap and never uses ecaps */
-		if (pos == scx_root)
+		if (!pos->level)
 			continue;
 
 		ps = pos->pshard[shard];
@@ -679,13 +680,15 @@ void scx_online_ecaps(struct rq *rq)
 void scx_offline_ecaps(struct rq *rq)
 {
 	s32 cpu = cpu_of(rq);
-	struct scx_sched *pos;
+	struct scx_sched *root, *pos;
 
 	guard(rq_lock_irqsave)(rq);
 
-	scx_for_each_descendant_pre(pos, scx_root) {
+	root = scx_root_protected();
+
+	scx_for_each_descendant_pre(pos, root) {
 		/* root holds every cap and never uses ecaps */
-		if (pos == scx_root)
+		if (!pos->level)
 			continue;
 
 		WRITE_ONCE(per_cpu_ptr(pos->pcpu, cpu)->ecaps, 0);
-- 
2.55.0

[PATCH 3/3] sched_ext: Use rcu_access_pointer() for the first_task comparison

[Tejun Heo]

dsq->first_task is __rcu for the lockless scx_bpf_dsq_peek(). The task
removal path compares it against the departing task with a plain load, which
sparse flags. The comparison runs under the dsq lock and only tests
identity, so rcu_access_pointer() is the fit.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/sched/ext/ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 95aadce7b104..53e4aaac46b7 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -1642,7 +1642,7 @@ static void task_unlink_from_dsq(struct task_struct *p,
 	list_del_init(&p->scx.dsq_list.node);
 	dsq_dec_nr(dsq, p);
 
-	if (!(dsq->id & SCX_DSQ_FLAG_BUILTIN) && dsq->first_task == p) {
+	if (!(dsq->id & SCX_DSQ_FLAG_BUILTIN) && rcu_access_pointer(dsq->first_task) == p) {
 		struct task_struct *first_task;
 
 		first_task = nldsq_next_task(dsq, NULL, false);
-- 
2.55.0