Re: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority

[Peter Zijlstra]

On Thu, Nov 08, 2018 at 10:15:49AM +0800, Muchun Song wrote:
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> ä2018å11æ8æåå äå1:31åéï

> > I think you only need the less thing, because:
> >
> > static inline bool prio_lower(int a, int b)
> > {
> >         return a > b;
> > }
> >
> > prio_higher(a,b) := prio_lower(b,a)
> > prio_higher_eq(a,b) := !prio_lower(a,b)
> > prio_lower_eq(a,b) := !prio_lower(b,a)
> 
> Yeah, it can be simpler here. Thanks for your advice.
> I will send a v2 patch which will fix it.
> 
> >
> > Now, I'm not sure if that actually improves readability if you go around
> > and directly substitute those identities instead of doing those defines.
> >
> 
> When I first read rt.c, I couldn't quickly realize which priority was higher
> in if condition. With this patch applied, if I know what's the meaning
> of prio_higher()
> or prio_lower() so that I can quickly know who's priority is higher.
> So I think that
> it can improves readability.

Ah, yes, I agree it improves readability; what I wondered was if instead
of doing:

@@ -1424,7 +1446,7 @@ select_task_rq_rt(struct task_struct *p, int cpu, int sd_flag, int flags)
 	 */
 	if (curr && unlikely(rt_task(curr)) &&
 	    (curr->nr_cpus_allowed < 2 ||
-	     curr->prio <= p->prio)) {
+	     prio_higher_eq(curr->prio, p->prio))) {
 		int target = find_lowest_rq(p);
 
 		/*
@@ -1432,7 +1454,7 @@ select_task_rq_rt(struct task_struct *p, int cpu, int sd_flag, int flags)
 		 * not running a lower priority task.
 		 */
 		if (target != -1 &&
-		    p->prio < cpu_rq(target)->rt.highest_prio.curr)
+		    prio_higher(p->prio, cpu_rq(target)->rt.highest_prio.curr))
 			cpu = target;
 	}
 	rcu_read_unlock();


Something like so might be better:


@@ -1424,7 +1446,7 @@ select_task_rq_rt(struct task_struct *p, int cpu, int sd_flag, int flags)
 	 */
 	if (curr && unlikely(rt_task(curr)) &&
 	    (curr->nr_cpus_allowed < 2 ||
-	     curr->prio <= p->prio)) {
+	     !prio_lower(curr->prio, p->prio))) {
 		int target = find_lowest_rq(p);
 
 		/*
@@ -1432,7 +1454,7 @@ select_task_rq_rt(struct task_struct *p, int cpu, int sd_flag, int flags)
 		 * not running a lower priority task.
 		 */
 		if (target != -1 &&
-		    p->prio < cpu_rq(target)->rt.highest_prio.curr)
+		    prio_lower(cpu_rq(target)->rt.highest_prio.curr, p->prio))
 			cpu = target;
 	}
 	rcu_read_unlock();

That is, always use prio_lower() and not introduce the other helpers.

I'm not sure; those identities are faily basic for me; but I can imagine
someone who's not yet read code for 30 odd years might struggle with
that a bit.