2.5: MAX_PRIO cleanup

PATCH In review Robert Love 2 participants 10 messages Apr 23, 2002

lore

Share & tools

lore

Robert Love

author

2002-04-23 02:23 UTC #0 lore raw

PATCH 1/1 2.5: MAX_PRIO cleanup

Linus,

Attached patch replaces occurrences of the magic numbers representing maximum priority / maximum RT priority with the (already defined and used) MAX_PRIO and MAX_RT_PRIO defines. The patch also contains some comment additions/changes (particularly to address the double_rq_lock ambiguity). Very simple.

This is the invariant cleanup portion of another patch I am working on:
compile-time configurable maximum RT priorities. But whether you buy that or not, this is a sane cleanup.

Patch is against 2.5.9, super-duper-please apply. Thank you,

Robert Love

kernel/sched.c +28 ~~-15~~

diff -urN linux-2.5.9/kernel/sched.c linux/kernel/sched.c
@@ -24,17 +24,18 @@
 #include <linux/kernel_stat.h>
 
 /*
- * Priority of a process goes from 0 to 139. The 0-99
- * priority range is allocated to RT tasks, the 100-139
- * range is for SCHED_OTHER tasks. Priority values are
- * inverted: lower p->prio value means higher priority.
+ * Priority of a process goes from 0 to MAX_PRIO-1.  The
+ * 0 to MAX_RT_PRIO-1 priority range is allocated to RT tasks,
+ * the MAX_RT_PRIO to MAX_PRIO range is for SCHED_OTHER tasks.
+ * Priority values are inverted: lower p->prio value means higher
+ * priority.
  */
 #define MAX_RT_PRIO		100
 #define MAX_PRIO		(MAX_RT_PRIO + 40)
 
 /*
  * Convert user-nice values [ -20 ... 0 ... 19 ]
- * to static priority [ 100 ... 139 (MAX_PRIO-1) ],
+ * to static priority [ MAX_RT_PRIO..MAX_PRIO-1 ],
  * and back.
  */
 #define NICE_TO_PRIO(nice)	(MAX_RT_PRIO + (nice) + 20)

@@ -1071,7 +1072,7 @@
  */
 int task_prio(task_t *p)
 {
-	return p->prio - 100;
+	return p->prio - MAX_RT_PRIO;
 }
 
 int task_nice(task_t *p)

@@ -1131,13 +1132,13 @@
 				policy != SCHED_OTHER)
 			goto out_unlock;
 	}
-	
+
 	/*
-	 * Valid priorities for SCHED_FIFO and SCHED_RR are 1..99, valid
-	 * priority for SCHED_OTHER is 0.
+	 * Valid priorities for SCHED_FIFO and SCHED_RR are 1..MAX_RT_PRIO-1,
+	 * valid priority for SCHED_OTHER is 0.
 	 */
 	retval = -EINVAL;
-	if (lp.sched_priority < 0 || lp.sched_priority > 99)
+	if (lp.sched_priority < 0 || lp.sched_priority > MAX_RT_PRIO - 1)
 		goto out_unlock;
 	if ((policy == SCHED_OTHER) != (lp.sched_priority == 0))
 		goto out_unlock;

@@ -1157,7 +1158,7 @@
 	p->policy = policy;
 	p->rt_priority = lp.sched_priority;
 	if (policy != SCHED_OTHER)
-		p->prio = 99 - p->rt_priority;
+		p->prio = (MAX_RT_PRIO - 1) - p->rt_priority;
 	else
 		p->prio = p->static_prio;
 	if (array)

