Re: [PATCH 13/18] sched_ext: Fix bypassed tasks stalling when the host lacks SCX_CAP_BASE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] sched_ext: missing slice end for NONE tasks in __scx_disable_and_exit_task()
- [High] sched_ext: misattributed rescue usage and stalls for bypassed tasks
--

commit 22b95f16249b92e8730c0989de8ef32cf7909110
Author: Tejun Heo <tj@kernel.org>

sched_ext: Fix bypassed tasks stalling when the host lacks SCX_CAP_BASE

This commit fixes bypassed tasks stalling on a CPU where their non-bypassing
host ancestor lacks SCX_CAP_BASE by resolving them against the host with
SCX_ENQ_RESCUE at enqueue time and keeping preempted rescuees on the local
DSQ during bypass.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 75f863de565a2..985a9cc24db56 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -388,8 +388,12 @@ static const struct sched_class *scx_setscheduler_class(struct task_struct *p)
>  	return __setscheduler_class(p->policy, p->prio);
>  }
>  
> -static struct scx_dispatch_q *bypass_enq_target_dsq(struct scx_sched *sch, s32 cpu)
> +static struct scx_dispatch_q *bypass_enq_target_dsq(struct scx_sched *sch, struct rq *rq,
> +						    struct task_struct *p, u64 *enq_flags)
>  {
> +	struct scx_sched *host = scx_exec_sched(sch, cpu_of(rq));
> +	struct scx_dispatch_q *dsq;
> +
>  	/*
>  	 * If @sch is a sub-sched which is bypassing, its tasks should go into
>  	 * the bypass DSQs of the nearest ancestor which is not bypassing. The
> @@ -400,8 +404,29 @@ static struct scx_dispatch_q *bypass_enq_target_dsq(struct scx_sched *sch, s32 c
>  	 * Whenever a sched starts bypassing, all runnable tasks in its subtree
>  	 * are re-enqueued after scx_bypassing() is turned on, guaranteeing that
>  	 * all tasks are transferred to the right DSQs.
> -	 */
> -	return scx_bypass_dsq(scx_exec_sched(sch, cpu), cpu);
> +	 *
> +	 * A cpu's dispatch only descends into a host holding %SCX_CAP_BASE on
> +	 * it, so the bypass DSQ of a host without BASE here is never consumed.
> +	 * Send @p to rescue on this cpu instead, resolving against @host.
> +	 * scx_dispatch_enqueue() resolves a local DSQ again, against the
> +	 * bypassing @sch. %SCX_ENQ_IGNORE_CAPS makes that second resolution
> +	 * admit @p.
> +	 *
> +	 * A migration-disabled task can't leave this cpu, and a task with a
> +	 * migration pending must run here until the migration moves it. Either
> +	 * would wait on the unconsumed bypass DSQ until the host regains BASE.
> +	 * Resolve them too when rescue is disabled. The resolution admits both
> +	 * to the local DSQ, as it does any insert of such a task.
> +	 */
> +	if (unlikely(scx_missing_caps(host, cpu_of(rq), SCX_CAP_BASE)) &&
> +	    (scx_rescue_enabled() || is_migration_disabled(p) || p->migration_pending)) {
> +		*enq_flags |= SCX_ENQ_RESCUE;
> +		dsq = scx_resolve_local_dsq(host, rq, p, enq_flags);
> +		*enq_flags |= SCX_ENQ_IGNORE_CAPS;
> +	} else {
> +		dsq = scx_bypass_dsq(host, cpu_of(rq));
> +	}
> +	return dsq;
>  }

[Severity: High]
When bypass_enq_target_dsq() resolves p against host via
scx_resolve_local_dsq(), p is placed on rq->scx.rescue.dsq or admitted to
rq->scx.local_dsq, whereas scx_task_sched(p) remains the bypassing child sch
(for instance during scx_sub_disable() where sch->exit_kind is already
SCX_EXIT_DONE, or after scx_fail_parent()).

In kernel/sched/ext/sub.c, scx_rescue_charge() charges the rescue runtime to
scx_task_sched(rq->scx.rescue.curr) rather than the executing host:

kernel/sched/ext/sub.c:scx_rescue_charge() {
	...
	/* per-cpu usage average feeds the overload victim pick */
	pcpu = per_cpu_ptr(scx_task_sched(rq->scx.rescue.curr)->pcpu,
			   cpu_of(rq));
	pcpu->rescue_avg = scx_rescue_decay_avg(pcpu) + delta_exec;
}

When rescue becomes overloaded, scx_rescue_check_overload() skips schedulers
whose exit_kind is not SCX_EXIT_NONE:

kernel/sched/ext/sub.c:scx_rescue_check_overload() {
	...
	list_for_each_entry_rcu(pos, &scx_sched_all, all) {
		u64 avg = scx_rescue_decay_avg(per_cpu_ptr(pos->pcpu, cpu));

		/* skip an already-exiting sub, else the ejection is wasted */
		if (pos->level && avg > max_avg &&
		    atomic_read(&pos->exit_kind) == SCX_EXIT_NONE) {
			max_avg = avg;
			victim = pos;
		}
	}
}

Because the runtime was credited to the already-exiting sch instead of host,
host has avg == 0 and is not selected, which can cause
scx_rescue_check_overload() to either eject an unrelated active sub-scheduler
on that CPU with a small non-zero rescue_avg or find no victim at all.

