Messages in this thread Patches in this message | | | Date | Thu, 8 Nov 2018 10:52:39 +0100 | From | Peter Zijlstra <> | 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@infradead.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.
| |