Re: [PATCH 05/11] sched_ext: Restructure dispatch_to_local_dsq()

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]

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@kernel.org>

LGTM, just one comment below.

Acked-by: David Vernet <void@manifault.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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]