[PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority

[PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority

[Muchun Song]

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)
+
 #ifdef CONFIG_RT_GROUP_SCHED
 static void destroy_rt_bandwidth(struct rt_bandwidth *rt_b)
 {
@@ -262,7 +284,7 @@ static void pull_rt_task(struct rq *this_rq);
 static inline bool need_pull_rt_task(struct rq *rq, struct task_struct *prev)
 {
 	/* Try to pull RT tasks here if we lower this rq's prio */
-	return rq->rt.highest_prio.curr > prev->prio;
+	return prio_lower(rq->rt.highest_prio.curr, prev->prio);
 }
 
 static inline int rt_overloaded(struct rq *rq)
@@ -377,7 +399,7 @@ static void enqueue_pushable_task(struct rq *rq, struct task_struct *p)
 	plist_add(&p->pushable_tasks, &rq->rt.pushable_tasks);
 
 	/* Update the highest prio pushable task */
-	if (p->prio < rq->rt.highest_prio.next)
+	if (prio_higher(p->prio, rq->rt.highest_prio.next))
 		rq->rt.highest_prio.next = p->prio;
 }
 
@@ -498,7 +520,7 @@ static void sched_rt_rq_enqueue(struct rt_rq *rt_rq)
 		else if (!on_rt_rq(rt_se))
 			enqueue_rt_entity(rt_se, 0);
 
-		if (rt_rq->highest_prio.curr < curr->prio)
+		if (prio_higher(rt_rq->highest_prio.curr, curr->prio))
 			resched_curr(rq);
 	}
 }
@@ -1044,7 +1066,7 @@ inc_rt_prio_smp(struct rt_rq *rt_rq, int prio, int prev_prio)
 	if (&rq->rt != rt_rq)
 		return;
 #endif
-	if (rq->online && prio < prev_prio)
+	if (rq->online && prio_higher(prio, prev_prio))
 		cpupri_set(&rq->rd->cpupri, rq->cpu, prio);
 }
 