Similarly, while p waits on rq->scx.rescue.dsq or rq->scx.local_dsq,
dsq->sched is NULL (unlike scx_bypass_dsq(host, cpu), where dsq->sched is
host). In kernel/sched/ext/ext.c, check_rq_for_timeouts() only overrides
sch = scx_task_sched(p) when dsq->sched is non-NULL:

kernel/sched/ext/ext.c:check_rq_for_timeouts() {
	...
			if (dsq && dsq->sched && dsq->id != SCX_DSQ_LOCAL)
				sch = dsq->sched;

			__scx_exit(sch, SCX_EXIT_ERROR_STALL, 0, cpu_of(rq),
				   "%s[%d] failed to run for %u.%03us",
				   p->comm, p->pid, dur_ms / 1000,
				   dur_ms % 1000);
			timed_out = true;
			break;
}

If p stalls there while sch is already exiting, __scx_exit(sch, ...) becomes
a no-op because scx_claim_exit() fails on sch, yet check_rq_for_timeouts()
sets timed_out = true and stops scanning the rest of rq->scx.runnable_list
and the remaining CPUs in scx_watchdog_workfn(). If the stalled task holds a
lock that scx_sub_disable(sch) waits on (such as cgroup_mutex or
scx_fork_rwsem), could this prevent host from being ejected and cause
scx_sub_disable() to hang?

Should scx_rescue_charge() and check_rq_for_timeouts() attribute a bypassed
task's rescue usage and stall timeouts to scx_exec_sched(sch, cpu_of(rq))?

[ ... ]

> @@ -4984,11 +5014,14 @@ static void switched_from_scx(struct rq *rq, struct task_struct *p)
>  	 * parent's own teardown). There is nothing to disable; calling
>  	 * scx_disable_task() would WARN on the non-%ENABLED state and trigger a
>  	 * NONE -> READY validation failure.
> +	 *
> +	 * @p can still be a rescuee under the parent. End its slice as
> +	 * scx_disable_task() does for an enabled task.
>  	 */
>  	if (scx_get_task_state(p) == SCX_TASK_NONE)
> -		return;
> -
> -	scx_disable_task(scx_task_sched(p), p);
> +		scx_task_slice_ended(rq, p);
> +	else
> +		scx_disable_task(scx_task_sched(p), p);
>  }
>  
>  static void switched_to_scx(struct rq *rq, struct task_struct *p) {}

[Severity: High]
What happens if a running SCX_TASK_NONE rescuee leaves its bypassing parent
without changing scheduler class, such as via scx_sub_disable() ->
scx_rehome_task(), scx_cgroup_task_migrated() -> scx_rehome_task(), or
another scx_fail_parent() -> scx_punt_task()?

When scx_fail_parent() punts p to parent at SCX_TASK_NONE, it has already
called scx_bypass(parent, true). When scx_sub_disable(parent) later calls
scx_bypass(parent, true), inc_bypass_depth() sees parent->bypass_depth > 1
and does not walk the CPUs to call scx_task_slice_ended(rq, rq->donor).

Then scx_rehome_task() and scx_punt_task() run under scoped_guard(sched_change,
p, DEQUEUE_SAVE | DEQUEUE_MOVE | DEQUEUE_NOCLOCK) without DEQUEUE_CLASS, so
switched_from_scx() is not invoked. For a running donor with p->scx.slice > 0,
dequeue_task_scx() skips scx_task_slice_ended() due to DEQUEUE_SAVE:

kernel/sched/ext/ext.c:dequeue_task_scx() {
	...
	/* see scx_task_slice_ended() for the save/restore exception */
	if (!((deq_flags & DEQUEUE_SAVE) && task_current_donor(rq, p)))
		scx_task_slice_ended(rq, p);
}

put_prev_task_scx() also skips scx_task_slice_ended() because p->scx.slice is
non-zero, and __scx_disable_and_exit_task() returns early for SCX_TASK_NONE:

kernel/sched/ext/ext.c:__scx_disable_and_exit_task() {
	...
	switch (scx_get_task_state(p)) {
	case SCX_TASK_NONE:
		return;
	case SCX_TASK_INIT:
		args.cancelled = true;
		break;
	case SCX_TASK_READY:
		break;
	case SCX_TASK_ENABLED:
		scx_disable_task(sch, p);
		break;
}

When sched_change_end() restores p via enqueue_task_scx() with
ENQUEUE_RESTORE, it takes the ENQ_ACTION_LOCAL_NOREFILL path, leaving
rq->scx.rescue.curr == p and SCX_TASK_KERNEL_SLICE (and SCX_TASK_PROTECTED if
set) active under the new scheduler.

If p is re-homed to scx_root and all sub-schedulers finish disabling (turning
off __scx_has_subs via scx_dec_has_subs()), scx_rescuee(rq) begins returning
NULL:

kernel/sched/ext/sub.h:scx_rescuee() {
	lockdep_assert_rq_held(rq);

	if (!scx_has_subs())
		return NULL;

	return rq->scx.rescue.curr;
}

At that point scx_task_slice_ended() can no longer call scx_rescue_end(rq) to
clear rq->scx.rescue.curr, leaving a dangling task_struct pointer in
rq->scx.rescue.curr after p exits that scx_rescue_timerfn() continues to
dereference.

Should __scx_disable_and_exit_task() also call scx_task_slice_ended() and
scx_set_task_slice(p, 0) when p is in SCX_TASK_NONE?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027001749.1821379-1-tj@kernel.org?part=13