linux-kernel - Re: [PATCH sched_ext/for-6.16] sched_ext: Call ops.update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aC7R80ADwPtJmNhu@gpd3>
Date: Thu, 22 May 2025 09:27:47 +0200
From: Andrea Righi <arighi@...dia.com>
To: Tejun Heo <tj@...nel.org>
Cc: David Vernet <void@...ifault.com>, Changwoo Min <changwoo@...lia.com>,
	linux-kernel@...r.kernel.org, sched-ext@...a.com
Subject: Re: [PATCH sched_ext/for-6.16] sched_ext: Call ops.update_idle()
 after updating builtin idle bits

On Wed, May 21, 2025 at 12:23:06PM -1000, Tejun Heo wrote:
> BPF schedulers that use both builtin CPU idle mechanism and
> ops.update_idle() may want to use the latter to create interlocking between
> ops.enqueue() and CPU idle transitions so that either ops.enqueue() sees the
> idle bit or ops.update_idle() sees the task queued somewhere. This can
> prevent race conditions where CPUs go idle while tasks are waiting in DSQs.
> 
> For such interlocking to work, ops.update_idle() must be called after
> builtin CPU masks are updated. Relocate the invocation. Currently, there are
> no ordering requirements on transitions from idle and this relocation isn't
> expected to make meaningful differences in that direction.
> 
> Signed-off-by: Tejun Heo <tj@...nel.org>

Looks good and it also makes sense semantically: potentially any action
performed in ops.update_idle() should be able to override the built-in idle
state, not the other way around.

For example, if we call scx_bpf_test_and_clear_cpu_idle(cpu) from within
ops.update_idle(), I would expect that to effectively "exclude" the CPU
from the idle selection, since the intention is to override the built-in
idle state. But that's not what it's happening if we update the idle
cpumasks after ops.update_idle(). With this patch applied, it works as
expected.

Maybe we should mention this aspect as well in the commit message,
something like this (feel free to rephrase/ignore):

  This also makes the ops.update_idle() behavior semantically consistent:
  any action performed in this callback should be able to override the
  builtin idle state, not the other way around.

In any case:

Reviewed-and-tested-by: Andrea Righi <arighi@...dia.com>

Thanks,
-Andrea

> ---
>  kernel/sched/ext_idle.c |   25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/sched/ext_idle.c b/kernel/sched/ext_idle.c
> index ae30de383913..66da03cc0b33 100644
> --- a/kernel/sched/ext_idle.c
> +++ b/kernel/sched/ext_idle.c
> @@ -738,16 +738,6 @@ void __scx_update_idle(struct rq *rq, bool idle, bool do_notify)
>  
>  	lockdep_assert_rq_held(rq);
>  
> -	/*
> -	 * Trigger ops.update_idle() only when transitioning from a task to
> -	 * the idle thread and vice versa.
> -	 *
> -	 * Idle transitions are indicated by do_notify being set to true,
> -	 * managed by put_prev_task_idle()/set_next_task_idle().
> -	 */
> -	if (SCX_HAS_OP(sch, update_idle) && do_notify && !scx_rq_bypassing(rq))
> -		SCX_CALL_OP(sch, SCX_KF_REST, update_idle, rq, cpu_of(rq), idle);
> -
>  	/*
>  	 * Update the idle masks:
>  	 * - for real idle transitions (do_notify == true)
> @@ -765,6 +755,21 @@ void __scx_update_idle(struct rq *rq, bool idle, bool do_notify)
>  	if (static_branch_likely(&scx_builtin_idle_enabled))
>  		if (do_notify || is_idle_task(rq->curr))
>  			update_builtin_idle(cpu, idle);
> +
> +	/*
> +	 * Trigger ops.update_idle() only when transitioning from a task to
> +	 * the idle thread and vice versa.
> +	 *
> +	 * Idle transitions are indicated by do_notify being set to true,
> +	 * managed by put_prev_task_idle()/set_next_task_idle().
> +	 *
> +	 * This must come after builtin idle update so that BPF schedulers can
> +	 * create interlocking between ops.update_idle() and ops.enqueue() -
> +	 * either enqueue() sees the idle bit or update_idle() sees the task
> +	 * that enqueue() queued.
> +	 */
> +	if (SCX_HAS_OP(sch, update_idle) && do_notify && !scx_rq_bypassing(rq))
> +		SCX_CALL_OP(sch, SCX_KF_REST, update_idle, rq, cpu_of(rq), idle);
>  }
>  
>  static void reset_idle_masks(struct sched_ext_ops *ops)
>