[PATCH] sched: Rationalize sys_sched_rr_get_interval()

[PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Peter Williams]

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

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 <pwil3058@bigpond.net.au>

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

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





[-- Attachment #2: rationalize-sys_sched_rr_get_interval.patch --]
[-- Type: text/x-patch, Size: 5120 bytes --]

diff -r 3df82b0661ca include/linux/sched.h
--- a/include/linux/sched.h	Mon Sep 03 12:06:59 2007 +1000
+++ b/include/linux/sched.h	Mon Sep 03 12:06:59 2007 +1000
@@ -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 {
diff -r 3df82b0661ca kernel/sched.c
--- a/kernel/sched.c	Mon Sep 03 12:06:59 2007 +1000
+++ b/kernel/sched.c	Mon Sep 03 12:06:59 2007 +1000
@@ -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:
diff -r 3df82b0661ca kernel/sched_fair.c
--- a/kernel/sched_fair.c	Mon Sep 03 12:06:59 2007 +1000
+++ b/kernel/sched_fair.c	Mon Sep 03 12:06:59 2007 +1000
@@ -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
diff -r 3df82b0661ca kernel/sched_idletask.c
--- a/kernel/sched_idletask.c	Mon Sep 03 12:06:59 2007 +1000
+++ b/kernel/sched_idletask.c	Mon Sep 03 12:06:59 2007 +1000
@@ -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,
 };
diff -r 3df82b0661ca kernel/sched_rt.c
--- a/kernel/sched_rt.c	Mon Sep 03 12:06:59 2007 +1000
+++ b/kernel/sched_rt.c	Mon Sep 03 12:06:59 2007 +1000
@@ -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,
 };

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Ingo Molnar]

* Peter Williams <pwil3058@bigpond.net.au> wrote:

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

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Dmitry Adamushko]

On 11/10/2007, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Peter Williams <pwil3058@bigpond.net.au> wrote:
>
> > -#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.


-- 
Best regards,
Dmitry Adamushko

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Peter Williams]

Dmitry Adamushko wrote:
> On 11/10/2007, Ingo Molnar <mingo@elte.hu> wrote:
>> * Peter Williams <pwil3058@bigpond.net.au> wrote:
>>
>>> -#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.

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
-- 
Peter Williams                                   pwil3058@bigpond.net.au

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

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Jarek Poplawski]

On 12-10-2007 00:23, Peter Williams wrote:
...
> 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:

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

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Ingo Molnar]

* Peter Williams <pwil3058@bigpond.net.au> wrote:

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

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Peter Williams]

Jarek Poplawski wrote:
> On 12-10-2007 00:23, Peter Williams wrote:
> ...
>> 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:
> 
>> +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.

> 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
-- 
Peter Williams                                   pwil3058@bigpond.net.au

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

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Jarek Poplawski]

On 13-10-2007 03:29, Peter Williams wrote:
> Jarek Poplawski wrote:
>> On 12-10-2007 00:23, Peter Williams wrote:
>> ...
>>> 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:
>>
>>> +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.

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

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

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Peter Williams]

Jarek Poplawski wrote:
> On 13-10-2007 03:29, Peter Williams wrote:
>> Jarek Poplawski wrote:
>>> On 12-10-2007 00:23, Peter Williams wrote:
>>> ...
>>>> 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:
>>>
>>>> +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.
> 
> So, now all is clear: this is the misleading case!
> 
>>> 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.

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?

> 
> Sorry for too harsh words.

I didn't consider them harsh.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

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

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Jarek Poplawski]

On 16-10-2007 03:16, Peter Williams wrote:
...
> 
> 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...

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

Re: [PATCH] sched: Rationalize sys_sched_rr_get_interval()

[Peter Williams]

Jarek Poplawski wrote:
> On 16-10-2007 03:16, Peter Williams wrote:
> ...
>> 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 :-).

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

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

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

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