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

[David Vernet <void@manifault.com>]

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