Re: [PATCH 5/6] sched_ext: Report the ecaps loss when a cpu goes offline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched_ext: deadlock in scx_offline_ecaps() when scx_root_disable() races hotplug
--

commit 7354181ddf2a55286ce8abbd0374e5b9d7fdb269
Author: Tejun Heo <tj@kernel.org>

sched_ext: Report the ecaps loss when a cpu goes offline

This patch reports the loss of effective caps on a cid to the sub-scheduler
and its parent before the root's ops.cpu_offline() runs, and reports the
regain after ops.cpu_online(). To ensure the loss is reported before the CPU
goes offline, scx_offline_ecaps() marks a sync for each sub-scheduler, starts
the ext server, and waits for all marked syncs to be processed.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index ab9327b819304..ac86bd5ec21b7 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c
[ ... ]
> @@ -1298,40 +1332,69 @@ void scx_online_ecaps(struct rq *rq)
[ ... ]
>  void scx_offline_ecaps(struct rq *rq)
>  {
>  	s32 cpu = cpu_of(rq);
>  	struct scx_sched *root, *pos;
> +	u32 nr = 0;
> +	s32 cid;
>  
> -	guard(rq_lock_irqsave)(rq);
> +	/*
> +	 * ecaps and the tables need a live root, see scx_online_ecaps(). The
> +	 * ext class isn't picked without one, so the syncs wouldn't be
> +	 * processed and the wait would never end. scx_enabled() can't change
> +	 * during hotplug.
> +	 */
> +	if (scx_enabled()) {
> +		scoped_guard (rq_lock_irqsave, rq) {
> +			root = scx_root_protected();
> +			cid = __scx_cpu_to_cid(cpu);
>  
> -	root = scx_root_protected();
> +			scx_for_each_descendant_pre(pos, root) {
> +				struct scx_sched_pcpu *pcpu = per_cpu_ptr(pos->pcpu, cpu);
> +				struct scx_pshard *ps;
>  
> -	scx_for_each_descendant_pre(pos, root) {
> -		struct scx_sched_pcpu *pcpu = per_cpu_ptr(pos->pcpu, cpu);
> +				/* root holds every cap and never uses ecaps */
> +				if (!pos->level)
> +					continue;
>  
> -		/* root holds every cap and never uses ecaps */
> -		if (!pos->level)
> -			continue;
> +				/*
> +				 * A batch that has yet to reach the sync sees
> +				 * the mark. A sync already being processed has
> +				 * left the list and is queued again below.
> +				 */
> +				pcpu->flags |= SCX_SCHED_PCPU_ECAPS_OFFLINE;
> +				nr++;
> +
> +				ps = cid_pshard(pos, cid);
> +				guard(raw_spinlock)(&ps->lock);
> +				queue_sync_ecaps(pos, cid);
> +			}
>  
> -		WRITE_ONCE(pcpu->ecaps, 0);
> +			WRITE_ONCE(rq->scx.nr_ecaps_offline, nr);
> +			resched_sync_ecaps(rq);
> +		}
>  
> -		/* deliveries have stopped on this cpu, clear its mark */
> -		if (pcpu->flags & SCX_SCHED_PCPU_ECAPS_REPORT)
> -			ecaps_report_done(pcpu);
> +		wait_event(ecaps_offline_waitq, !READ_ONCE(rq->scx.nr_ecaps_offline));
>  	}

[Severity: High]
Can scx_offline_ecaps() deadlock here if a CPU hotplug offline operation
races with scx_sub_disable() and scx_root_disable()?

The comment notes that scx_enabled() cannot change during hotplug, and that
without a live root the ext class will not be picked and the wait would never
end. However, static_branch_disable(&__scx_enabled) in scx_root_disable() runs
before cpus_read_lock() is acquired.

In scx_sub_disable(), cpus_read_lock() is only held around clear_all_caps()
and ecaps_report(), and is dropped while sch remains linked on
parent->children until scx_unlink_sched(sch):

kernel/sched/ext/sub.c:scx_sub_disable() {
	scoped_guard(cpus_read_lock) {
		clear_all_caps(sch);
		if (sch->ecaps_enabled)
			ecaps_report(sch);
	}

	percpu_down_write(&scx_fork_rwsem);
	scx_cgroup_lock();
	...
	scx_unlink_sched(sch);
}

If the last sub-scheduler has passed that cpus_read_lock() block in
scx_sub_disable() when a CPU begins going offline (holding cpus_write_lock),
scx_offline_ecaps() sees scx_enabled() == true and visits sch via
scx_for_each_descendant_pre(pos, root). It sets SCX_SCHED_PCPU_ECAPS_OFFLINE,
sets rq->scx.nr_ecaps_offline = 1, starts rq->ext_server (which enters its
dl_defer zero-laxity wait if a fair task is running), and sleeps in
wait_event(ecaps_offline_waitq, ...).

Meanwhile, scx_sub_disable() does not take cpus_read_lock() again. It finishes
scx_unlink_sched(sch) and wakes scx_unlink_waitq. If the root scheduler is
also disabling, scx_root_disable() wakes from drain_descendants(), detaches
rq->ext_server, and disables __scx_enabled before blocking on
cpus_read_lock():

kernel/sched/ext/ext.c:scx_root_disable() {
	drain_descendants();
	...
	for_each_possible_cpu(cpu) {
		struct rq *rq = cpu_rq(cpu);

		scoped_guard(rq_lock_irqsave, rq) {
			update_rq_clock(rq);
			if (was_switched_all) {
				if (WARN_ON_ONCE(dl_server_swap_bw(&rq->ext_server,
								   &rq->fair_server)))
					pr_warn("failed to re-attach fair_server on CPU %d\n", cpu);
			} else {
				dl_server_detach_bw(&rq->ext_server);
			}
		}
	}

	scx_ops_cid_sched_updated_disable(sch);

	/* no task is on scx, turn off all the switches and flush in-progress calls */
	static_branch_disable(&__scx_enabled);
	...
	scoped_guard(cpus_read_lock) {
		...
	}
}

If the offline CPU had not yet picked ext_sched_class before __scx_enabled was
disabled and ext_server was detached (for instance, while a fair or RT task
was running during ext_server's dl_defer window), __scx_process_sync_ecaps()
will never be called on that rq. Furthermore, scx_discard_ecaps_to_sync() is
only invoked from scx_sched_quiesce_rcu_work() after scx_kobj_release() when
the sub-scheduler's bpf_link is closed, so while the sub-scheduler's bpf_link
remains open (or if userspace is blocked in bpf_scx_unreg(root) ->
scx_flush_disable_work(root)), wouldn't the hotplug thread and
scx_root_disable() wait on each other forever?

>  
> +	guard(rq_lock_irqsave)(rq);
>  	rq->scx.flags &= ~SCX_RQ_OPS_ONLINE;
>  }

[ ... ]

> @@ -1808,9 +1862,9 @@ void scx_sub_disable(struct scx_sched *sch)
>  	/*
>  	 * Clear @sch's caps and report the loss before any task goes back to
>  	 * the parent. A disabling sched is granted nothing, so the caps stay
> -	 * cleared. Hold off cpu hotplug across both. The report covers every
> -	 * active cpu and the clear drops the unreported loss on the rest, so
> -	 * that no loss reaches the parent after ops.sub_detach().
> +	 * cleared. Hold off cpu hotplug across both so that a cpu the report
> +	 * skips has finished going down. Its loss was reported as it went down,
> +	 * see scx_offline_ecaps().
>  	 *
>  	 * An enable that failed before its attach report has nothing to report.
>  	 * If it failed before scx_link_sched(), the root's disable doesn't wait

[ ... ]

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