[PATCH] niceness magic numbers, 2.4.20-pre11

[PATCH] niceness magic numbers, 2.4.20-pre11

[Kristis Makris]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

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.


[-- Attachment #2: niceness_magic_numbers.patch --]
[-- Type: text/x-patch, Size: 2060 bytes --]

diff -ur linux-2.4.20-pre11.orig/include/linux/resource.h linux/include/linux/resource.h
--- linux-2.4.20-pre11.orig/include/linux/resource.h	Tue Jun 18 20:10:36 2002
+++ linux/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
 
 #define	PRIO_PROCESS	0
 #define	PRIO_PGRP	1
diff -ur linux-2.4.20-pre11.orig/kernel/sched.c linux/kernel/sched.c
--- linux-2.4.20-pre11.orig/kernel/sched.c	Sat Oct 19 13:26:59 2002
+++ linux/kernel/sched.c	Sat Oct 19 13:41:19 2002
@@ -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;
 }
diff -ur linux-2.4.20-pre11.orig/kernel/sys.c linux/kernel/sys.c
--- linux-2.4.20-pre11.orig/kernel/sys.c	Sat Oct 19 11:58:48 2002
+++ linux/kernel/sys.c	Sat Oct 19 13:37:48 2002
@@ -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;
 	}

Re: [PATCH] niceness magic numbers, 2.4.20-pre11

[Randy.Dunlap]

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.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

 #define	PRIO_PROCESS	0
 #define	PRIO_PGRP	1
--- ./kernel/sys.c.nice	Fri Oct 18 21:01:11 2002
+++ ./kernel/sys.c	Tue Oct 22 22:57:49 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)
+		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.nice	Fri Oct 18 21:02:28 2002
+++ ./kernel/sched.c	Tue Oct 22 22:51:43 2002
@@ -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)

-- 
~Randy

Re: [PATCH] niceness magic numbers, 2.4.20-pre11

[Alan Cox]

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

Re: [PATCH] niceness magic numbers, 2.4.20-pre11

[Robert Love]

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..

	Robert Love

Re: [PATCH] niceness magic numbers, 2.5.44 now.

[Randy.Dunlap]

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?

-- 
~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.

[Robert Love]

On Wed, 2002-10-23 at 19:50, Randy.Dunlap wrote:

> 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

Re: [PATCH] niceness magic numbers, 2.4.20-pre11

[Kristis Makris]

> > > -#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.

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

Re: [PATCH] niceness magic numbers, 2.4.20-pre11

[Robert Love]

On Wed, 2002-10-23 at 23:22, Kristis Makris wrote:
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)."

It is exported to user-space.  Changing it will break things.

	Robert Love