[patch] sched: fix select_idle_sibling() regression in selecting an idle SMT sibling

[patch] sched: fix select_idle_sibling() regression in selecting an idle SMT sibling

[Suresh Siddha]

Ingo, Please queue this scheduler patch for the next 3.2-rc release. We
already have a cleaned up version of this patch queued in -tip. If you
have any merge problems in tip/master, me or peterz can help resolve it.

Thanks.
---
From: Peter Zijlstra <peterz@infradead.org>
Subject: sched: fix select_idle_sibling() regression in selecting an idle SMT sibling

Mike Galbraith reported that this recent commit,

   commit 4dcfe1025b513c2c1da5bf5586adb0e80148f612
   Author: Peter Zijlstra <peterz@infradead.org>
   Date:   Thu Nov 10 13:01:10 2011 +0100

       sched: Avoid SMT siblings in select_idle_sibling() if possible

stopped selecting an idle SMT sibling when there are no idle cores in a
single socket system.

Intent of the select_idle_sibling() was to fallback to an idle SMT sibling,
if it fails to identify an idle core. But this fallback was not happening
on systems where all the scheduler domains had `SD_SHARE_PKG_RESOURCES'
flag set.

Fix it. Slightly bigger patch of cleaning all these goto's etc is
queued up for the next release.

Reported-by: Mike Galbraith <efault@gmx.de>
Reported-by: Alex Shi <alex.shi@intel.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/sched_fair.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index a78ed27..8a39fa3 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -2352,13 +2352,11 @@ again:
 		if (!smt && (sd->flags & SD_SHARE_CPUPOWER))
 			continue;
 
-		if (!(sd->flags & SD_SHARE_PKG_RESOURCES)) {
-			if (!smt) {
-				smt = 1;
-				goto again;
-			}
+		if (smt && !(sd->flags & SD_SHARE_CPUPOWER))
+			break;
+
+		if (!(sd->flags & SD_SHARE_PKG_RESOURCES))
 			break;
-		}
 
 		sg = sd->groups;
 		do {
@@ -2378,6 +2376,10 @@ next:
 			sg = sg->next;
 		} while (sg != sd->groups);
 	}
+	if (!smt) {
+		smt = 1;
+		goto again;
+	}
 done:
 	rcu_read_unlock();
 

Re: [patch] sched: fix select_idle_sibling() regression in selecting an idle SMT sibling

[Ingo Molnar]

* Suresh Siddha <suresh.b.siddha@intel.com> wrote:

> Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

I changed this to Signed-off-by as you are forwarding Peter's 
patch. (let me know if that is not fine by you)

Thanks,

	Ingo

[tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[tip-bot for Peter Zijlstra]

Commit-ID:  ab2789213d224202237292d78aaa0c386c7b28b2
Gitweb:     http://git.kernel.org/tip/ab2789213d224202237292d78aaa0c386c7b28b2
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 15 Dec 2011 11:47:00 -0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 16 Dec 2011 09:44:58 +0100

sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

Mike Galbraith reported that this recent commit:

   commit 4dcfe1025b513c2c1da5bf5586adb0e80148f612
   Author: Peter Zijlstra <peterz@infradead.org>
   Date:   Thu Nov 10 13:01:10 2011 +0100

       sched: Avoid SMT siblings in select_idle_sibling() if possible

stopped selecting an idle SMT sibling when there are no idle
cores in a single socket system.

Intent of the select_idle_sibling() was to fallback to an idle
SMT sibling, if it fails to identify an idle core. But this
fallback was not happening on systems where all the scheduler
domains had `SD_SHARE_PKG_RESOURCES' flag set.

Fix it. Slightly bigger patch of cleaning all these goto's etc
is queued up for the next release.

Reported-by: Mike Galbraith <efault@gmx.de>
Reported-by: Alex Shi <alex.shi@intel.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Link: http://lkml.kernel.org/r/1323978421.1984.244.camel@sbsiddha-desk.sc.intel.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched_fair.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index a78ed27..8a39fa3 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -2352,13 +2352,11 @@ again:
 		if (!smt && (sd->flags & SD_SHARE_CPUPOWER))
 			continue;
 