@@ -1079,7 +1101,7 @@ inc_rt_prio(struct rt_rq *rt_rq, int prio)
 {
 	int prev_prio = rt_rq->highest_prio.curr;
 
-	if (prio < prev_prio)
+	if (prio_higher(prio, prev_prio))
 		rt_rq->highest_prio.curr = prio;
 
 	inc_rt_prio_smp(rt_rq, prio, prev_prio);
@@ -1092,7 +1114,7 @@ dec_rt_prio(struct rt_rq *rt_rq, int prio)
 
 	if (rt_rq->rt_nr_running) {
 
-		WARN_ON(prio < prev_prio);
+		WARN_ON(prio_higher(prio, prev_prio));
 
 		/*
 		 * This may have been our highest task, and therefore
@@ -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();
@@ -1475,7 +1497,7 @@ static void check_preempt_equal_prio(struct rq *rq, struct task_struct *p)
  */
 static void check_preempt_curr_rt(struct rq *rq, struct task_struct *p, int flags)
 {
-	if (p->prio < rq->curr->prio) {
+	if (prio_higher(p->prio, rq->curr->prio)) {
 		resched_curr(rq);
 		return;
 	}
@@ -1732,7 +1754,8 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
 
 		lowest_rq = cpu_rq(cpu);
 
-		if (lowest_rq->rt.highest_prio.curr <= task->prio) {
+		if (prio_higher_eq(lowest_rq->rt.highest_prio.curr,
+				   task->prio)) {
 			/*
 			 * Target rq has tasks of equal or higher priority,
 			 * retrying does not release any lock and is unlikely
@@ -1763,7 +1786,7 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
 		}
 
 		/* If this rq is still suitable use it. */
-		if (lowest_rq->rt.highest_prio.curr > task->prio)
+		if (prio_lower(lowest_rq->rt.highest_prio.curr, task->prio))
 			break;
 
 		/* try again */
@@ -1823,7 +1846,7 @@ static int push_rt_task(struct rq *rq)
 	 * higher priority than current. If that's the case
 	 * just reschedule current.
 	 */
-	if (unlikely(next_task->prio < rq->curr->prio)) {
+	if (unlikely(prio_higher(next_task->prio, rq->curr->prio))) {
 		resched_curr(rq);
 		return 0;
 	}
@@ -2100,8 +2123,8 @@ static void pull_rt_task(struct rq *this_rq)
 		 * logically higher, the src_rq will push this task away.
 		 * And if its going logically lower, we do not care
 		 */
-		if (src_rq->rt.highest_prio.next >=
-		    this_rq->rt.highest_prio.curr)
+		if (prio_lower_eq(src_rq->rt.highest_prio.next,
+				  this_rq->rt.highest_prio.curr))
 			continue;
 
 		/*
@@ -2121,7 +2144,7 @@ static void pull_rt_task(struct rq *this_rq)
 		 * Do we have an RT task that preempts
 		 * the to-be-scheduled task?
 		 */
-		if (p && (p->prio < this_rq->rt.highest_prio.curr)) {
+		if (p && prio_higher(p->prio, this_rq->rt.highest_prio.curr)) {
 			WARN_ON(p == src_rq->curr);
 			WARN_ON(!task_on_rq_queued(p));
 
@@ -2133,7 +2156,7 @@ static void pull_rt_task(struct rq *this_rq)
 			 * p if it is lower in priority than the
 			 * current task on the run queue
 			 */
-			if (p->prio < src_rq->curr->prio)
+			if (prio_higher(p->prio, src_rq->curr->prio))
 				goto skip;
 
 			resched = true;
@@ -2167,7 +2190,7 @@ static void task_woken_rt(struct rq *rq, struct task_struct *p)
 	    p->nr_cpus_allowed > 1 &&
 	    (dl_task(rq->curr) || rt_task(rq->curr)) &&
 	    (rq->curr->nr_cpus_allowed < 2 ||
-	     rq->curr->prio <= p->prio))
+	     prio_higher_eq(rq->curr->prio, p->prio)))
 		push_rt_tasks(rq);
 }
 
@@ -2242,7 +2265,8 @@ static void switched_to_rt(struct rq *rq, struct task_struct *p)
 		if (p->nr_cpus_allowed > 1 && rq->rt.overloaded)
 			rt_queue_push_tasks(rq);
 #endif /* CONFIG_SMP */
-		if (p->prio < rq->curr->prio && cpu_online(cpu_of(rq)))
+		if (prio_higher(p->prio, rq->curr->prio) &&
+		    cpu_online(cpu_of(rq)))
 			resched_curr(rq);
 	}
 }
@@ -2263,18 +2287,18 @@ prio_changed_rt(struct rq *rq, struct task_struct *p, int oldprio)
 		 * If our priority decreases while running, we
 		 * may need to pull tasks to this runqueue.
 		 */
-		if (oldprio < p->prio)
+		if (prio_higher(oldprio, p->prio))
 			rt_queue_pull_task(rq);
 
 		/*
 		 * If there's a higher priority task waiting to run
 		 * then reschedule.
 		 */
-		if (p->prio > rq->rt.highest_prio.curr)
+		if (prio_lower(p->prio, rq->rt.highest_prio.curr))
 			resched_curr(rq);
 #else
 		/* For UP simply resched on drop of prio */
-		if (oldprio < p->prio)
+		if (prio_higher(oldprio, p->prio))
 			resched_curr(rq);
 #endif /* CONFIG_SMP */
 	} else {
@@ -2283,7 +2307,7 @@ prio_changed_rt(struct rq *rq, struct task_struct *p, int oldprio)
 		 * greater than the current running task
 		 * then reschedule.
 		 */
-		if (p->prio < rq->curr->prio)
+		if (prio_higher(p->prio, rq->curr->prio))
 			resched_curr(rq);
 	}
 }
-- 
2.17.1

Re: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority

[Peter Zijlstra]

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)

Now, I'm not sure if that actually improves readability if you go around
and directly substitute those identities instead of doing those defines.

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.

Re: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority

[Muchun Song]

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.

Re: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority

[Peter Zijlstra]

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.

Re: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> I think you only need the less thing, because:
> 
> static inline bool prio_lower(int a, int b)
> {
> 	return a > b;
> }

I'd say that should be named rt_prio_lower(), even if it's local to 
sched/rt.c, right?

Thanks,

	Ingo

Re: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for comparing RT task prority

[Muchun Song]

Hi Peter,

Ingo Molnar <mingo@kernel.org> 于2018年11月12日周一 下午2:57写道：
>
>
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
> > I think you only need the less thing, because:
> >
> > static inline bool prio_lower(int a, int b)
> > {
> >       return a > b;
> > }
>
> I'd say that should be named rt_prio_lower(), even if it's local to
> sched/rt.c, right?
>

I agree with Ingo's point of view. Looks great. What is your opinion?
If you also agree. I will update the patch and send it.

Thanks.