[PATCH] sched: leave sched_setscheduler earlier if possible.

[PATCH] sched: leave sched_setscheduler earlier if possible.

[Dario Faggioli]

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

sched_setscheduler (in sched.c) is called in order of changing the
scheduling policy and/or the real-time priority of a task. Thus,
if we find out that neither of those are actually being modified, it
is possible to return earlier and save the overhead of a full
deactivate+activate cycle of the task in question.

Beside that, if we have more than one SCHED_FIFO task with the same
priority on the same rq (which means they share the same priority queue)
having one of them changing its position in the priority queue because of
a sched_setscheduler (as it happens by means of the deactivate+activate)
that does not actually change the priority violates POSIX which states,
for SCHED_FIFO:

"If a thread whose policy or priority has been modified by
pthread_setschedprio() is a running thread or is runnable, the effect on
its position in the thread list depends on the direction of the
modification, as follows: a. <...> b. If the priority is unchanged, the
thread does not change position in the thread list. c. <...>"

(http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_08.html)

Signed-off-by: Dario Faggioli <raistlin@linux.it>
---
 kernel/sched.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index c5ae6bc..d73bbc5 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -4998,6 +4998,16 @@ recheck:
 		return -EINVAL;
 	}
 
+	/*
+	 * If not changing anything there's no need to proceed further
+	 */
+	if (unlikely(policy == p->policy && (!rt_policy(policy) ||
+	    param->sched_priority == p->rt_priority))) {
+			__task_rq_unlock(rq);
+			raw_spin_unlock_irqrestore(&p->pi_lock, flags);
+			return 0;
+	}
+
 #ifdef CONFIG_RT_GROUP_SCHED
 	if (user) {
 		/*
-- 
1.7.4.1

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- dario.faggioli@jabber.org


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] sched: leave sched_setscheduler earlier if possible.

[Steven Rostedt]

On Thu, 2011-03-24 at 14:00 +0100, Dario Faggioli wrote:
> sched_setscheduler (in sched.c) is called in order of changing the
> scheduling policy and/or the real-time priority of a task. Thus,
> if we find out that neither of those are actually being modified, it
> is possible to return earlier and save the overhead of a full
> deactivate+activate cycle of the task in question.
> 
> Beside that, if we have more than one SCHED_FIFO task with the same
> priority on the same rq (which means they share the same priority queue)
> having one of them changing its position in the priority queue because of
> a sched_setscheduler (as it happens by means of the deactivate+activate)
> that does not actually change the priority violates POSIX which states,
> for SCHED_FIFO:
> 
> "If a thread whose policy or priority has been modified by
> pthread_setschedprio() is a running thread or is runnable, the effect on
> its position in the thread list depends on the direction of the
> modification, as follows: a. <...> b. If the priority is unchanged, the
> thread does not change position in the thread list. c. <...>"
> 
> (http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_08.html)
> 
> Signed-off-by: Dario Faggioli <raistlin@linux.it>

Acked-by: Steven Rostedt <rostedt@goodmis.org>

> ---
>  kernel/sched.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index c5ae6bc..d73bbc5 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c


> @@ -4998,6 +4998,16 @@ recheck:

Peter, this is why I prefer:

 recheck:
over
recheck:

-- Steve


>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * If not changing anything there's no need to proceed further
> +	 */
> +	if (unlikely(policy == p->policy && (!rt_policy(policy) ||
> +	    param->sched_priority == p->rt_priority))) {
> +			__task_rq_unlock(rq);
> +			raw_spin_unlock_irqrestore(&p->pi_lock, flags);
> +			return 0;
> +	}
> +
>  #ifdef CONFIG_RT_GROUP_SCHED
>  	if (user) {
>  		/*
> -- 
> 1.7.4.1
> 

Re: [PATCH] sched: leave sched_setscheduler earlier if possible.

[Peter Zijlstra]

On Thu, 2011-03-24 at 10:21 -0400, Steven Rostedt wrote:
> > @@ -4998,6 +4998,16 @@ recheck:
> 
> Peter, this is why I prefer:
> 
>  recheck:
> over
> recheck:
> 
And you know I prefer to fix the tools ;-)

Re: [PATCH] sched: leave sched_setscheduler earlier if possible.

[Steven Rostedt]

On Fri, 2011-03-25 at 11:28 +0100, Peter Zijlstra wrote:
> On Thu, 2011-03-24 at 10:21 -0400, Steven Rostedt wrote:
> > > @@ -4998,6 +4998,16 @@ recheck:
> > 
> > Peter, this is why I prefer:
> > 
> >  recheck:
> > over
> > recheck:
> > 
> And you know I prefer to fix the tools ;-)

And when the tools are fixed, I'll also prefer

recheck:
over
 recheck:

;-)

-- Steve

Re: [PATCH] sched: leave sched_setscheduler earlier if possible.

[Peter Zijlstra]

On Fri, 2011-03-25 at 08:52 -0400, Steven Rostedt wrote:
> > And you know I prefer to fix the tools ;-)
> 
> And when the tools are fixed, I'll also prefer
> 
So instead of complaining about it, tell people to use:

QUILT_DIFF_OPTS="-F ^[[:alpha:]\$_].*[^:]\$"

Just like you tell them about all your other patch style issues.

Junio, know of any way to make git-diff do the same? The purpose is to
skip labels as functions so that people stop doing stupid crap like
indenting labels, eg.:

void foo(void)
{
 again:
	/* do stuff */
	if (retry)
		goto again;
}

instead of the normal:

void foo(void)
{
again:
	/* do stuff */
	if (retry)
		goto again;
}

Re: [PATCH] sched: leave sched_setscheduler earlier if possible.

[Junio C Hamano]

Peter Zijlstra <a.p.zijlstra@chello.nl> writes:

> Junio, know of any way to make git-diff do the same? The purpose is to
> skip labels as functions so that people stop doing stupid crap like
> indenting labels, eg.:
>
> void foo(void)
> {
>  again:

Perhaps "git help attributes" and look for "funcname"?

Re: [PATCH] sched: leave sched_setscheduler earlier if possible.

[Peter Zijlstra]

On Fri, 2011-03-25 at 15:36 -0700, Junio C Hamano wrote:
> Peter Zijlstra <a.p.zijlstra@chello.nl> writes:
> 
> > Junio, know of any way to make git-diff do the same? The purpose is to
> > skip labels as functions so that people stop doing stupid crap like
> > indenting labels, eg.:
> >
> > void foo(void)
> > {
> >  again:
> 
> Perhaps "git help attributes" and look for "funcname"?

Awesome so the diff.$foo.xfuncname is about what I want, except I seem
to need a .gitattributes file per repository. 

Is there a way to over-ride the default in a global way so that I can
only change ~/.gitconfig and not bother with all various repos I have?

Another question, the built-in patterns consist of multiple regexes, can
custom patterns also have multiple?


So what worked for me was:

~/.gitconfig:

[diff "cpp"]
	xfuncname = "^[[:alpha:]$_].*[^:]$"

and linux-2.6/.gitattributes:

*.h diff=cpp
*.c diff=cpp

What I tried was:

[diff]
  xfuncname = "^[[:alpha:]$_].*[^:]$"

But that didn't seem to work.. I also tried ~/.gitattributes, but again,
no joy.

Re: [PATCH] sched: leave sched_setscheduler earlier if possible.

[Peter Zijlstra]

On Sat, 2011-03-26 at 11:26 +0100, Peter Zijlstra wrote:
> Awesome so the diff.$foo.xfuncname is about what I want, except I seem
> to need a .gitattributes file per repository. 
> 
> Is there a way to over-ride the default in a global way so that I can
> only change ~/.gitconfig and not bother with all various repos I have?
> 
> Another question, the built-in patterns consist of multiple regexes, can
> custom patterns also have multiple?
> 
> 
> So what worked for me was:
> 
> ~/.gitconfig:
> 
> [diff "cpp"]
>         xfuncname = "^[[:alpha:]$_].*[^:]$"
> 
> and linux-2.6/.gitattributes:
> 
> *.h diff=cpp
> *.c diff=cpp
> 
> What I tried was:
> 
> [diff]
>   xfuncname = "^[[:alpha:]$_].*[^:]$"
> 
> But that didn't seem to work.. I also tried ~/.gitattributes, but again,
> no joy. 

D0'h, so what worked is:

# cat ~/.gitconfig
[diff "default"]
        xfuncname = "^[[:alpha:]$_].*[^:]$"

[tip:sched/urgent] sched: Leave sched_setscheduler() earlier if possible, do not disturb SCHED_FIFO tasks

[tip-bot for Dario Faggioli]

Commit-ID:  a51e91981870d013fcfcc08b0117997edbcbc7a7
Gitweb:     http://git.kernel.org/tip/a51e91981870d013fcfcc08b0117997edbcbc7a7
Author:     Dario Faggioli <raistlin@linux.it>
AuthorDate: Thu, 24 Mar 2011 14:00:18 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 31 Mar 2011 13:00:34 +0200

sched: Leave sched_setscheduler() earlier if possible, do not disturb SCHED_FIFO tasks

sched_setscheduler() (in sched.c) is called in order of changing the
scheduling policy and/or the real-time priority of a task. Thus,
if we find out that neither of those are actually being modified, it
is possible to return earlier and save the overhead of a full
deactivate+activate cycle of the task in question.

Beside that, if we have more than one SCHED_FIFO task with the same
priority on the same rq (which means they share the same priority queue)
having one of them changing its position in the priority queue because of
a sched_setscheduler (as it happens by means of the deactivate+activate)
that does not actually change the priority violates POSIX which states,
for SCHED_FIFO:

  "If a thread whose policy or priority has been modified by
   pthread_setschedprio() is a running thread or is runnable, the effect on
   its position in the thread list depends on the direction of the
   modification, as follows: a. <...> b. If the priority is unchanged, the
   thread does not change position in the thread list. c. <...>"

     http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_08.html

 (ed: And the POSIX specification here does, briefly and somewhat unexpectedly,
      match what common sense tells us as well. )

Signed-off-by: Dario Faggioli <raistlin@linux.it>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1300971618.3960.82.camel@Palantir>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index f592ce6..a884551 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -5011,6 +5011,17 @@ recheck:
 		return -EINVAL;
 	}
 
+	/*
+	 * If not changing anything there's no need to proceed further:
+	 */
+	if (unlikely(policy == p->policy && (!rt_policy(policy) ||
+			param->sched_priority == p->rt_priority))) {
+
+		__task_rq_unlock(rq);
+		raw_spin_unlock_irqrestore(&p->pi_lock, flags);
+		return 0;
+	}
+
 #ifdef CONFIG_RT_GROUP_SCHED
 	if (user) {
 		/*