-		if (!(sd->flags & SD_SHARE_PKG_RESOURCES)) {
-			if (!smt) {
-				smt = 1;
-				goto again;
-			}
+		if (smt && !(sd->flags & SD_SHARE_CPUPOWER))
+			break;
+
+		if (!(sd->flags & SD_SHARE_PKG_RESOURCES))
 			break;
-		}
 
 		sg = sd->groups;
 		do {
@@ -2378,6 +2376,10 @@ next:
 			sg = sg->next;
 		} while (sg != sd->groups);
 	}
+	if (!smt) {
+		smt = 1;
+		goto again;
+	}
 done:
 	rcu_read_unlock();
 

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[alex shi]

> Fix it. Slightly bigger patch of cleaning all these goto's etc
> is queued up for the next release.
>

Even with this fix, the loopback netperf testing TCP-RR, still have
about 5% regresion on our 2 socket * 8 cores SNB machine.
which is your patchset includes all cleaning? wondering if that is helpful.

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Peter Zijlstra]

On Wed, 2011-12-21 at 20:33 +0800, alex shi wrote:
> > Fix it. Slightly bigger patch of cleaning all these goto's etc
> > is queued up for the next release.
> >
> 
> Even with this fix, the loopback netperf testing TCP-RR, still have
> about 5% regresion on our 2 socket * 8 cores SNB machine.
> which is your patchset includes all cleaning? wondering if that is helpful.

Sorry, but that question fails to parse here. What were you asking?

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Suresh Siddha]

On Wed, 2011-12-21 at 04:52 -0800, Peter Zijlstra wrote:
> On Wed, 2011-12-21 at 20:33 +0800, alex shi wrote:
> > > Fix it. Slightly bigger patch of cleaning all these goto's etc
> > > is queued up for the next release.
> > >
> > 
> > Even with this fix, the loopback netperf testing TCP-RR, still have
> > about 5% regresion on our 2 socket * 8 cores SNB machine.
> > which is your patchset includes all cleaning? wondering if that is helpful.
> 
> Sorry, but that question fails to parse here. What were you asking?

Yes Alex. You need to elaborate your question. Regression to what kernel
etc?

Also there is another known issue that's been addressed in -tip. Here is
that patch. https://lkml.org/lkml/2011/12/21/137

Please try and if you still see an issue, please report more details
about the regression you are seeing.

thanks,
suresh

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Alex,Shi]

> > > Even with this fix, the loopback netperf testing TCP-RR, still have
> > > about 5% regresion on our 2 socket * 8 cores SNB machine.
> > > which is your patchset includes all cleaning? wondering if that is helpful.
> > 
> > Sorry, but that question fails to parse here. What were you asking?

This patch partly fixed a performance regression that triggered by
4dcfe1025b513c2c, but issue still exists.
> 
> Yes Alex. You need to elaborate your question. Regression to what kernel
> etc?
> 
> Also there is another known issue that's been addressed in -tip. Here is
> that patch. https://lkml.org/lkml/2011/12/21/137

Thanks! 
I am testing whole tip changes against 3.2-rc6. On the 4dcfe1025b513c2c
commit, seems interrupts is abnormal. So maybe above fix is suit for
this issue. wait the results. 

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Suresh Siddha]

On Wed, 2011-12-21 at 17:31 -0800, Shi, Alex wrote:
> This patch partly fixed a performance regression that triggered by
> 4dcfe1025b513c2c, but issue still exists.

So how much was the regression caused by the commit 4dcfe1025b513c2c and
how much did we recover with this fix I posted. If we are talking about
the regression caused by this single commit 4dcfe1025b513c2c, then I
don't know of any other related fixes other than the recent fix we
pushed upstream (ab2789213d224202237292d78aaa0c386c7b28b2).

thanks,
suresh

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Alex,Shi]

