[PATCHSET sched_ext/for-7.3-fixes] sched_ext: Fix bypass propagation and the bypass dispatch claim for overlapping bypasses

[PATCHSET sched_ext/for-7.3-fixes] sched_ext: Fix bypass propagation and the bypass dispatch claim for overlapping bypasses

[Tejun Heo]

Hello,

scx_bypass() propagates a bypass to the descendants, and claims bypass
dispatch on the host, only when the scheduler's own depth crosses zero. A
sub-scheduler's own bypass that overlaps an ancestor's then misses both: its
descendants drop out of bypass early, and its claim is leaked or never
taken. A PM suspend over a sub-scheduler's enable trips the WARN_ON_ONCE()
in enable_bypass_dsp() on the next bypass, and one over a disable leaves its
tasks on the parent's bypass DSQs with nobody consuming them.

Patch 1 propagates every depth change so that a descendant's depth is the
sum of its own and its ancestors' bypasses. Patch 2 claims and releases
bypass dispatch for the propagated descendants too.

Verified with a debug patch holding a sub-scheduler's enable bypass open
across PM suspend cycles: every suspend landed inside it, the unfixed kernel
trips the WARN on the following disable and the fixed one does not, with the
sub-scheduler's tasks running after each resume. The claim protocol across
the PM notifier, sub-scheduler enable and disable and root disable was
model-checked with TLC, including the sequences fixed here.

Based on sched_ext/for-7.3-fixes (069403dff337).

This patchset contains the following 2 patches.

 0001 sched_ext: Propagate every bypass depth change to the descendants
 0002 sched_ext: Fix the bypass dispatch claim for schedulers bypassed by an ancestor

The patchset is also available in the following git branch:

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext.git bypass-claim-fixes

diffstat follows. Thanks.

 kernel/sched/ext/ext.c | 54 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

--
tejun

[PATCH 1/2] sched_ext: Propagate every bypass depth change to the descendants

[Tejun Heo]

scx_bypass() propagates a depth change to the descendants only when the
scheduler's own depth crosses zero, so an own bypass that starts under an
ancestor's never reaches them. When the ancestor's bypass ends first, the
descendants leave bypass while the scheduler is still bypassing: a
sub-scheduler disabled under a PM suspend has its children re-queue their
tasks through their own BPF schedulers at resume, mid-teardown. If the own
bypass then ends, every descendant is decremented once more than incremented
and dec_bypass_depth() warns, though no current caller gets there.

Propagate on every call so that a descendant's depth is the sum of its own
bypasses and its ancestors'. The early return follows the walk, as a
descendant's depth is at least the scheduler's and nothing changes bypass
state unless the scheduler's own does.

Fixes: c7f0e467a27a ("sched_ext: Prepare bypass mode for hierarchical operation")
Cc: stable@vger.kernel.org # v7.1+
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/sched/ext/ext.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index a6a72ae7f815..3edb51bdd0ef 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -6216,22 +6216,23 @@ void scx_bypass(struct scx_sched *sch, bool bypass)
 	struct scx_sched *pos;
 	unsigned long flags;
 	int cpu;
+	bool changed;
 
 	raw_spin_lock_irqsave(&scx_bypass_lock, flags);
 
 	if (bypass) {
-		if (!inc_bypass_depth(sch))
-			goto unlock;
-
-		enable_bypass_dsp(sch);
+		changed = inc_bypass_depth(sch);
+		if (changed)
+			enable_bypass_dsp(sch);
 	} else {
-		if (!dec_bypass_depth(sch))
-			goto unlock;
+		changed = dec_bypass_depth(sch);
 	}
 
 	/*
 	 * Bypass state is propagated to all descendants - an scx_sched bypasses
-	 * if itself or any of its ancestors are in bypass mode.
+	 * if itself or any of its ancestors are in bypass mode. A descendant's
+	 * depth is the sum of its own bypasses and its ancestors', so every
+	 * call propagates whether or not @sch changed state.
 	 */
 	raw_spin_lock(&scx_sched_lock);
 	scx_for_each_descendant_pre(pos, sch) {
@@ -6244,6 +6245,13 @@ void scx_bypass(struct scx_sched *sch, bool bypass)
 	}
 	raw_spin_unlock(&scx_sched_lock);
 
