* Re: [PATCH] kernel: a few readability changes for nice values
2008-10-17 3:50 [PATCH] kernel: a few readability changes for nice values Mike Steiner
@ 2008-10-19 1:00 ` Johannes Weiner
2008-10-20 16:42 ` Randy Dunlap
2008-10-20 16:57 ` Matthew Wilcox
2 siblings, 0 replies; 4+ messages in thread
From: Johannes Weiner @ 2008-10-19 1:00 UTC (permalink / raw)
To: kernel-janitors
Hi Mike,
"Mike Steiner" <mike65536@gmail.com> writes:
> From: Mike Steiner <mike65536@gmail.com>
newline -- see #15 in Documentation/SubmittingPatches
> Replaced raw nice values with constants and added macro to wrap
> range check with a descriptive name.
newline -- as above
> Signed-off-by: Mike Steiner <mike65536@gmail.com>
> ---
> FYI, this is my first kernel patch. I ran checkpatch.pl on it and fixed the
> errors, except for 2 concerning spaces around "<" and ">" because it would
> make the code less readable.
> diff -up linux-2.6.27.1/kernel/sched.c new/kernel/sched.c
> --- linux-2.6.27.1/kernel/sched.c 2008-10-15 16:02:53.000000000 -0700
> +++ new/kernel/sched.c 2008-10-16 20:37:27.000000000 -0700
> @@ -24,6 +24,7 @@
> * 2007-07-01 Group scheduling enhancements by Srivatsa Vaddagiri
> * 2007-11-29 RT balancing improvements by Steven Rostedt, Gregory Haskins,
> * Thomas Gleixner, Mike Kravetz
> + * 2008-10-16 A few readability changes for nice values by Mike Steiner
Changes are logged through git, no longer in the file headers.
> +#define MIN_NICE (-20)
> +#define MAX_NICE (19)
IMO, these values are traditionally so well known that the numbers
themselve have symbolic values :)
Best thing is probably to address people mostly working with the code
directly; you can use git to see who that would be. Then add them to
the CC:.
I suspect that smaller cleanups will be overlooked more easily during
the merge window. If you don't get a response, just resend it after
things have gotten a bit more calm again.
HTH,
Hannes
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kernel: a few readability changes for nice values
2008-10-17 3:50 [PATCH] kernel: a few readability changes for nice values Mike Steiner
2008-10-19 1:00 ` Johannes Weiner
@ 2008-10-20 16:42 ` Randy Dunlap
2008-10-20 16:57 ` Matthew Wilcox
2 siblings, 0 replies; 4+ messages in thread
From: Randy Dunlap @ 2008-10-20 16:42 UTC (permalink / raw)
To: kernel-janitors
On Thu, 16 Oct 2008 20:50:51 -0700 Mike Steiner wrote:
> From: Mike Steiner <mike65536@gmail.com>
> Replaced raw nice values with constants and added macro to wrap
> range check with a descriptive name.
> Signed-off-by: Mike Steiner <mike65536@gmail.com>
> ---
> FYI, this is my first kernel patch. I ran checkpatch.pl on it and fixed the
> errors, except for 2 concerning spaces around "<" and ">" because it would
> make the code less readable.
> diff -up linux-2.6.27.1/kernel/sched.c new/kernel/sched.c
> --- linux-2.6.27.1/kernel/sched.c 2008-10-15 16:02:53.000000000 -0700
> +++ new/kernel/sched.c 2008-10-16 20:37:27.000000000 -0700
> @@ -24,6 +24,7 @@
> * 2007-07-01 Group scheduling enhancements by Srivatsa Vaddagiri
> * 2007-11-29 RT balancing improvements by Steven Rostedt, Gregory Haskins,
> * Thomas Gleixner, Mike Kravetz
> + * 2008-10-16 A few readability changes for nice values by Mike Steiner
> */
>
> #include <linux/mm.h>
> @@ -77,13 +78,21 @@
>
> #include "sched_cpupri.h"
>
> +#define MIN_NICE (-20)
> +#define MAX_NICE (19)
Hm, I recall trying to do this about 5 years ago. Never went anywhere.
Maybe I wasn't persistent enough.
> +
> +/*
> + * If n is outside of range [a...z], use closest value in range.
> + */
> +#define CONSTRAIN_TO_RANGE(a, n, z) ((n)<(a) ? (a) : ((n)>(z) ? (z) : (n)))
As used (below), those macro args are not subject to side effects,
but they could be in general. (E.g., if the 'n' parameter was "nice++".)
There are at least 2 ways to fix that. The older way (in linux-kernel land)
is to use local variables in the macro (see include/linux/kernel.h, for the
abs(), clamp() [oh, you should probably use clamp() instead of adding a new
macro], min(), max(), etc., for how to do that.
The newer (and more preferred way currently) is to make constrain() an inline
function so that its parameters won't be evaluated more than one time (vs. the
macro, where the parameters could be evaluated more than one time if locals
are not used). As Andrew Morton says it, write in C, not in cpp (C preprocessor)
language. [paraphrasing]
> +
> /*
> * Convert user-nice values [ -20 ... 0 ... 19 ]
> * to static priority [ MAX_RT_PRIO..MAX_PRIO-1 ],
> * and back.
> */
> -#define NICE_TO_PRIO(nice) (MAX_RT_PRIO + (nice) + 20)
> -#define PRIO_TO_NICE(prio) ((prio) - MAX_RT_PRIO - 20)
> +#define NICE_TO_PRIO(nice) ((nice) - MIN_NICE + MAX_RT_PRIO)
> +#define PRIO_TO_NICE(prio) ((prio) - MAX_RT_PRIO + MIN_NICE)
> #define TASK_NICE(p) PRIO_TO_NICE((p)->static_prio)
>
> /*
> @@ -4877,7 +4886,7 @@ void set_user_nice(struct task_struct *p
> unsigned long flags;
> struct rq *rq;
>
> - if (TASK_NICE(p) = nice || nice < -20 || nice > 19)
> + if (TASK_NICE(p) = nice || nice < MIN_NICE || nice > MAX_NICE)
> return;
> /*
> * We have to be careful, if called from sys_setpriority(),
> @@ -4951,16 +4960,11 @@ asmlinkage long sys_nice(int increment)
> * We don't have to worry. Conceptually one call occurs first
> * and we have a single winner.
> */
> - if (increment < -40)
> - increment = -40;
> - if (increment > 40)
> - increment = 40;
> + increment = CONSTRAIN_TO_RANGE(-40, increment, 40);
>
> nice = PRIO_TO_NICE(current->static_prio) + increment;
> - if (nice < -20)
> - nice = -20;
> - if (nice > 19)
> - nice = 19;
> +
> + nice = CONSTRAIN_TO_RANGE(MIN_NICE, nice, MAX_NICE);
>
> if (increment < 0 && !can_nice(current, nice))
> return -EPERM;
> --
---
~Randy
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kernel: a few readability changes for nice values
2008-10-17 3:50 [PATCH] kernel: a few readability changes for nice values Mike Steiner
2008-10-19 1:00 ` Johannes Weiner
2008-10-20 16:42 ` Randy Dunlap
@ 2008-10-20 16:57 ` Matthew Wilcox
2 siblings, 0 replies; 4+ messages in thread
From: Matthew Wilcox @ 2008-10-20 16:57 UTC (permalink / raw)
To: kernel-janitors
On Thu, Oct 16, 2008 at 08:50:51PM -0700, Mike Steiner wrote:
> - if (increment < -40)
> - increment = -40;
> - if (increment > 40)
> - increment = 40;
> + increment = CONSTRAIN_TO_RANGE(-40, increment, 40);
I'm sorry, I just don't see this as a 'readability improvement'. I can
tell exactly what the original does; the second one looks complex.
Even writing it as:
increment = max(min(increment, 40), -40)
doesn't really improve it.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
^ permalink raw reply [flat|nested] 4+ messages in thread