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

David Vernet
2024-09-01T01:09:40Z

[-- 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 --]