lkml.org 
[lkml]   [2018]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority
Hi Peter,

Thanks for your review.

Peter Zijlstra <peterz@infradead.org> 于2018年11月8日周四 上午1:31写道:
>
> On Thu, Nov 08, 2018 at 12:15:05AM +0800, Muchun Song wrote:
> > We use a value to represent the priority of the RT task. But a smaller
> > value corresponds to a higher priority. If there are two RT task A and B,
> > their priorities are prio_a and prio_b, respectively. If prio_a is larger
> > than prio_b, which means that the priority of RT task A is lower than RT
> > task B. It may seem a bit strange.
> >
> > In rt.c, there are many if condition of priority comparison. We need to
> > think carefully about which priority is higher because of this opposite
> > logic when read those code. So we introduce prio_{higher,lower}() helper
> > for comparing RT task prority, which can make code more readable.
> >
> > Signed-off-by: Muchun Song <smuchun@gmail.com>
> > ---
> > kernel/sched/rt.c | 68 ++++++++++++++++++++++++++++++++---------------
> > 1 file changed, 46 insertions(+), 22 deletions(-)
> >
> > diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> > index 9aa3287ce301..adf0f653c963 100644
> > --- a/kernel/sched/rt.c
> > +++ b/kernel/sched/rt.c
> > @@ -101,6 +101,28 @@ void init_rt_rq(struct rt_rq *rt_rq)
> > raw_spin_lock_init(&rt_rq->rt_runtime_lock);
> > }
> >
> > +/**
> > + * prio_higher(a, b) returns true if the priority a
> > + * is higher than the priority b.
> > + */
> > +static inline bool prio_higher(int a, int b)
> > +{
> > + return a < b;
> > +}
> > +
> > +#define prio_lower(a, b) prio_higher(b, a)
> > +
> > +/**
> > + * prio_higher_eq(a, b) returns true if the priority a
> > + * is higher than or equal to the priority b.
> > + */
> > +static inline bool prio_higher_eq(int a, int b)
> > +{
> > + return a <= b;
> > +}
> > +
> > +#define prio_lower_eq(a, b) prio_higher_eq(b, a)
>
> 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.

> The other option is of course to flip the in-kernel priority range the
> right way up, but that's a much more difficult patch and will terminally
> confuse people for a while.

Yeah, it is very difficult. There may be a lot of code than should be modified.
If we are not careful enough, there is a chance that something will be missed.

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