sched: Rationalize sys_sched_rr_get_interval()

PATCH In review Peter Williams 4 participants 11 messages Oct 11, 2007 – Oct 17, 2007 Scheduler

Share & tools

Peter Williams

author

2007-10-11 06:54 UTC #0 lore raw

PATCH 1/1 sched: Rationalize sys_sched_rr_get_interval()

At the moment, static_prio_timeslice() is only used in sys_sched_rr_get_interval() and only gives the correct result for SCHED_FIFO and SCHED_RR tasks as the time slice for normal tasks is unrelated to the values returned by static_prio_timeslice().

This patch addresses this problem and in the process moves all the code associated with static_prio_timeslice() to sched_rt.c which is the only place where it now has relevance.

Signed-off-by Peter Williams <[email protected]>

Peter

Signature · 4 lines

Peter Williams                                   [email protected]

"Learning, n. The kind of ignorance distinguishing the studious."
     -- Ambrose Bierce

include/linux/sched.h +1 -0

diff -r 3df82b0661ca include/linux/sched.h
@@ -878,6 +878,7 @@ struct sched_class {
 	void (*set_curr_task) (struct rq *rq);
 	void (*task_tick) (struct rq *rq, struct task_struct *p);
 	void (*task_new) (struct rq *rq, struct task_struct *p);
+	unsigned int (*default_timeslice) (struct task_struct *p);
 };
 
 struct load_weight {

kernel/sched.c +1 ~~-30~~

diff -r 3df82b0661ca kernel/sched.c
@@ -101,16 +101,6 @@ unsigned long long __attribute__((weak))
 #define NICE_0_LOAD		SCHED_LOAD_SCALE
 #define NICE_0_SHIFT		SCHED_LOAD_SHIFT
 
-/*
- * These are the 'tuning knobs' of the scheduler:
- *
- * Minimum timeslice is 5 msecs (or 1 jiffy, whichever is larger),
- * default timeslice is 100 msecs, maximum timeslice is 800 msecs.
- * Timeslices get refilled after they expire.
- */
-#define MIN_TIMESLICE		max(5 * HZ / 1000, 1)
-#define DEF_TIMESLICE		(100 * HZ / 1000)
-
 #ifdef CONFIG_SMP
 /*
  * Divide a load by a sched group cpu_power : (load / sg->__cpu_power)

@@ -131,24 +121,6 @@ static inline void sg_inc_cpu_power(stru
 	sg->reciprocal_cpu_power = reciprocal_value(sg->__cpu_power);
 }
 #endif
-
-#define SCALE_PRIO(x, prio) \
-	max(x * (MAX_PRIO - prio) / (MAX_USER_PRIO / 2), MIN_TIMESLICE)
-
-/*
- * static_prio_timeslice() scales user-nice values [ -20 ... 0 ... 19 ]
- * to time slice values: [800ms ... 100ms ... 5ms]
- */
-static unsigned int static_prio_timeslice(int static_prio)
-{
-	if (static_prio == NICE_TO_PRIO(19))
-		return 1;
-
-	if (static_prio < NICE_TO_PRIO(0))
-		return SCALE_PRIO(DEF_TIMESLICE * 4, static_prio);
-	else
-		return SCALE_PRIO(DEF_TIMESLICE, static_prio);
-}
 
 static inline int rt_policy(int policy)
 {

@@ -4784,8 +4756,7 @@ long sys_sched_rr_get_interval(pid_t pid
 	if (retval)
 		goto out_unlock;
 
-	jiffies_to_timespec(p->policy == SCHED_FIFO ?
-				0 : static_prio_timeslice(p->static_prio), &t);
+	jiffies_to_timespec(p->sched_class->default_timeslice(p), &t);
 	read_unlock(&tasklist_lock);
 	retval = copy_to_user(interval, &t, sizeof(t)) ? -EFAULT : 0;
 out_nounlock:

kernel/sched_fair.c +6 -0

diff -r 3df82b0661ca kernel/sched_fair.c
@@ -1159,6 +1159,11 @@ static void set_curr_task_fair(struct rq
 }
 #endif
 
+static unsigned int default_timeslice_fair(struct task_struct *p)
+{
+	return NS_TO_JIFFIES(sysctl_sched_min_granularity);
+}
+
 /*
  * All the scheduling class methods:
  */

@@ -1180,6 +1185,7 @@ struct sched_class fair_sched_class __re
 	.set_curr_task          = set_curr_task_fair,
 	.task_tick		= task_tick_fair,
 	.task_new		= task_new_fair,
+	.default_timeslice	= default_timeslice_fair,
 };
 
 #ifdef CONFIG_SCHED_DEBUG

kernel/sched_idletask.c +6 -0

diff -r 3df82b0661ca kernel/sched_idletask.c
@@ -59,6 +59,11 @@ static void task_tick_idle(struct rq *rq
 {
 }
 
+static unsigned int default_timeslice_idle(struct task_struct *p)
+{
+	return 0;
+}
+
 /*
  * Simple, special scheduling class for the per-CPU idle tasks:
  */

@@ -80,4 +85,5 @@ static struct sched_class idle_sched_cla
 
 	.task_tick		= task_tick_idle,
 	/* no .task_new for idle tasks */
+	.default_timeslice	= default_timeslice_idle,
 };

kernel/sched_rt.c +37 -0

diff -r 3df82b0661ca kernel/sched_rt.c
@@ -205,6 +205,34 @@ move_one_task_rt(struct rq *this_rq, int
 }
 #endif
 
+/*
+ * These are the 'tuning knobs' of the scheduler:
+ *
+ * Minimum timeslice is 5 msecs (or 1 jiffy, whichever is larger),
+ * default timeslice is 100 msecs, maximum timeslice is 800 msecs.
+ * Timeslices get refilled after they expire.
+ */
+#define MIN_TIMESLICE		max(5 * HZ / 1000, 1)
+#define DEF_TIMESLICE		(100 * HZ / 1000)
+
+#define SCALE_PRIO(x, prio) \
+	max(x * (MAX_PRIO - prio) / (MAX_USER_PRIO / 2), MIN_TIMESLICE)
+
+/*
+ * static_prio_timeslice() scales user-nice values [ -20 ... 0 ... 19 ]
+ * to time slice values: [800ms ... 100ms ... 5ms]
+ */
+static unsigned int static_prio_timeslice(int static_prio)
+{
+	if (static_prio == NICE_TO_PRIO(19))
+		return 1;
+
+	if (static_prio < NICE_TO_PRIO(0))
+		return SCALE_PRIO(DEF_TIMESLICE * 4, static_prio);
+	else
+		return SCALE_PRIO(DEF_TIMESLICE, static_prio);
+}
+
 static void task_tick_rt(struct rq *rq, struct task_struct *p)
 {
 	/*

@@ -229,6 +257,14 @@ static void task_tick_rt(struct rq *rq, 
 	}
 }
 
+static unsigned int default_timeslice_rt(struct task_struct *p)
+{
+	if (p->policy == SCHED_FIFO)
+		return 0;
+
+	return static_prio_timeslice(p->static_prio);
+}
+
 static struct sched_class rt_sched_class __read_mostly = {
 	.enqueue_task		= enqueue_task_rt,
 	.dequeue_task		= dequeue_task_rt,

@@ -245,4 +281,5 @@ static struct sched_class rt_sched_class
 #endif
 
 	.task_tick		= task_tick_rt,
+	.default_timeslice	= default_timeslice_rt,
 };

Ingo Molnar

2007-10-11 07:00 UTC #1 lore raw

kernel/sched.c unsigned long long __attribute__((weak)) · line 112

Quoted 2 lines · Peter Williams

kernel/sched.c +0 -2

-#define MIN_TIMESLICE		max(5 * HZ / 1000, 1)
-#define DEF_TIMESLICE		(100 * HZ / 1000)

hm, this got removed by Dmitry quite some time ago. Could you please do this patch against the sched-devel git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-sched-devel.git

thanks,

Ingo

Dmitry Adamushko

2007-10-11 07:44 UTC #2 lore raw

Quoted 8 lines · Ingo Molnar

kernel/sched.c unsigned long long __attribute__((weak)) · line 112
Quoted 2 lines · Peter Williams
kernel/sched.c +0 -2
-#define MIN_TIMESLICE                max(5 * HZ / 1000, 1)
-#define DEF_TIMESLICE                (100 * HZ / 1000)
hm, this got removed by Dmitry quite some time ago. Could you please do this patch against the sched-devel git tree:

here is the commit:
http://git.kernel.org/?p=linux/kernel/git/mingo/linux-2.6-sched-devel.git;a=commit;h=dd3fec36addd1bf76b05225b7e483378b80c3f9e

I had also considered introducing smth like sched_class::task_timeslice() but decided it was not worth it.

Signature · 2 lines

Best regards,
Dmitry Adamushko

Peter Williams

author

2007-10-11 22:24 UTC #3 lore raw

Quoted 8 lines · Dmitry Adamushko

Quoted 6 lines · Ingo Molnar
kernel/sched.c unsigned long long __attribute__((weak)) · line 112
Quoted 2 lines · Peter Williams
kernel/sched.c +0 -2
-#define MIN_TIMESLICE                max(5 * HZ / 1000, 1)
-#define DEF_TIMESLICE                (100 * HZ / 1000)
hm, this got removed by Dmitry quite some time ago. Could you please do this patch against the sched-devel git tree:

Quoted 5 lines

here is the commit:
http://git.kernel.org/?p=linux/kernel/git/mingo/linux-2.6-sched-devel.git;a=commit;h=dd3fec36addd1bf76b05225b7e483378b80c3f9e
I had also considered introducing smth like sched_class::task_timeslice() but decided it was not worth it.

The reason I was going that route was for modularity (which helps when adding plugsched patches). I'll submit a revised patch for consideration.

Peter

Signature · 4 lines

Peter Williams                                   [email protected]

"Learning, n. The kind of ignorance distinguishing the studious."
 -- Ambrose Bierce

Jarek Poplawski

2007-10-12 06:47 UTC #4 lore raw

On 12-10-2007 00:23, Peter Williams wrote:
...

Quoted 2 lines

The reason I was going that route was for modularity (which helps when adding plugsched patches). I'll submit a revised patch for consideration.

...

IMHO, it looks like modularity could suck here:

kernel/sched_fair.c static void set_curr_task_fair(struct rq · line 1165

Quoted 4 lines

kernel/sched_fair.c +4 -0

+static unsigned int default_timeslice_fair(struct task_struct *p)
+{
+	return NS_TO_JIFFIES(sysctl_sched_min_granularity);
+}

If it's needed for outside and sched_fair will use something else (to avoid double conversion) this could be misleading. Shouldn't this be kind of private and return something usable for the class mainly? Why anything else than sched_fair should care about this?

Regards,
Jarek P.

Peter Williams

author

2007-10-13 05:00 UTC #5 lore raw

Quoted 7 lines · Jarek Poplawski

On 12-10-2007 00:23, Peter Williams wrote:
...
Quoted 1 line
The reason I was going that route was for modularity (which helps when adding plugsched patches). I'll submit a revised patch for consideration.
...
IMHO, it looks like modularity could suck here:

Quoted 9 lines

kernel/sched_fair.c static void set_curr_task_fair(struct rq · line 1165
Quoted 4 lines
kernel/sched_fair.c +4 -0
+static unsigned int default_timeslice_fair(struct task_struct *p)
+{
+	return NS_TO_JIFFIES(sysctl_sched_min_granularity);
+}
If it's needed for outside and sched_fair will use something else (to avoid double conversion) this could be misleading. Shouldn't this be kind of private and return something usable for the class mainly?

This is supplying data for a system call not something for internal use by the class. As far as the sched_fair class is concerned this is just a (necessary - because it's need by a system call) diversion.

Quoted 1 line

Why anything else than sched_fair should care about this?

sched_fair doesn't care so if nothing else does why do we even have sys_sched_rr_get_interval()? Is this whole function an anachronism that can be expunged? I'm assuming that the reason it exists is that there are user space programs that use this system call. Am I correct in this assumption? Personally, I can't think of anything it would be useful for other than satisfying curiosity.

Peter

Signature · 4 lines

Peter Williams                                   [email protected]

"Learning, n. The kind of ignorance distinguishing the studious."
 -- Ambrose Bierce

Jarek Poplawski

2007-10-15 11:08 UTC #6 lore raw

Quoted 20 lines · Peter Williams

Quoted 9 lines · Jarek Poplawski
On 12-10-2007 00:23, Peter Williams wrote:
...
Quoted 3 lines
The reason I was going that route was for modularity (which helps when adding plugsched patches). I'll submit a revised patch for consideration.
...
IMHO, it looks like modularity could suck here:
Quoted 9 lines
kernel/sched_fair.c static void set_curr_task_fair(struct rq · line 1165
Quoted 4 lines
kernel/sched_fair.c +4 -0
+static unsigned int default_timeslice_fair(struct task_struct *p)
+{
+    return NS_TO_JIFFIES(sysctl_sched_min_granularity);
+}
If it's needed for outside and sched_fair will use something else (to avoid double conversion) this could be misleading. Shouldn't this be kind of private and return something usable for the class mainly?

Quoted 3 lines

This is supplying data for a system call not something for internal use by the class. As far as the sched_fair class is concerned this is just a (necessary - because it's need by a system call) diversion.

So, now all is clear: this is the misleading case!

Quoted 9 lines

Quoted 2 lines
Why anything else than sched_fair should care about this?
sched_fair doesn't care so if nothing else does why do we even have sys_sched_rr_get_interval()? Is this whole function an anachronism that can be expunged? I'm assuming that the reason it exists is that there are user space programs that use this system call. Am I correct in this assumption? Personally, I can't think of anything it would be useful for other than satisfying curiosity.

Since this is for some special aim (not default for most classes, at least not for sched_fair) I'd suggest to change names:
default_timeslice_fair() and .default_timeslice to something like eg.:
rr_timeslice_fair() and .rr_timeslice or rr_interval_fair() and .rr_interval (maybe with this "default" before_"rr_" if necessary).

On the other hand man (2) sched_rr_get_interval mentions that:
"The identified process should be running under the SCHED_RR scheduling policy".

Also this place seems to say about something simpler:
http://www.gnu.org/software/libc/manual/html_node/Basic-Scheduling-Functions.html

So, I still doubt sched_fair's "notion" of timeslices should be necessary here.

Sorry for too harsh words.

Thanks,
Jarek P.

Peter Williams

author

2007-10-16 05:11 UTC #7 lore raw

Quoted 34 lines · Jarek Poplawski

Quoted 16 lines · Peter Williams
Quoted 7 lines · Jarek Poplawski
On 12-10-2007 00:23, Peter Williams wrote:
...
Quoted 1 line
The reason I was going that route was for modularity (which helps when adding plugsched patches). I'll submit a revised patch for consideration.
...
IMHO, it looks like modularity could suck here:
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+static unsigned int default_timeslice_fair(struct task_struct *p)
+{
+    return NS_TO_JIFFIES(sysctl_sched_min_granularity);
+}
If it's needed for outside and sched_fair will use something else (to avoid double conversion) this could be misleading. Shouldn't this be kind of private and return something usable for the class mainly?
Quoted 1 line
This is supplying data for a system call not something for internal use by the class. As far as the sched_fair class is concerned this is just a (necessary - because it's need by a system call) diversion.
So, now all is clear: this is the misleading case!
Quoted 2 lines
Quoted 1 line
Why anything else than sched_fair should care about this?
sched_fair doesn't care so if nothing else does why do we even have sys_sched_rr_get_interval()? Is this whole function an anachronism that can be expunged? I'm assuming that the reason it exists is that there are user space programs that use this system call. Am I correct in this assumption? Personally, I can't think of anything it would be useful for other than satisfying curiosity.
Since this is for some special aim (not default for most classes, at least not for sched_fair) I'd suggest to change names:
default_timeslice_fair() and .default_timeslice to something like eg.:
rr_timeslice_fair() and .rr_timeslice or rr_interval_fair() and .rr_interval (maybe with this "default" before_"rr_" if necessary).
On the other hand man (2) sched_rr_get_interval mentions that:
"The identified process should be running under the SCHED_RR scheduling policy".

Quoted 5 lines

Also this place seems to say about something simpler:
http://www.gnu.org/software/libc/manual/html_node/Basic-Scheduling-Functions.html
So, I still doubt sched_fair's "notion" of timeslices should be necessary here.

As do I. Even more so now that you've shown me the man page for sched_rr_get_interval().

I'd suggest that we modify sched_rr_get_interval() to return -EINVAL (with *interval set to zero) if the target task is not SCHED_RR. That way we can save a lot of unnecessary code. I'll work on a patch. Unless you want to do it?

Quoted 2 lines

Sorry for too harsh words.

I didn't consider them harsh.

Peter

Signature · 4 lines

Peter Williams                                   [email protected]

"Learning, n. The kind of ignorance distinguishing the studious."
 -- Ambrose Bierce

Jarek Poplawski

2007-10-16 09:40 UTC #8 lore raw

On 16-10-2007 03:16, Peter Williams wrote:
...

Quoted 4 lines

I'd suggest that we modify sched_rr_get_interval() to return -EINVAL (with *interval set to zero) if the target task is not SCHED_RR. That way we can save a lot of unnecessary code. I'll work on a patch.

...

I like this idea! But, since this a system call maybe at least something like RFC would be nicer...

Quoted 4 lines

Quoted 2 lines
Sorry for too harsh words.
I didn't consider them harsh.

So, I can't be mistaken for a rapper yet? I'll work on it...

Cheers,
Jarek P.

PS: Peter, for some unknown reason I don't receive your messages. If you get back some errors from my side I'd be interested to see it (alternative: jarkao2 at gmail.com).

Peter Williams

author

2007-10-17 00:23 UTC #9 lore raw

Quoted 7 lines · Jarek Poplawski

On 16-10-2007 03:16, Peter Williams wrote:
...
Quoted 1 line
I'd suggest that we modify sched_rr_get_interval() to return -EINVAL (with *interval set to zero) if the target task is not SCHED_RR. That way we can save a lot of unnecessary code. I'll work on a patch.
...
I like this idea! But, since this a system call maybe at least something like RFC would be nicer...

We would be just modifying the code to meet that specification so a patch would be OK. Anyone who wants to comment will do so anyway :-).

Quoted 4 lines

Quoted 3 lines
Quoted 2 lines
Sorry for too harsh words.
I didn't consider them harsh.

Quoted 8 lines

So, I can't be mistaken for a rapper yet? I'll work on it...
Cheers,
Jarek P.
PS: Peter, for some unknown reason I don't receive your messages. If you get back some errors from my side I'd be interested to see it (alternative: jarkao2 at gmail.com).

I haven't seen any bounce notifications. I've added the qmail address as a CC.

Peter

Signature · 4 lines

Peter Williams                                   [email protected]

"Learning, n. The kind of ignorance distinguishing the studious."
 -- Ambrose Bierce

Ingo Molnar

2007-10-12 06:59 UTC #10 lore raw

In reply to Peter Williams

Quoted 6 lines · Peter Williams

Quoted 2 lines
I had also considered introducing smth like sched_class::task_timeslice() but decided it was not worth it.
The reason I was going that route was for modularity (which helps when adding plugsched patches). I'll submit a revised patch for consideration.

no strong feelings - since this isnt a performance-sensitive codepath we could certainly abstract it a bit more if it's convenient for hackability. Depends on how the patch looks like and how much code increase there is - but i'd not expect it to be anything controversial.

Ingo