Re: [sched/numa] 096aa33863a: -21.4% hackbench.throughput, -20.2% netperf.Throughput_Mbps

[Peter Zijlstra]

On Thu, Aug 07, 2014 at 02:16:16PM -0400, Rik van Riel wrote:
> On 08/07/2014 06:53 AM, Fengguang Wu wrote:
> > Hi Rik,
> > 
> > We noticed the below performance regression in commit
> > 096aa33863a5e48de52d2ff30e0801b7487944f4 ("sched/numa: Decay
> > ->wakee_flips instead of zeroing")
> > 
> > b1ad065e65f5610  096aa33863a5e48de52d2ff30  testbox/testcase/testparams
> > ---------------  -------------------------  ---------------------------
> >     122361 ± 0%     -21.4%      96140 ± 0%  lkp-snb01/hackbench/50%-process-pipe
> >     122361 ± 0%     -21.4%      96140 ± 0%  TOTAL hackbench.throughput
> 
> I guess the performance of that benchmark depends on it
> "slipping under the wire" after each time the kernel
> zeroes out ->wakee_flips.
> 
> Depending on repeatedly pulling the wakee back to the same
> node as the waker suggests something else in the kernel may
> be pulling the wakee to another place in the system repeatedly,
> as well, just at a lower frequency (load balancer?).
> 
> I have also noticed that select_idle_sibling often fails to
> find an idle sibling within the LLC domain, even when it
> exists. Fixing that bug sometimes results in lower performance.
> 
> It appears that some of the performance results of the scheduler
> appear on the code acting in an opposite way to its documented
> intention.
> 
> It may be best to revert 096aa33863a for now...

Does something like so cure anything? Is that a 'normal' hackbench run?
lemme see if I can reproduce on anything.

---
 kernel/sched/fair.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index bfa3c86d0d68..13f00daf8028 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4098,14 +4098,19 @@ static unsigned long cpu_avg_load_per_task(int cpu)
 
 static void record_wakee(struct task_struct *p)
 {
+	unsigned long now = jiffies;
 	/*
 	 * Rough decay (wiping) for cost saving, don't worry
 	 * about the boundary, really active task won't care
 	 * about the loss.
 	 */
-	if (time_after(jiffies, current->wakee_flip_decay_ts + HZ)) {
+	if (time_after(now, current->wakee_flip_decay_ts + BITS_PER_LONG * HZ)) {
+		current->wakee_flips = 0;
+		current->wakee_flip_decay_ts = now;
+	} else while (time_after(now, current->wakee_flip_decay_ts + HZ) && 
+		      current->wakee_flips) {
 		current->wakee_flips >>= 1;
-		current->wakee_flip_decay_ts = jiffies;
+		current->wakee_flip_decay_ts += HZ;
 	}
 
 	if (current->last_wakee != p) {

Attachment: pgpACn1DFxN4m.pgp
Description: PGP signature