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]

Date:   Wed, 7 Nov 2018 18:31:40 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Muchun Song <smuchun@...il.com>
Cc:     mingo@...nel.org, linux-kernel@...r.kernel.org,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH] sched/rt: Introduce prio_{higher,lower}() helper for
 comparing RT task prority

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)

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.