@@ -1237,7 +1238,7 @@
 /**
  * sys_sched_setaffinity - set the cpu affinity of a process
  * @pid: pid of the process
- * @len: length of the bitmask pointed to by user_mask_ptr
+ * @len: length in bytes of the bitmask pointed to by user_mask_ptr
  * @user_mask_ptr: user-space pointer to the new cpu mask
  */
 asmlinkage int sys_sched_setaffinity(pid_t pid, unsigned int len,

@@ -1289,7 +1290,7 @@
 /**
  * sys_sched_getaffinity - get the cpu affinity of a process
  * @pid: pid of the process
- * @len: length of the bitmask pointed to by user_mask_ptr
+ * @len: length in bytes of the bitmask pointed to by user_mask_ptr
  * @user_mask_ptr: user-space pointer to hold the current cpu mask
  */
 asmlinkage int sys_sched_getaffinity(pid_t pid, unsigned int len,

@@ -1371,7 +1372,7 @@
 	switch (policy) {
 	case SCHED_FIFO:
 	case SCHED_RR:
-		ret = 99;
+		ret = MAX_RT_PRIO - 1;
 		break;
 	case SCHED_OTHER:
 		ret = 0;

@@ -1511,6 +1512,12 @@
 	read_unlock(&tasklist_lock);
 }
 
+/*
+ * double_rq_lock - safely lock two runqueues
+ *
+ * Note this does not disable interrupts like task_rq_lock,
+ * you need to do so manually before calling.
+ */
 static inline void double_rq_lock(runqueue_t *rq1, runqueue_t *rq2)
 {
 	if (rq1 == rq2)

@@ -1526,6 +1533,12 @@
 	}
 }
 
+/*
+ * double_rq_unlock - safely unlock two runqueues
+ *
+ * Note this does not restore interrupts like task_rq_unlock,
+ * you need to do so manually after calling.
+ */
 static inline void double_rq_unlock(runqueue_t *rq1, runqueue_t *rq2)
 {
 	spin_unlock(&rq1->lock);

@@ -1675,7 +1688,7 @@
 static int migration_thread(void * bind_cpu)
 {
 	int cpu = cpu_logical_map((int) (long) bind_cpu);
-	struct sched_param param = { sched_priority: 99 };
+	struct sched_param param = { sched_priority: MAX_RT_PRIO - 1 };
 	runqueue_t *rq;
 	int ret;

Ingo Molnar

2002-04-23 09:57 UTC #1 lore raw

Quoted 5 lines · Robert Love

Attached patch replaces occurrences of the magic numbers representing maximum priority / maximum RT priority with the (already defined and used) MAX_PRIO and MAX_RT_PRIO defines. The patch also contains some comment additions/changes (particularly to address the double_rq_lock ambiguity). Very simple.

i agree that this area needs cleaning up, but i dont agree with all aspects of your patch. I intentionally left the user-space API side separate, MAX_RT can in fact be higher than 100 (without changing the user-space API), the only rule is that it must not be smaller. We in fact had such a situation once. It's a perfectly valid goal to have 'super high prio' kernel-space threads in the future that have in fact some priority that cannot be reached by user-space threads.

so i've re-done a variation of your patch, which defines USER_MAX_RT_PRIO, so the user-space API can still stay separate from the kernel-internal representation.

i've also done some other changes:

Quoted 10 lines

/*
- * Priority of a process goes from 0 to 139. The 0-99
- * priority range is allocated to RT tasks, the 100-139
- * range is for SCHED_OTHER tasks. Priority values are
- * inverted: lower p->prio value means higher priority.
+ * Priority of a process goes from 0 to MAX_PRIO-1. The
+ * 0 to MAX_RT_PRIO-1 priority range is allocated to RT tasks,
+ * the MAX_RT_PRIO to MAX_PRIO range is for SCHED_OTHER tasks.
+ * Priority values are inverted: lower p->prio value means higher
+ * priority.

this i dont agree with either. The point of comments is easy understanding, so i intentionally kept the 'hard' constants and i'm updating them constantly - it's much easier to understand how things happen if it does not happen via a define. The code itself i agree should stay abstract, but the comments should stay as humanly readable as possible.

(the set|get_affinity comment fixes i kept, plus the runqueue double-lock/unlock comments as well, see the attached patch.)

Ingo

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.544 -> 1.545
# kernel/sched.c 1.71 -> 1.72
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/04/23 [email protected] 1.545
# - introduce MAX_USER_RT_PRIO
# - comment fixes from rml
# --------------------------------------------
#

kernel/sched.c +22 -7

diff -Nru a/kernel/sched.c b/kernel/sched.c
@@ -28,8 +28,11 @@
  * priority range is allocated to RT tasks, the 100-139
  * range is for SCHED_OTHER tasks. Priority values are
  * inverted: lower p->prio value means higher priority.
+ * 
+ * NOTE: MAX_RT_PRIO must not be smaller than MAX_USER_RT_PRIO.
  */
 #define MAX_RT_PRIO		100
+#define MAX_USER_RT_PRIO	100
 #define MAX_PRIO		(MAX_RT_PRIO + 40)
 
 /*

@@ -1071,7 +1074,7 @@
  */
 int task_prio(task_t *p)
 {
-	return p->prio - 100;
+	return p->prio - MAX_USER_RT_PRIO;
 }
 
 int task_nice(task_t *p)

@@ -1137,7 +1140,7 @@
 	 * priority for SCHED_OTHER is 0.
 	 */
 	retval = -EINVAL;
-	if (lp.sched_priority < 0 || lp.sched_priority > 99)
+	if (lp.sched_priority < 0 || lp.sched_priority > MAX_USER_RT_PRIO-1)
 		goto out_unlock;
 	if ((policy == SCHED_OTHER) != (lp.sched_priority == 0))
 		goto out_unlock;

@@ -1157,7 +1160,7 @@
 	p->policy = policy;
 	p->rt_priority = lp.sched_priority;
 	if (policy != SCHED_OTHER)
-		p->prio = 99 - p->rt_priority;
+		p->prio = MAX_USER_RT_PRIO-1 - p->rt_priority;
 	else
 		p->prio = p->static_prio;
 	if (array)

@@ -1237,7 +1240,7 @@
 /**
  * sys_sched_setaffinity - set the cpu affinity of a process
  * @pid: pid of the process
- * @len: length of the bitmask pointed to by user_mask_ptr
+ * @len: length in bytes of the bitmask pointed to by user_mask_ptr
  * @user_mask_ptr: user-space pointer to the new cpu mask
  */
 asmlinkage int sys_sched_setaffinity(pid_t pid, unsigned int len,

@@ -1289,7 +1292,7 @@
 /**
  * sys_sched_getaffinity - get the cpu affinity of a process
  * @pid: pid of the process
- * @len: length of the bitmask pointed to by user_mask_ptr
+ * @len: length in bytes of the bitmask pointed to by user_mask_ptr
  * @user_mask_ptr: user-space pointer to hold the current cpu mask
  */
 asmlinkage int sys_sched_getaffinity(pid_t pid, unsigned int len,

@@ -1371,7 +1374,7 @@
 	switch (policy) {
 	case SCHED_FIFO:
 	case SCHED_RR:
-		ret = 99;
+		ret = MAX_USER_RT_PRIO-1;
 		break;
 	case SCHED_OTHER:
 		ret = 0;

@@ -1511,6 +1514,12 @@
 	read_unlock(&tasklist_lock);
 }
 
+/*
+ * double_rq_lock - safely lock two runqueues
+ *
+ * Note this does not disable interrupts like task_rq_lock,
+ * you need to do so manually before calling.
+ */
 static inline void double_rq_lock(runqueue_t *rq1, runqueue_t *rq2)
 {
 	if (rq1 == rq2)

@@ -1526,6 +1535,12 @@
 	}
 }
 
+/*
+ * double_rq_unlock - safely unlock two runqueues
+ *
+ * Note this does not restore interrupts like task_rq_unlock,
+ * you need to do so manually after calling.
+ */
 static inline void double_rq_unlock(runqueue_t *rq1, runqueue_t *rq2)
 {
 	spin_unlock(&rq1->lock);

@@ -1675,7 +1690,7 @@
 static int migration_thread(void * bind_cpu)
 {
 	int cpu = cpu_logical_map((int) (long) bind_cpu);
-	struct sched_param param = { sched_priority: 99 };
+	struct sched_param param = { sched_priority: MAX_RT_PRIO-1 };
 	runqueue_t *rq;
 	int ret;

Robert Love

author

2002-04-23 16:53 UTC #2 lore raw

Quoted 11 lines · Ingo Molnar

i agree that this area needs cleaning up, but i dont agree with all aspects of your patch. I intentionally left the user-space API side separate, MAX_RT can in fact be higher than 100 (without changing the user-space API), the only rule is that it must not be smaller. We in fact had such a situation once. It's a perfectly valid goal to have 'super high prio' kernel-space threads in the future that have in fact some priority that cannot be reached by user-space threads.
so i've re-done a variation of your patch, which defines USER_MAX_RT_PRIO, so the user-space API can still stay separate from the kernel-internal representation.

This is better. I did not want to add another define or a new policy (i.e. user != kernel maximum priority) but doing so is valid. Actually, I think there are a lot of kernel threads where we probably want to set a priority above the max user-space priority.

There are circumstances in user programming where we want a larger maximum RT priority, too. In serious RT programming it wouldn't be uncommon to see 100-1000 priority levels. I think having such a wide range is partly to make programming easier (i.e. a lame crutch) but it does help to more readily layer real-time tasks.

Now the hard part is abstracting sched_find_first_set for an arbitrary MAX_RT_PRIO.

Quoted 13 lines

i've also done some other changes:
Quoted 10 lines
/*
- * Priority of a process goes from 0 to 139. The 0-99
- * priority range is allocated to RT tasks, the 100-139
- * range is for SCHED_OTHER tasks. Priority values are
- * inverted: lower p->prio value means higher priority.
+ * Priority of a process goes from 0 to MAX_PRIO-1. The
+ * 0 to MAX_RT_PRIO-1 priority range is allocated to RT tasks,
+ * the MAX_RT_PRIO to MAX_PRIO range is for SCHED_OTHER tasks.
+ * Priority values are inverted: lower p->prio value means higher
+ * priority.

Quoted 6 lines

this i dont agree with either. The point of comments is easy understanding, so i intentionally kept the 'hard' constants and i'm updating them constantly - it's much easier to understand how things happen if it does not happen via a define. The code itself i agree should stay abstract, but the comments should stay as humanly readable as possible.

Whatever you prefer...

Quoted 2 lines

(the set|get_affinity comment fixes i kept, plus the runqueue double-lock/unlock comments as well, see the attached patch.)

Great, thank you.

Linus, Ingo's patch is fine by me. Apply?

Robert Love

Ingo Molnar

2002-04-23 17:27 UTC #3 lore raw

Quoted 2 lines · Robert Love

Now the hard part is abstracting sched_find_first_set for an arbitrary MAX_RT_PRIO.

i'd suggest the following: keep the current hand-optimized one for the bitrange it's good for, and use the find_bit variant for all other values. (We had this before, check out some of the older versions of the O(1) scheduler.)

Ingo

Robert Love

author

2002-04-23 18:15 UTC #4 lore raw

Quoted 9 lines · Ingo Molnar

Quoted 2 lines · Robert Love
Now the hard part is abstracting sched_find_first_set for an arbitrary MAX_RT_PRIO.
i'd suggest the following: keep the current hand-optimized one for the bitrange it's good for, and use the find_bit variant for all other values. (We had this before, check out some of the older versions of the O(1) scheduler.)

In earlier releases we have

include/asm-i386/mmu_context.h :: sched_find_first_zero_bit

but that seems to operate on a 168-bit bitmap only. Around what time did we have a routine that operated on arbitrary maps? What name/file?

Thanks,

Robert Love

Ingo Molnar

2002-04-23 18:17 UTC #5 lore raw

Quoted 11 lines · Robert Love

Quoted 4 lines
i'd suggest the following: keep the current hand-optimized one for the bitrange it's good for, and use the find_bit variant for all other values. (We had this before, check out some of the older versions of the O(1) scheduler.)
In earlier releases we have
include/asm-i386/mmu_context.h :: sched_find_first_zero_bit
but that seems to operate on a 168-bit bitmap only. Around what time did we have a routine that operated on arbitrary maps? What name/file?

hm, perhaps it was only my own internal version then.

in any case, you can use the find_first_zero_bit() library function i added for exactly this purpose. Any problems with that? It wont be the fastest option, but it will do.

Ingo

Robert Love

author

2002-04-23 18:24 UTC #6 lore raw

Quoted 5 lines · Ingo Molnar

hm, perhaps it was only my own internal version then.
in any case, you can use the find_first_zero_bit() library function i added for exactly this purpose. Any problems with that? It wont be the fastest option, but it will do.

Err find_first_bit ?

I guess that will work but your hand-coded version is so much faster :)

I would need to abstract sched_find_first_bit into a generic function that also accepted a size argument. Something like:

Rename sched_find_first_bit to _sched_find_first_bit, and

#if MAX_RT_PRIO < 120 && MAX_RT_PRIO > 99
#define sched_find_first_bit(b, size) _sched_find_first_bit(b)
#else
#define sched_find_first_bit(b, size) find_first_bit(b, size)
#endif

We can let MAX_RT_PRIO go up 120 since the bitmap is actually 160, not 140, size 5*32=160.

Look good?

Robert Love

author

2002-04-23 22:44 UTC #7 lore raw

In reply to Ingo Molnar

Quoted 11 lines · Ingo Molnar

Quoted 10 lines
/*
- * Priority of a process goes from 0 to 139. The 0-99
- * priority range is allocated to RT tasks, the 100-139
- * range is for SCHED_OTHER tasks. Priority values are
- * inverted: lower p->prio value means higher priority.
+ * Priority of a process goes from 0 to MAX_PRIO-1. The
+ * 0 to MAX_RT_PRIO-1 priority range is allocated to RT tasks,
+ * the MAX_RT_PRIO to MAX_PRIO range is for SCHED_OTHER tasks.
+ * Priority values are inverted: lower p->prio value means higher
+ * priority.

Quoted 6 lines

this i dont agree with either. The point of comments is easy understanding, so i intentionally kept the 'hard' constants and i'm updating them constantly - it's much easier to understand how things happen if it does not happen via a define. The code itself i agree should stay abstract, but the comments should stay as humanly readable as possible.

Now that I am working on the configurable maximum RT value patch, I see why I did this: we can't hardcode the values like "0 to 99" because that 99 is set now via a compile-time define. Even if it defaults to 100, it can be a range of values so the comments should be specific and give the exact define.

That is why I did it in the invariant patch, anyhow - and I think it makes the most sense to do it in this patch.

Robert Love

Ingo Molnar

2002-04-23 22:48 UTC #8 lore raw

Quoted 5 lines · Robert Love

Now that I am working on the configurable maximum RT value patch, I see why I did this: we can't hardcode the values like "0 to 99" because that 99 is set now via a compile-time define. Even if it defaults to 100, it can be a range of values so the comments should be specific and give the exact define.

i'm not so sure whether we want to make the last step to make the maximum RT priority value to be a .config option. Sure we can keep things flexible so that it's just a matter of a single redefine, but do we really want users to be able to change the API of Linux in such a way?

the applications which need 1000 RT threads are i suspect specialized, and since it needs a kernel recompile anyway, it's not a big problem to change a single constant in the source code - we do that for many other things. I'd very much suggest we keep the 0-99 range for RT tasks, it's been well established and not making it a .config doesnt make it any harder for 1000 RT priority levels to be defined for specific applications.

Ingo

Robert Love

author

2002-04-23 23:03 UTC #9 lore raw

Quoted 11 lines · Ingo Molnar

i'm not so sure whether we want to make the last step to make the maximum RT priority value to be a .config option. Sure we can keep things flexible so that it's just a matter of a single redefine, but do we really want users to be able to change the API of Linux in such a way?
the applications which need 1000 RT threads are i suspect specialized, and since it needs a kernel recompile anyway, it's not a big problem to change a single constant in the source code - we do that for many other things. I'd very much suggest we keep the 0-99 range for RT tasks, it's been well established and not making it a .config doesnt make it any harder for 1000 RT priority levels to be defined for specific applications.

I understand your points - I am doing the patch for those that could benefit, I felt it is useful enough to be in the kernel. It is in fact the only real-time item missing from your scheduler - otherwise it is an ideal RT scheduler.

It is common for real-time schedules to either have very large RT priority ranges or allow customizing of these ranges.

I should note that while this changes the API, it should not break any existing applications (as long as the number only goes up). Existing scheduler calls will succeed. In fact, the only difference to existing programs would be sched_getparam returning a different maximum RT priority value.

Finally, it is not so simple as "just setting the define" because of sched_find_first_bit. I have a working patch that I will post soon - I did pretty much what I mentioned in an earlier email where if MAX_PRIO is outside of a certain range we fall back on using find_first_bit.

Would you be open to a patch to do the rest of the invariant work and the sched_find_first_bit switch, but not export a configure option? Or have I convinced you the configure option is not bad? :-)

The whole patch is not large either way.

Robert Love