lkml.org 
[lkml]   [2018]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2018-11-08 10:52    [W:0.216 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site