Re: [PATCH] sched_ext: Use atomic cpumask_clear_cpu in scx_idle_test_and_clear_cpu()

[Andrea Righi]

Hi Michal,

On Tue, Sep 01, 2026 at 03:22:12PM +0000, Michal Blaszczyk wrote:
> In scx_idle_test_and_clear_cpu(), the shared idle_smts mask is modified
> locklessly by concurrent CPUs. Currently, the code uses
> __cpumask_clear_cpu() to clear a CPU from the mask. Because this is
> a non-atomic read-modify-write operation, concurrent modifications to
> different bits within the same memory word can lead to data races and
> lost updates.
> 
> Fix this by replacing __cpumask_clear_cpu() with the atomic
> cpumask_clear_cpu().

I think the atomic clear makes sense here, but, as sashiko also pointed out, it
does not fully address the race, because idle_smts is also modified by the
non-atomic cpumask_andnot() below and cpumask_or() in update_builtin_idle().

> 
> Fixes: 48849271e661 ("sched_ext: idle: Per-node idle cpumasks")

And the race existed way before this commit, the idle SMT tracking has been
always documented as racy and self-correcting.

This change may still be a best-effort improvement, but the commit message
should describe it in this way. Did you notice any improvements/benefits with
some workloads with this patch applied?

Thanks,
-Andrea

> Signed-off-by: Michal Blaszczyk <michalblk@xxxxxxxxxx>
> ---
>  kernel/sched/ext/idle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/ext/idle.c b/kernel/sched/ext/idle.c
> index d2973fb3af6d..8985b48c83a5 100644
> --- a/kernel/sched/ext/idle.c
> +++ b/kernel/sched/ext/idle.c
> @@ -104,7 +104,7 @@ static bool scx_idle_test_and_clear_cpu(int cpu)
>  		if (cpumask_intersects(smt, idle_smts))
>  			cpumask_andnot(idle_smts, idle_smts, smt);
>  		else if (cpumask_test_cpu(cpu, idle_smts))
> -			__cpumask_clear_cpu(cpu, idle_smts);
> +			cpumask_clear_cpu(cpu, idle_smts);
>  	}
>  
>  	return cpumask_test_and_clear_cpu(cpu, idle_cpus);
> -- 
> 2.55.0.897.gb25b4bd76c-goog
>