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

[Tejun Heo]

Hello,

On Sat, Aug 31, 2024 at 08:40:00PM -0500, David Vernet wrote:
...
> > @@ -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.

re. these arguments, there are largely two schemes - one coming from sched
core and shared with other scheds where the leading [this_]rq denotes the
current / local rq, and the other internal to SCX where more parties are
involved - this_rq, src_rq and dst_rq. While the latter situation may not be
unique to SCX, it is a lot more pronounced in SCX than other scheduling
classes as migrations are integral to how it works.

I'm not sure about applying the latter naming scheme to everything. Where
SCX is interfacing with sched core, I think there are more benefits in
following the existing naming scheme. This means that there are going to be
places where the two naming schemes interact, which is what this patch is
showing - consume*() functions are following the sched core scheme as
they're always used to pull tasks to the "current" rq for ops.balance() -
it's still doing what balances do in other sched classes. However, the
helpers that they use are more generic and flexible ones which are going to
be used for SCX specific things such as moving tasks between arbitrary DSQs.

That said, the use of @task_rq instead of @src_rq here is just confusing. I
will rename it to @src_rq.

Thanks.

-- 
tejun