Re: [PATCH 06/11] sched_ext: Reorder args for consume_local/remote_task()

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

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

On Fri, Aug 30, 2024 at 01:03:50AM -1000, Tejun Heo wrote:
> Reorder args for consistency in the order of:
>
> current_rq, p, src_[rq|dsq], dst_[rq|dsq].
>
> No functional changes intended.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
> kernel/sched/ext.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> index add267f31396..620cc0586c4b 100644
> --- a/kernel/sched/ext.c
> +++ b/kernel/sched/ext.c
> @@ -2139,8 +2139,8 @@ static void move_task_to_local_dsq(struct task_struct *p, u64 enq_flags,
>
> #endif /* CONFIG_SMP */
>
> -static void consume_local_task(struct rq *rq, struct scx_dispatch_q *dsq,
> - struct task_struct *p)
> +static void consume_local_task(struct task_struct *p,
> + struct scx_dispatch_q *dsq, struct rq *rq)
> {
> lockdep_assert_held(&dsq->lock); /* released on return */
>
> @@ -2249,8 +2249,8 @@ static bool unlink_dsq_and_lock_task_rq(struct task_struct *p,
> !WARN_ON_ONCE(task_rq != task_rq(p));
> }
>
> -static bool consume_remote_task(struct rq *this_rq, struct scx_dispatch_q *dsq,
> - struct task_struct *p, struct rq *task_rq)
> +static bool consume_remote_task(struct rq *this_rq, struct task_struct *p,
> + struct scx_dispatch_q *dsq, struct rq *task_rq)
> {
> raw_spin_rq_unlock(this_rq);
>
> @@ -2265,7 +2265,7 @@ static bool consume_remote_task(struct rq *this_rq, struct scx_dispatch_q *dsq,
> }
> #else /* CONFIG_SMP */
> static inline bool task_can_run_on_remote_rq(struct task_struct *p, struct rq *rq, bool trigger_error) { return false; }
> -static inline bool consume_remote_task(struct rq *rq, struct scx_dispatch_q *dsq, struct task_struct *p, struct rq *task_rq) { return false; }
> +static inline bool consume_remote_task(struct rq *this_rq, struct task_struct *p, struct scx_dispatch_q *dsq, struct rq *task_rq) { return false; }
> #endif /* CONFIG_SMP */
>
> static bool consume_dispatch_q(struct rq *rq, struct scx_dispatch_q *dsq)
> @@ -2286,12 +2286,12 @@ static bool consume_dispatch_q(struct rq *rq, struct scx_dispatch_q *dsq)
> struct rq *task_rq = task_rq(p);
>
> if (rq == task_rq) {
> - consume_local_task(rq, dsq, p);
> + consume_local_task(p, dsq, rq);
> return true;
> }
>
> if (task_can_run_on_remote_rq(p, rq, false)) {

How do you feel about always prefixing src_ and dst_ for any arguments
that refer to either (with any @rq before @p implying current as this
patch proposes)? In this case it's a bit confusing to read because
technically according to the convention proposed in this patch, @rq
could be either curr_rq or src_rq in consume_dispatch_q() (there's no
@p to disambiguate), and @rq could be either src_rq or dst_rq in
task_can_run_on_remote_rq() (they both come after @p).

It's pretty obvious from context that @rq is referring to a dst_rq in
task_can_run_on_remote_rq(), but it might still be a bit easier on the
eyes to be explicit. And for functions like consume_remote_task() which
take both a src_dsq and a src_rq, I think it will be easier to follow
then the convention.

Thanks,
David

> - if (likely(consume_remote_task(rq, dsq, p, task_rq)))
> + if (likely(consume_remote_task(rq, p, dsq, task_rq)))
> return true;
> goto retry;
> }
> --
> 2.46.0
>

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