+	/*
+	 * A descendant's depth is at least @sch's, so no bypass state changed
+	 * unless @sch's did.
+	 */
+	if (!changed)
+		goto unlock;
+
 	/*
 	 * No task property is changing. We just need to make sure all currently
 	 * queued tasks are re-queued according to the new scx_bypassing()
-- 
2.56.0

[PATCH 2/2] sched_ext: Fix the bypass dispatch claim for schedulers bypassed by an ancestor

[Tejun Heo]

scx_bypass() claims bypass dispatch when the scheduler's depth goes from
zero to one and releases it when the depth returns to zero, but propagates
only the depth to the descendants, not the claim. A descendant that starts
bypassing with its ancestor never claims, and one whose own bypass ends
under the ancestor's keeps its claim, as its depth does not return to zero.

A sub-scheduler enable whose bypass ends under a PM suspend thus leaks its
claim: the parent keeps hosting bypass dispatch and the sub-scheduler's next
bypass trips the WARN_ON_ONCE() on the claim. A sub-scheduler disable that
starts under a PM suspend has no claim, so once the resume lifts the root's
bypass its tasks sit on the parent's bypass DSQs with nobody consuming them.

Claim for every descendant whose depth goes from zero to one in the
propagation and release every scheduler in the subtree whose depth returned
to zero once the tasks have moved out of the bypass DSQs. A parent that is
bypassing itself when a descendant claims consumes only its own bypass DSQs,
empty until it leaves bypass and becomes the host.

Fixes: aa2a0a19686c ("sched_ext: Separate bypass dispatch enabling from bypass depth tracking")
Cc: stable@vger.kernel.org # v7.1+
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/sched/ext/ext.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 3edb51bdd0ef..7303c2c45964 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -6099,15 +6099,12 @@ static void enable_bypass_dsp(struct scx_sched *sch)
 
 	/*
 	 * When a sub-sched bypasses, its tasks are queued on the bypass DSQs of
-	 * the nearest non-bypassing ancestor or root. As enable_bypass_dsp() is
-	 * called iff @sch is not already bypassed due to an ancestor bypassing,
-	 * we can assume that the parent is not bypassing and thus will be the
-	 * host of the bypass DSQs.
-	 *
-	 * While the situation may change in the future, the following
-	 * guarantees that the nearest non-bypassing ancestor or root has bypass
-	 * dispatch enabled while a descendant is bypassing, which is all that's
-	 * required.
+	 * the nearest non-bypassing ancestor or root. The parent hosts them
+	 * unless it is bypassing too. Then they sit further up and the parent
+	 * consumes only its own bypass DSQs, empty until it leaves bypass and
+	 * becomes the host. Either way the nearest non-bypassing ancestor or
+	 * root has bypass dispatch enabled while a descendant is bypassing,
+	 * which is all that's required.
 	 *
 	 * scx_bypass_dsp_enabled() test is used to determine whether to enter
 	 * the bypass dispatch handling path from both bypassing and hosting
@@ -6232,16 +6229,19 @@ void scx_bypass(struct scx_sched *sch, bool bypass)
 	 * Bypass state is propagated to all descendants - an scx_sched bypasses
 	 * if itself or any of its ancestors are in bypass mode. A descendant's
 	 * depth is the sum of its own bypasses and its ancestors', so every
-	 * call propagates whether or not @sch changed state.
+	 * call propagates whether or not @sch changed state. The descendants'
+	 * claims are released below with @sch's, after their tasks have moved.
 	 */
 	raw_spin_lock(&scx_sched_lock);
 	scx_for_each_descendant_pre(pos, sch) {
 		if (pos == sch)
 			continue;
-		if (bypass)
-			inc_bypass_depth(pos);
-		else
+		if (bypass) {
+			if (inc_bypass_depth(pos))
+				enable_bypass_dsp(pos);
+		} else {
 			dec_bypass_depth(pos);
+		}
 	}
 	raw_spin_unlock(&scx_sched_lock);
 
@@ -6330,8 +6330,12 @@ void scx_bypass(struct scx_sched *sch, bool bypass)
 	}
 
 	/* disarming must come after moving all tasks out of the bypass DSQs */
-	if (!bypass)
-		scx_disable_bypass_dsp(sch);
+	if (!bypass) {
+		guard(raw_spinlock)(&scx_sched_lock);
+		scx_for_each_descendant_pre(pos, sch)
+			if (!pos->bypass_depth)
+				scx_disable_bypass_dsp(pos);
+	}
 unlock:
 	raw_spin_unlock_irqrestore(&scx_bypass_lock, flags);
 }
-- 
2.56.0