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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181108095239.GO9761@hirez.programming.kicks-ass.net>
Date:   Thu, 8 Nov 2018 10:52:39 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Muchun Song <smuchun@...il.com>
Cc:     Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
        rostedt@...dmis.org
Subject: Re: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for
 comparing RT task prority

On Thu, Nov 08, 2018 at 10:15:49AM +0800, Muchun Song wrote:
> Peter Zijlstra <peterz@...radead.org> 于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.