niceness magic numbers, 2.4.20-pre11
PATCH 1/1 niceness magic numbers, 2.4.20-pre11
This untested patch removes use of process priority magic numbers in sys_nice, sys_setpriority, sys_getpriority, and properly uses their #define'd values.
It would be nice if someone tested if it applies against 2.5.
include/linux/resource.h +1 -1
@@ -43,7 +43,7 @@ };#define PRIO_MIN (-20) -#define PRIO_MAX 20 +#define PRIO_MAX 19#define PRIO_PROCESS 0#define PRIO_PGRP 1
kernel/sched.c +10 -8
@@ -870,17 +870,19 @@if (increment <0 ) {if (!capable (CAP_SYS_NICE ))return -EPERM ; -if (increment <-40 ) - increment =-40 ; + +/* +1 to account for 0 in min<-->max range */ +if (increment <PRIO_MIN - (PRIO_MAX +1 )) + increment =PRIO_MIN - (PRIO_MAX +1 ); } -if (increment >40 ) - increment =40 ; +if (increment > -(PRIO_MIN - (PRIO_MAX +1 ))) + increment = -(PRIO_MIN - (PRIO_MAX +1 )); newprio = current->nice + increment; -if (newprio <-20 ) - newprio =-20 ; -if (newprio >19 ) - newprio =19 ; +if (newprio <PRIO_MIN ) + newprio =PRIO_MIN ; +if (newprio >PRIO_MAX ) + newprio =PRIO_MAX ; current->nice = newprio;return 0 ; }
kernel/sys.c +6 -5
@@ -204,10 +204,10 @@/* normalize: avoid signed division (rounding problems) */ error = -ESRCH ; -if (niceval <-20 ) - niceval =-20 ; -if (niceval >19 ) - niceval =19 ; +if (niceval <PRIO_MIN ) + niceval =PRIO_MIN ; +if (niceval >PRIO_MAX ) + niceval =PRIO_MAX ;read_lock (&tasklist_lock);for_each_task (p) {
@@ -249,7 +249,8 @@long niceval;if (!proc_sel (p, which, who))continue ; - niceval =20 - p->nice; + niceval =PRIO_MAX +1 - p->nice;/* +1 to account for 0 +* in 0<-->max range */ if (niceval > retval) retval = niceval; }
On 22 Oct 2002, Kristis Makris wrote:
| This untested patch removes use of process priority magic numbers in | sys_nice, sys_setpriority, sys_getpriority, and properly uses their | #define'd values. | | It would be nice if someone tested if it applies against 2.5.
Nope, it doesn't apply cleanly to 2.5.44:
[rddunlap@midway linux-2544]$ patch -p1 -b --dry-run < ~/patches/niceness_magic_numbers.patch
patching file include/linux/resource.h
patching file kernel/sched.c
Hunk #1 FAILED at 870.
1 out of 1 hunk FAILED -- saving rejects to file kernel/sched.c.rej patching file kernel/sys.c
Hunk #1 succeeded at 236 with fuzz 1 (offset 32 lines). Hunk #2 FAILED at 281.
1 out of 2 hunks FAILED -- saving rejects to file kernel/sys.c.rej
Here is a working version for 2.5.44,
with a small change so that
-(A - (B + 1)) is written as B - A + 1
so go push it. :)
include/linux/resource.h renamed from include/linux/resource.h.nice +1 -1
@@ -43,7 +43,7 @@ };#define PRIO_MIN (-20) -#define PRIO_MAX 20 +#define PRIO_MAX 19#define PRIO_PROCESS 0#define PRIO_PGRP 1
kernel/sys.c renamed from kernel/sys.c.nice +7 -7
@@ -236,10 +236,10 @@/* normalize: avoid signed division (rounding problems) */ error = -ESRCH ; -if (niceval <-20 ) - niceval =-20 ; -if (niceval >19 ) - niceval =19 ; +if (niceval <PRIO_MIN ) + niceval =PRIO_MIN ; +if (niceval >PRIO_MAX ) + niceval =PRIO_MAX ;read_lock (&tasklist_lock);switch (which) {
@@ -301,7 +301,7 @@ who = current->pid; p =find_task_by_pid (who);if (p) { - niceval =20 -task_nice (p); + niceval =PRIO_MAX +1 -task_nice (p);if (niceval > retval) retval = niceval; }
@@ -310,7 +310,7 @@if (!who) who = current->pgrp;for_each_task_pid (who,PIDTYPE_PGID , p, l, pid) { - niceval =20 -task_nice (p); + niceval =PRIO_MAX +1 -task_nice (p);if (niceval > retval) retval = niceval; }
@@ -326,7 +326,7 @@ do_each_thread(g, p)if (p->uid == who) { - niceval =20 -task_nice (p); + niceval =PRIO_MAX +1 -task_nice (p);if (niceval > retval) retval = niceval; }
kernel/sched.c renamed from kernel/sched.c.nice +10 -8
@@ -1317,17 +1317,19 @@if (increment <0 ) {if (!capable (CAP_SYS_NICE ))return -EPERM ; -if (increment <-40 ) - increment =-40 ; + +/* +1 to account for 0 in min<-->max range */ +if (increment <PRIO_MIN -PRIO_MAX +1 ) + increment =PRIO_MIN -PRIO_MAX +1 ; } -if (increment >40 ) - increment =40 ; +if (increment > (PRIO_MAX -PRIO_MIN +1 )) + increment =PRIO_MAX -PRIO_MIN +1 ; nice =PRIO_TO_NICE (current->static_prio) + increment; -if (nice <-20 ) - nice =-20 ; -if (nice >19 ) - nice =19 ; +if (nice <PRIO_MIN ) + nice =PRIO_MIN ; +if (nice >PRIO_MAX ) + nice =PRIO_MAX ; retval = security_ops->task_setnice (current, nice);if (retval)
Signature · 1 line
~Randy
include/linux/resource.h line 46
Quoted 8 lines · Randy.Dunlap
include/linux/resource.h renamed from include/linux/resource.h.nice +1
-1@@ -43,7 +43,7 @@ };#define PRIO_MIN (-20) -#define PRIO_MAX 20 +#define PRIO_MAX 19
I suspect this isnt correct
Quoted 11 lines · Alan Cox
include/linux/resource.h line 46
Quoted 8 lines · Randy.Dunlap
include/linux/resource.h renamed from include/linux/resource.h.nice +1
-1@@ -43,7 +43,7 @@ };#define PRIO_MIN (-20) -#define PRIO_MAX 20 +#define PRIO_MAX 19I suspect this isnt correct
Agreed. Its not.
It should remain 20 and places that truly want 19 should do PRIO_MAX-1.
The idea of cleaning up the magic numbers is fine, otherwise..
Robert Love
Re: [PATCH] niceness magic numbers, 2.5.44 now.
On 23 Oct 2002, Robert Love wrote:
| On Wed, 2002-10-23 at 04:16, Alan Cox wrote: | | > On Wed, 2002-10-23 at 07:37, Randy.Dunlap wrote: | > > --- ./include/linux/resource.h.nice Tue Jun 18 20:10:36 2002 | > > +++ ./include/linux/resource.h Sat Oct 19 13:55:10 2002 | > > @@ -43,7 +43,7 @@ | > > }; | > > | > > #define PRIO_MIN (-20) | > > -#define PRIO_MAX 20 | > > +#define PRIO_MAX 19 | > | > I suspect this isnt correct | | Agreed. Its not. | | It should remain 20 and places that truly want 19 should do PRIO_MAX-1. | | The idea of cleaning up the magic numbers is fine, otherwise..
Thanks Alan and Robert.
Yes, I also prefer to remove magic numbers like these. Here's a corrected patch to 2.5.44 with an additional magic# instance removal. See any other problems with it?
Signature · 97 lines
~Randy
--- ./kernel/sys.c.nice Fri Oct 18 21:01:11 2002
+++ ./kernel/sys.c Wed Oct 23 16:20:54 2002
@@ -236,10 +236,10 @@
/* normalize: avoid signed division (rounding problems) */
error = -ESRCH;
- if (niceval < -20)
- niceval = -20;
- if (niceval > 19)
- niceval = 19;
+ if (niceval < PRIO_MIN)
+ niceval = PRIO_MIN;
+ if (niceval > PRIO_MAX - 1)
+ niceval = PRIO_MAX - 1;
read_lock(&tasklist_lock);
switch (which) {
@@ -301,7 +301,7 @@
who = current->pid;
p = find_task_by_pid(who);
if (p) {
- niceval = 20 - task_nice(p);
+ niceval = PRIO_MAX - task_nice(p);
if (niceval > retval)
retval = niceval;
}
@@ -310,7 +310,7 @@
if (!who)
who = current->pgrp;
for_each_task_pid(who, PIDTYPE_PGID, p, l, pid) {
- niceval = 20 - task_nice(p);
+ niceval = PRIO_MAX - task_nice(p);
if (niceval > retval)
retval = niceval;
}
@@ -326,7 +326,7 @@
do_each_thread(g, p)
if (p->uid == who) {
- niceval = 20 - task_nice(p);
+ niceval = PRIO_MAX - task_nice(p);
if (niceval > retval)
retval = niceval;
}
--- ./kernel/sched.c.nice Tue Oct 22 17:56:13 2002
+++ ./kernel/sched.c Wed Oct 23 16:28:01 2002
@@ -45,7 +45,7 @@
/*
* 'User priority' is the nice value converted to something we
* can work with better when scaling various scheduler parameters,
- * it's a [ 0 ... 39 ] range.
+ * it's in [ 0 ... 39 ] range.
*/
#define USER_PRIO(p) ((p)-MAX_RT_PRIO)
#define TASK_USER_PRIO(p) USER_PRIO((p)->static_prio)
@@ -1265,7 +1265,7 @@
prio_array_t *array;
runqueue_t *rq;
- if (TASK_NICE(p) == nice || nice < -20 || nice > 19)
+ if (TASK_NICE(p) == nice || nice < PRIO_MIN || nice > PRIO_MAX - 1)
return;
/*
* We have to be careful, if called from sys_setpriority(),
@@ -1317,17 +1317,18 @@
if (increment < 0) {
if (!capable(CAP_SYS_NICE))
return -EPERM;
- if (increment < -40)
- increment = -40;
+
+ if (increment < PRIO_MIN - PRIO_MAX)
+ increment = PRIO_MIN - PRIO_MAX;
}
- if (increment > 40)
- increment = 40;
+ if (increment > (PRIO_MAX - PRIO_MIN))
+ increment = PRIO_MAX - PRIO_MIN;
nice = PRIO_TO_NICE(current->static_prio) + increment;
- if (nice < -20)
- nice = -20;
- if (nice > 19)
- nice = 19;
+ if (nice < PRIO_MIN)
+ nice = PRIO_MIN;
+ if (nice > PRIO_MAX - 1)
+ nice = PRIO_MAX - 1;
retval = security_ops->task_setnice(current, nice);
if (retval)
###Re: [PATCH] niceness magic numbers, 2.5.44 now.
Quoted 4 lines · Randy.Dunlap
Thanks Alan and Robert.
Yes, I also prefer to remove magic numbers like these. Here's a corrected patch to 2.5.44 with an additional magic# instance removal. See any other problems with it?
Nope, looks good to me.
Robert Love
Quoted 8 lines
Quoted 4 lines
include/linux/resource.h line 46
Quoted 2 lines
include/linux/resource.h renamed from include/linux/resource.h.nice +1
-1-#define PRIO_MAX 20 +#define PRIO_MAX 19I suspect this isnt correct
Agreed. Its not.
It should remain 20 and places that truly want 19 should do PRIO_MAX-1.
Please excuse my inexperience; I don't understand why a 41 value range is defined instead, and faked as a 40. Is this merely for the convinience of ignoring 0 ? From the nice manpage:
"Range goes from -20 (highest priority) to 19 (lowest)."
Thanks,
Kristis
On Wed, 2002-10-23 at 23:22, Kristis Makris wrote:
excuse my inexperience; I don't understand why a 41 value range
Quoted 4 lines
is defined instead, and faked as a 40. Is this merely for the convinience of ignoring 0 ? From the nice manpage:
"Range goes from -20 (highest priority) to 19 (lowest)."
It is exported to user-space. Changing it will break things.
Robert Love