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: <CAPSr9jGXPr-fcywjBTmGa5myeKSoGn+N3ybs9b2s2ayfU=sL2A@mail.gmail.com>
Date:   Thu, 8 Nov 2018 10:15:49 +0800
From:   Muchun Song <smuchun@...il.com>
To:     Peter Zijlstra <peterz@...radead.org>
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

Hi Peter,

Thanks for your review.

Peter Zijlstra <peterz@...radead.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@...il.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.