On Thu, 2011-12-22 at 10:03 +0800, Siddha, Suresh B wrote:
> On Wed, 2011-12-21 at 17:31 -0800, Shi, Alex wrote:
> > This patch partly fixed a performance regression that triggered by
> > 4dcfe1025b513c2c, but issue still exists.
> 
> So how much was the regression caused by the commit 4dcfe1025b513c2c and
> how much did we recover with this fix I posted. If we are talking about
> the regression caused by this single commit 4dcfe1025b513c2c, then I
> don't know of any other related fixes other than the recent fix we
> pushed upstream (ab2789213d224202237292d78aaa0c386c7b28b2).

A little complex for the whole thing. 
on 4 sockets EX machine, 3~5% hackbench thread regression due to 4dcfe
can be recovered by ab2789. 

But on 2 sockets SNB machine, 1024 clients loop netperf TCP-RR has about
9% regression. and your patch seem recover 2~3%. 

And on a 2 sockets nhm, one of our private benchmark was impact much 20
+% regression. that benchmark just run 4 process, each of process open a
thread, and the thread tasks is to locate randomly pages and than read
from 4 times/write 1 time data into a page.  The ab2789 commit seems no
help our benchmark. 

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Alex,Shi]

On Thu, 2011-12-22 at 10:16 +0800, Alex,Shi wrote:
> On Thu, 2011-12-22 at 10:03 +0800, Siddha, Suresh B wrote:
> > On Wed, 2011-12-21 at 17:31 -0800, Shi, Alex wrote:
> > > This patch partly fixed a performance regression that triggered by
> > > 4dcfe1025b513c2c, but issue still exists.
> > 
> > So how much was the regression caused by the commit 4dcfe1025b513c2c and
> > how much did we recover with this fix I posted. If we are talking about
> > the regression caused by this single commit 4dcfe1025b513c2c, then I
> > don't know of any other related fixes other than the recent fix we
> > pushed upstream (ab2789213d224202237292d78aaa0c386c7b28b2).
> 
> A little complex for the whole thing. 
> on 4 sockets EX machine, 3~5% hackbench thread regression due to 4dcfe
> can be recovered by ab2789. 
> 
> But on 2 sockets SNB machine, 1024 clients loop netperf TCP-RR has about
> 9% regression. and your patch seem recover 2~3%. 

Got the scheduler part from tip tree by command "git diff v3.2-rc6
kernel/sched*" and apply it again 3.2-rc6. Peter's commit "Only queue
remote wakeups when crossing cache boundaries" was included. But testing
show no change for TCP-RR result.

> 
> And on a 2 sockets nhm, one of our private benchmark was impact much 20
> +% regression. that benchmark just run 4 process, each of process open a
> thread, and the thread tasks is to locate randomly pages and than read
> from 4 times/write 1 time data into a page.  The ab2789 commit seems no
> help our benchmark. 
> 
> 
> 
> 

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Suresh Siddha]

On Wed, 2011-12-21 at 18:16 -0800, Shi, Alex wrote:
> On Thu, 2011-12-22 at 10:03 +0800, Siddha, Suresh B wrote:
> > On Wed, 2011-12-21 at 17:31 -0800, Shi, Alex wrote:
> > > This patch partly fixed a performance regression that triggered by
> > > 4dcfe1025b513c2c, but issue still exists.
> > 
> > So how much was the regression caused by the commit 4dcfe1025b513c2c and
> > how much did we recover with this fix I posted. If we are talking about
> > the regression caused by this single commit 4dcfe1025b513c2c, then I
> > don't know of any other related fixes other than the recent fix we
> > pushed upstream (ab2789213d224202237292d78aaa0c386c7b28b2).
> 
> A little complex for the whole thing. 
> on 4 sockets EX machine, 3~5% hackbench thread regression due to 4dcfe
> can be recovered by ab2789. 
> 
> But on 2 sockets SNB machine, 1024 clients loop netperf TCP-RR has about
> 9% regression. and your patch seem recover 2~3%. 
> 
> And on a 2 sockets nhm, one of our private benchmark was impact much 20
> +% regression. that benchmark just run 4 process, each of process open a
> thread, and the thread tasks is to locate randomly pages and than read
> from 4 times/write 1 time data into a page.  The ab2789 commit seems no
> help our benchmark. 

