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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch attempts to fix a data race on the locklessly shared `idle_smts` mask by replacing `__cpumask_clear_cpu()` with the atomic `cpumask_clear_cpu()`. However, the fix is incomplete because adjacent non-atomic read-modify-write operations (`cpumask_andnot()` and `cpumask_or()`) are still performed on the exact same shared mask. Mixing atomic bitops and non-atomic whole-word operations on the same memory words allows the non-atomic operations to silently clobber and lose the atomic updates.
--

commit f5e98ee81f9e1114c10e89759d2151f581c39afa
Author: Michal Blaszczyk <michalblk@google.com>

sched_ext: Use atomic cpumask_clear_cpu in scx_idle_test_and_clear_cpu()

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().

> diff --git a/kernel/sched/ext/idle.c b/kernel/sched/ext/idle.c
> index 6f93cc32b650e..7be0b5d189cdc 100644
> --- a/kernel/sched/ext/idle.c
> +++ b/kernel/sched/ext/idle.c
> @@ -103,7 +103,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);

[Severity: High]
This is a pre-existing issue, but does the adjacent non-atomic operation defeat
the purpose of the patch's atomic fix?

If CPU A locklessly updates idle_smts using non-atomic bulk operations like
cpumask_andnot() here, or cpumask_or() in update_builtin_idle(), it reads the
whole memory word into a register.

If CPU B concurrently executes the atomic cpumask_clear_cpu() introduced by
this patch, CPU A will then write its register back to memory, silently
overwriting and discarding CPU B's atomic clear.

Could this lead to lost updates to the shared idle_smts mask, potentially
causing scx_pick_idle_cpu() to get caught in an infinite loop as mentioned in
the code comments?

>  	}
>  
>  	return cpumask_test_and_clear_cpu(cpu, idle_cpus);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901152212.1691696-1-michalblk@google.com?part=1