Hi Peter, Thanks for your review.
Peter Zijlstra <[email protected]> 于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 <[email protected]> > > --- > > 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.
