Re: [PATCH 6/6] sched_ext: Clear the built-in idle masks on CPU hotplug transitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] sched_ext: offline CPU re-sets idle mask bit after
  scx_idle_clear_cpu()
- [Medium] sched_ext: scx_idle_test_and_clear_cpu() claims inactive CPUs
  during hotplug
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] sched_ext: data race on shared idle_smts mask in
  scx_idle_test_and_clear_cpu() [1]
--

commit 0e2a7b76afc726a460e91d0e19897631e59a961f
Author: Tao Cui <cuitao@kylinos.cn>

sched_ext: Clear the built-in idle masks on CPU hotplug transitions

This commit clears a hotplugged CPU from the built-in idle masks in
handle_hotplug() and rejects offline CPUs in scx_idle_test_and_clear_cpu().

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 7d7e2a2fc5ae8..984d9224ab59c 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -4358,6 +4358,13 @@ static void handle_hotplug(struct rq *rq, bool online)
>  	if (scx_enabled())
>  		scx_idle_update_selcpu_topology(&sch->ops);
>  
> +	/*
> +	 * Keep the built-in idle masks in sync with the hotplug state: an
> +	 * offline CPU must stop being advertised as idle and a CPU coming
> +	 * back must not inherit a stale idle bit.
> +	 */
> +	scx_idle_clear_cpu(cpu);

[Severity: Medium]
Does the dying CPU re-set its bit in idle_cpus right after
scx_idle_clear_cpu() clears it?

During CPU teardown, handle_hotplug() runs at CPUHP_AP_ACTIVE, before
take_cpu_down() runs at CPUHP_TEARDOWN_CPU and parks migration/N.  When
migration/N parks and switches the dying CPU back to rq->idle so do_idle()
can halt it, set_next_task_idle() -> __scx_update_idle() calls
update_builtin_idle(cpu, true), which does not check whether the CPU or
runqueue is online and sets the bit in idle_cpus again.

Because normal tasks have their affinity restricted to online CPUs and BPF
schedulers typically pass task affinities to scx_bpf_pick_idle_cpu(), the
claim path in scx_idle_test_and_clear_cpu() is not reached for the offline
CPU.  Read-only consumers such as scx_bpf_get_idle_cpumask() and the
!cpumask_empty(idle_cpumask(waker_node)->cpu) check in scx_select_cpu_dfl()
therefore still see the offline CPU marked idle for the entire offline
window.  Gating idle=true updates in __scx_update_idle() or
update_builtin_idle() on the CPU/rq online state would prevent the dying
CPU from re-setting the bit.

> diff --git a/kernel/sched/ext/idle.c b/kernel/sched/ext/idle.c
> index 922fbc3065ccf..ff0c5402886e6 100644
> --- a/kernel/sched/ext/idle.c
> +++ b/kernel/sched/ext/idle.c
> @@ -107,6 +107,18 @@ static bool scx_idle_test_and_clear_cpu(int cpu)
>  			__cpumask_clear_cpu(cpu, idle_smts);
>  	}

[Severity: Medium]
This problem wasn't introduced by this patch, but nothing serializes
[1] concurrent updates to the shared idle_smts mask in
scx_idle_test_and_clear_cpu().  Both cpumask_andnot() and
__cpumask_clear_cpu() perform non-atomic read-modify-write operations on
the bitmap words:

CPU0                                CPU1
scx_idle_test_and_clear_cpu()       scx_idle_test_and_clear_cpu()
  cpumask_andnot()                    cpumask_andnot()
    val = *word;                        val = *word;
    val &= ~smt0;                       val &= ~smt1;
    *word = val;                        *word = val;  <-- CPU0 clear lost

A lost clear leaves a core that is no longer wholly idle marked in
idle_smts, causing SMT-aware idle selection to place tasks on busy sibling
threads instead of idle cores.

> +
> +	/*
> +	 * An offline CPU is never an idle candidate, but its bit can
> +	 * linger past @cpu going offline: the dying CPU keeps re-picking
> +	 * its idle task until it halts, which can happen after the
> +	 * hotplug paths have already run. Drop the stale bit and refuse
> +	 * the claim.
> +	 */
> +	if (unlikely(!cpu_online(cpu))) {
> +		cpumask_clear_cpu(cpu, idle_cpus);
> +		return false;
> +	}
> +
>  	return cpumask_test_and_clear_cpu(cpu, idle_cpus);
>  }

[Severity: Medium]
Can scx_idle_test_and_clear_cpu() still claim a CPU while it is inactive
during hotplug transitions?

On teardown, sched_cpu_deactivate() clears cpu_active(cpu) and
handle_hotplug() clears SCX_RQ_ONLINE at CPUHP_AP_ACTIVE, while
cpu_online(cpu) stays true until __cpu_disable() runs at
CPUHP_TEARDOWN_CPU.  Likewise on bringup, cpu_online(cpu) is set before the
CPU reaches CPUHP_AP_ACTIVE and sets cpu_active(cpu) and SCX_RQ_ONLINE.  If
cpuhp/N blocks during either window (for example in synchronize_rcu() or a
sleepable ops.cpu_offline() / ops.cpu_online() callback), switching to
rq->idle marks the CPU in idle_cpus, and scx_idle_test_and_clear_cpu() will
allow claiming it because cpu_online(cpu) is true.  select_task_rq() then
rejects the pick via !is_cpu_allowed() (which requires cpu_active(cpu) for
non-kthreads) and falls back with SCX_EV_SELECT_CPU_FALLBACK.  Checking
cpu_active(cpu) (or scx_rq_online(cpu_rq(cpu))) here would avoid claiming
CPUs that cannot accept tasks yet.

[1] https://sashiko.dev/#/bug/linux-ca6a3761-220e-4ce4-924a-d7356268acdd

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112130934.563321-1-cui.tao@linux.dev?part=6