linux-kernel - Re: [PATCH 05/11] sched_ext: Restructure dispatch_to_local

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: <20240901010936.GF70166@maniforge>
Date: Sat, 31 Aug 2024 20:09:36 -0500
From: David Vernet <void@...ifault.com>
To: Tejun Heo <tj@...nel.org>
Cc: kernel-team@...a.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/11] sched_ext: Restructure dispatch_to_local_dsq()

On Fri, Aug 30, 2024 at 01:03:49AM -1000, Tejun Heo wrote:
> Now that there's nothing left after the big if block, flip the if condition
> and unindent the body.
> 
> No functional changes intended.
> 
> Signed-off-by: Tejun Heo <tj@...nel.org>

LGTM, just one comment below.

Acked-by: David Vernet <void@...ifault.com>

> ---
>  kernel/sched/ext.c | 94 ++++++++++++++++++++++------------------------
>  1 file changed, 44 insertions(+), 50 deletions(-)
> 
> diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> index 34b4e63850c1..add267f31396 100644
> --- a/kernel/sched/ext.c
> +++ b/kernel/sched/ext.c
> @@ -2333,65 +2333,59 @@ static void dispatch_to_local_dsq(struct rq *rq, struct scx_dispatch_q *dst_dsq,
>  	}
>  
>  #ifdef CONFIG_SMP
> -	if (likely(task_can_run_on_remote_rq(p, dst_rq, true))) {
> -		/*
> -		 * @p is on a possibly remote @src_rq which we need to lock to
> -		 * move the task. If dequeue is in progress, it'd be locking
> -		 * @src_rq and waiting on DISPATCHING, so we can't grab @src_rq
> -		 * lock while holding DISPATCHING.
> -		 *
> -		 * As DISPATCHING guarantees that @p is wholly ours, we can
> -		 * pretend that we're moving from a DSQ and use the same
> -		 * mechanism - mark the task under transfer with holding_cpu,
> -		 * release DISPATCHING and then follow the same protocol. See
> -		 * unlink_dsq_and_lock_task_rq().
> -		 */
> -		p->scx.holding_cpu = raw_smp_processor_id();
> +	if (unlikely(!task_can_run_on_remote_rq(p, dst_rq, true))) {
> +		dispatch_enqueue(&scx_dsq_global, p, enq_flags | SCX_ENQ_CLEAR_OPSS);

Given that UP systems will always take the path above where rq == src_rq
&& rq == dst_rq, this isn't really a functional change, but technically
we are now moving the backup global DSQ dispatch_enqueue call into the
CONFIG_SMP block. Should we maybe add a BUG() in an #else block below?
It's UP so it's obviously not super critical, but it might help
readability? Feel free to ignore -- the fact that all the rest of the
logic is in a CONFIG_SMP block is pretty clear, and this arguably
already improves readability by moving an unreachable piece of code on
UP into the SMP block.

Thanks,
David

> +		return;
> +	}
>  

[... snip ...]

>  #endif	/* CONFIG_SMP */
> -
> -	dispatch_enqueue(&scx_dsq_global, p, enq_flags | SCX_ENQ_CLEAR_OPSS);
>  }
>  
>  /**
> -- 
> 2.46.0
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)