Ok. Can you please try couple of experiments with two kernels? Two
kernels being the base kernel (prior to 4dcfe1025b513c2c) and the second
kernel with the commit ab2789213d224202237292d78aaa0c386c7b28b2.

One experiment with p-states turned off and the second experiment with
c-states turned off.

I suspect mostly deeper core c-states might be contributing to the
behavior that you are seeing.

thanks,
suresh

RE: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Shi, Alex]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1444 bytes --]

> >
> > A little complex for the whole thing.
> > on 4 sockets EX machine, 3~5% hackbench thread regression due to 4dcfe
> > can be recovered by ab2789.
> >
> > But on 2 sockets SNB machine, 1024 clients loop netperf TCP-RR has about
> > 9% regression. and your patch seem recover 2~3%.
> >
> > And on a 2 sockets nhm, one of our private benchmark was impact much 20
> > +% regression. that benchmark just run 4 process, each of process open a
> > thread, and the thread tasks is to locate randomly pages and than read
> > from 4 times/write 1 time data into a page.  The ab2789 commit seems no
> > help our benchmark.

In fact, since L1 cache, D-TLB is shared between SMT siblings, it is not every application can benefit from wakeup on different Cores. 
> 
> Ok. Can you please try couple of experiments with two kernels? Two
> kernels being the base kernel (prior to 4dcfe1025b513c2c) and the second
> kernel with the commit ab2789213d224202237292d78aaa0c386c7b28b2.

Testing started on impacted NHM and SND machines for benchmark netperf hackbench thread and our small case. 
> 
> One experiment with p-states turned off and the second experiment with
> c-states turned off.
> 
> I suspect mostly deeper core c-states might be contributing to the
> behavior that you are seeing.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [tip:sched/urgent] sched: Fix select_idle_sibling() regression in selecting an idle SMT sibling

[Alex,Shi]

On Fri, 2011-12-23 at 10:43 -0800, Suresh Siddha wrote: 
> On Wed, 2011-12-21 at 18:16 -0800, Shi, Alex wrote:
> > On Thu, 2011-12-22 at 10:03 +0800, Siddha, Suresh B wrote:
> > > On Wed, 2011-12-21 at 17:31 -0800, Shi, Alex wrote:
> > > > This patch partly fixed a performance regression that triggered by
> > > > 4dcfe1025b513c2c, but issue still exists.
> > > 
> > > So how much was the regression caused by the commit 4dcfe1025b513c2c and
> > > how much did we recover with this fix I posted. If we are talking about
> > > the regression caused by this single commit 4dcfe1025b513c2c, then I
> > > don't know of any other related fixes other than the recent fix we
> > > pushed upstream (ab2789213d224202237292d78aaa0c386c7b28b2).
> > 
> > A little complex for the whole thing. 
> > on 4 sockets EX machine, 3~5% hackbench thread regression due to 4dcfe
> > can be recovered by ab2789. 
> > 
> > But on 2 sockets SNB machine, 1024 clients loop netperf TCP-RR has about
> > 9% regression. and your patch seem recover 2~3%. 
> > 
> > And on a 2 sockets nhm, one of our private benchmark was impact much 20
> > +% regression. that benchmark just run 4 process, each of process open a
> > thread, and the thread tasks is to locate randomly pages and than read
> > from 4 times/write 1 time data into a page.  The ab2789 commit seems no
> > help our benchmark. 
> 
> Ok. Can you please try couple of experiments with two kernels? Two
> kernels being the base kernel (prior to 4dcfe1025b513c2c) and the second
> kernel with the commit ab2789213d224202237292d78aaa0c386c7b28b2.
> 
> One experiment with p-states turned off and the second experiment with
> c-states turned off.

I did testing on both of kernel with setting 'performance' gov for all
CPU P-states, and disable cpuidle by setting cpuidle.off=1 in cmdline. 

But didn't find measurable impact on performance result.  

> 
> I suspect mostly deeper core c-states might be contributing to the
> behavior that you are seeing.
> 
> thanks,
> suresh
>