LinuxLists.cc - [ 00/75] 3.3.5-stable review

2012-05-04 20:43:06

Just earlier this morning, an issue was raised against this patch on the related thread on Ubuntu launchpad.
The complaint is that reported load averages are now to high under conditions of "high" frequency enter into / exit from cpu idle conditions where the cpu is very lightly loaded.
Most of the testing I did was with medium to heavy load on the cpu and relatively short idle periods. This is the opposite.
For a quick test, I hacked up my test program, and was able to reproduce the issue.
I am still attempting to understand better and also determine the lower bound to "high" frequency (I think it is 25 Hertz, and scales from there down, but no proof yet). I'll also go backwards and test this scenario without the patch. I'll let this list know the results, but it might be a few days.
My quick test results are attached.

Doug Smythies

References (even though I have been told not to include links in e-mails to this list):

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/838811 <<< starting from post #52

version:
doug@s15:~/c$ uname -a
Linux s15 3.2.0-24-generic #37-Ubuntu SMP Wed Apr 25 08:43:22 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
doug@s15:~/c$ cat /proc/version
Linux version 3.2.0-24-generic (buildd@yellow) (gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5) ) #37-Ubuntu SMP Wed Apr 25 08:43:22 UTC 2012

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: May-04-2012 13:43
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; Doug Smythies; LesÅ�aw Kopeć; Aman Gupta; Peter Zijlstra; Ingo Molnar; Kerin Millar
Subject: [ 56/75] sched: Fix nohz load accounting -- again!

3.3-stable review patch. If anyone has any objections, please let me know.

------------------

From: Peter Zijlstra <[email protected]>

commit c308b56b5398779cd3da0f62ab26b0453494c3d4 upstream.

Various people reported nohz load tracking still being wrecked, but Doug spotted the actual problem. We fold the nohz remainder in too soon, causing us to loose samples and under-account.

So instead of playing catch-up up-front, always do a single load-fold with whatever state we encounter and only then fold the nohz remainder and play catch-up.

Reported-by: Doug Smythies <[email protected]>
Reported-by: LesÅ=82aw Kope=C4=87 <[email protected]>
Reported-by: Aman Gupta <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/n/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
Cc: Kerin Millar <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>

---
kernel/sched/core.c | 53 +++++++++++++++++++++++++---------------------------
1 file changed, 26 insertions(+), 27 deletions(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2266,13 +2266,10 @@ calc_load_n(unsigned long load, unsigned
* Once we've updated the global active value, we need to apply the exponential
* weights adjusted to the number of cycles missed.
*/
-static void calc_global_nohz(unsigned long ticks)
+static void calc_global_nohz(void)
{
long delta, active, n;

- if (time_before(jiffies, calc_load_update))
- return;
-
/*
* If we crossed a calc_load_update boundary, make sure to fold
* any pending idle changes, the respective CPUs might have @@ -2284,31 +2281,25 @@ static void calc_global_nohz(unsigned lo
atomic_long_add(delta, &calc_load_tasks);

/*
- * If we were idle for multiple load cycles, apply them.
+ * It could be the one fold was all it took, we done!
*/
- if (ticks >= LOAD_FREQ) {
- n = ticks / LOAD_FREQ;
+ if (time_before(jiffies, calc_load_update + 10))
+ return;

- active = atomic_long_read(&calc_load_tasks);
- active = active > 0 ? active * FIXED_1 : 0;
+ /*
+ * Catch-up, fold however many we are behind still
+ */
+ delta = jiffies - calc_load_update - 10;
+ n = 1 + (delta / LOAD_FREQ);

- avenrun[0] = calc_load_n(avenrun[0], EXP_1, active, n);
- avenrun[1] = calc_load_n(avenrun[1], EXP_5, active, n);
- avenrun[2] = calc_load_n(avenrun[2], EXP_15, active, n);
+ active = atomic_long_read(&calc_load_tasks);
+ active = active > 0 ? active * FIXED_1 : 0;

- calc_load_update += n * LOAD_FREQ;
- }
+ avenrun[0] = calc_load_n(avenrun[0], EXP_1, active, n);
+ avenrun[1] = calc_load_n(avenrun[1], EXP_5, active, n);
+ avenrun[2] = calc_load_n(avenrun[2], EXP_15, active, n);

- /*
- * Its possible the remainder of the above division also crosses
- * a LOAD_FREQ period, the regular check in calc_global_load()
- * which comes after this will take care of that.
- *
- * Consider us being 11 ticks before a cycle completion, and us
- * sleeping for 4*LOAD_FREQ + 22 ticks, then the above code will
- * age us 4 cycles, and the test in calc_global_load() will
- * pick up the final one.
- */
+ calc_load_update += n * LOAD_FREQ;
}
#else
void calc_load_account_idle(struct rq *this_rq) @@ -2320,7 +2311,7 @@ static inline long calc_load_fold_idle(v
return 0;
}

-static void calc_global_nohz(unsigned long ticks)
+static void calc_global_nohz(void)
{
}
#endif
@@ -2348,8 +2339,6 @@ void calc_global_load(unsigned long tick {
long active;

- calc_global_nohz(ticks);
-
if (time_before(jiffies, calc_load_update + 10))
return;

@@ -2361,6 +2350,16 @@ void calc_global_load(unsigned long tick
avenrun[2] = calc_load(avenrun[2], EXP_15, active);

calc_load_update += LOAD_FREQ;
+
+ /*
+ * Account one period with whatever state we found before
+ * folding in the nohz state and ageing the entire idle period.
+ *
+ * This avoids loosing a sample when we go idle between
+ * calc_load_account_active() (10 ticks ago) and now and thus
+ * under-accounting.
+ */
+ calc_global_nohz();
}

/*

Attachments:

high01.txt (4.56 kB)

2012-05-05 19:39:38

by Greg Kroah-Hartman

[permalink] [raw]

On Mon, 2012-05-07 at 16:45 -0700, Stanislav Yakovlev wrote:
> On 7 May 2012 07:37, Ben Hutchings <[email protected]> wrote:
> > On Fri, 2012-05-04 at 13:43 -0700, Greg KH wrote:
> >> 3.3-stable review patch. If anyone has any objections, please let me know.
> >>
> >> ------------------
> >>
> >> From: Stanislav Yakovlev <[email protected]>
> >>
> >> commit dd447319895d0c0af423e483d9b63f84f3f8869a upstream.
> >>
> >> Driver incorrectly validates command completion: instead of waiting
> >> for a command to be acknowledged it continues execution. Most of the
> >> time driver gets acknowledge of the command completion in a tasklet
> >> before it executes the next one. But sometimes it sends the next
> >> command before it gets acknowledge for the previous one. In such a
> >> case one of the following error messages appear in the log:
> > [...]
> >> + now = jiffies;
> >> + end = now + HOST_COMPLETE_TIMEOUT;
> >> +again:
> >> rc = wait_event_interruptible_timeout(priv->wait_command_queue,
> >> !(priv->
> >> status & STATUS_HCMD_ACTIVE),
> >> - HOST_COMPLETE_TIMEOUT);
> >> + end - now);
> >> + if (rc < 0) {
> >> + now = jiffies;
> >> + if (time_before(now, end))
> >> + goto again;
> >> + rc = 0;
> >> + }
> > [...]
> >
> > If you don't want the wait to be interrupted, use wait_event_timeout()
> > instead of this ridiculous loop!
>
> Usually "modprobe ipw2200" takes less than a second, and after
> switching to wait_event_timeout it will take more than 10 seconds. We
> can not decrease the waiting timeout because we want to keep it big
> enough for the worst case(when the firmware did not reply).
>
> There is a relevant discussion on lklm:
> https://lkml.org/lkml/2008/9/25/2
> https://lkml.org/lkml/2008/9/26/194

That's irrelevant: you aren't letting userland handle the signal here,
so you should use wait_event_timeout() and no loop.

Ben.

--
Ben Hutchings
Life is what happens to you while you're busy making other plans.
- John Lennon

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2012-05-08 00:31:30

by Greg Kroah-Hartman

Further to my e-mail from a couple of weeks ago, attached is a revised version of the png file I attached. Hopefully it clarifies things a little, and I hope it helps.
As before, it would be best to view with zoom set to 1:1 and to scroll down as you read and look at the graphs.

Doug Smythies

-----Original Message-----
From: Doug Smythies [mailto:[email protected]]
Sent: May-09-2012 16:05
To: 'Greg KH'; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; 'LesÅ�aw Kopeć'; 'Aman Gupta'; 'Peter Zijlstra'; 'Ingo Molnar'; 'Kerin Millar'; 'Doug Smythies'
Subject: RE: [ 56/75] sched: Fix nohz load accounting -- again!

Now I have some more test results for low load conditions.

Summary:

Reported load averages are very much improved for conditions of medium to high actual loads and higher idle enter/exit frequencies.

Reported load averages are in error, too high, by a factor of roughly between 3 and 7 times for conditions of low actual loads and any idle enter/exit frequency.

The complaints that are being raised (not by me, I like the fix, but now by many on Ubuntu Luanchpad. I think all complaints are from desktop users. I only use server versions myself, and never observer the issues) are related to the actual low load average case, and is compounded by the point of reference, the non-commit kernels, being in error on the low side.

Detail:

For my part of it, I made a mistake when I was testing my original patch proposal and the one from Peter Z. I did notice a huge change in load averages when the idle / not idle duty cycle went low enough. However, I attributed it all to the CPUs throttling back to a lower clock frequency. I continued testing with a preference to always having a high enough load to keep the CPUs at maximum frequency. My mistake was not realizing the contribution from incorrectly reported load averages at lower loads, in addition to the CPU clock frequency changes. New tests done for this e-mail were done with the frequency governors set to "powersave" mode for all CPUs (except where I forgot for one test).

Attached is an old e-mail with a bunch of test results for high load conditions, with control samples and such.
Summary: Everything looked good for the patch Peter Z. proposed.

Attached is a PNG file with my test results and comments for new low actual load tests. It would be best to view with zoom set to 1:1 and to scroll down as you read and look at the graphs.

Doug Smythies

References (even though I have been told not to include links in e-mails to this list):
My web notes (although it will be a few days before they are caught up): http://www.smythies.com/~doug/network/load_average/index.html
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/838811 <<< starting from post #52
https://bugs.launchpad.net/xubuntu-desktop/+bug/985661

-----Original Message-----
From: Doug Smythies [mailto:[email protected]]
Sent: May-04-2012 15:03
To: 'Greg KH'; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; 'LesÅ�aw Kopeć'; 'Aman Gupta'; 'Peter Zijlstra'; 'Ingo Molnar'; 'Kerin Millar'; Doug Smythies
Subject: RE: [ 56/75] sched: Fix nohz load accounting -- again!

Just earlier this morning, an issue was raised against this patch on the related thread on Ubuntu launchpad.
The complaint is that reported load averages are now to high under conditions of "high" frequency enter into / exit from cpu idle conditions where the cpu is very lightly loaded.
Most of the testing I did was with medium to heavy load on the cpu and relatively short idle periods. This is the opposite.
For a quick test, I hacked up my test program, and was able to reproduce the issue.
I am still attempting to understand better and also determine the lower bound to "high" frequency (I think it is 25 Hertz, and scales from there down, but no proof yet). I'll also go backwards and test this scenario without the patch. I'll let this list know the results, but it might be a few days.
My quick test results are attached.

Doug Smythies

References (even though I have been told not to include links in e-mails to this list):

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/838811 <<< starting from post #52

version:
doug@s15:~/c$ uname -a
Linux s15 3.2.0-24-generic #37-Ubuntu SMP Wed Apr 25 08:43:22 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux doug@s15:~/c$ cat /proc/version Linux version 3.2.0-24-generic (buildd@yellow) (gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5) ) #37-Ubuntu SMP Wed Apr 25 08:43:22 UTC 2012

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: May-04-2012 13:43
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; Doug Smythies; LesÅ aw Kopeć; Aman Gupta; Peter Zijlstra; Ingo Molnar; Kerin Millar
Subject: [ 56/75] sched: Fix nohz load accounting -- again!

3.3-stable review patch. If anyone has any objections, please let me know.

------------------

From: Peter Zijlstra <[email protected]>

commit c308b56b5398779cd3da0f62ab26b0453494c3d4 upstream.

Various people reported nohz load tracking still being wrecked, but Doug spotted the actual problem. We fold the nohz remainder in too soon, causing us to loose samples and under-account.

So instead of playing catch-up up-front, always do a single load-fold with whatever state we encounter and only then fold the nohz remainder and play catch-up.

Reported-by: Doug Smythies <[email protected]>
Reported-by: LesÅ=82aw Kope=C4=87 <[email protected]>
Reported-by: Aman Gupta <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/n/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
Cc: Kerin Millar <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>

---
kernel/sched/core.c | 53 +++++++++++++++++++++++++---------------------------
1 file changed, 26 insertions(+), 27 deletions(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2266,13 +2266,10 @@ calc_load_n(unsigned long load, unsigned
* Once we've updated the global active value, we need to apply the exponential
* weights adjusted to the number of cycles missed.
*/
-static void calc_global_nohz(unsigned long ticks)
+static void calc_global_nohz(void)
{
long delta, active, n;

- if (time_before(jiffies, calc_load_update))
- return;
-
/*
* If we crossed a calc_load_update boundary, make sure to fold
* any pending idle changes, the respective CPUs might have @@ -2284,31 +2281,25 @@ static void calc_global_nohz(unsigned lo
atomic_long_add(delta, &calc_load_tasks);

/*
- * If we were idle for multiple load cycles, apply them.
+ * It could be the one fold was all it took, we done!
*/
- if (ticks >= LOAD_FREQ) {
- n = ticks / LOAD_FREQ;
+ if (time_before(jiffies, calc_load_update + 10))
+ return;

- active = atomic_long_read(&calc_load_tasks);
- active = active > 0 ? active * FIXED_1 : 0;
+ /*
+ * Catch-up, fold however many we are behind still
+ */
+ delta = jiffies - calc_load_update - 10;
+ n = 1 + (delta / LOAD_FREQ);

- avenrun[0] = calc_load_n(avenrun[0], EXP_1, active, n);
- avenrun[1] = calc_load_n(avenrun[1], EXP_5, active, n);
- avenrun[2] = calc_load_n(avenrun[2], EXP_15, active, n);
+ active = atomic_long_read(&calc_load_tasks);
+ active = active > 0 ? active * FIXED_1 : 0;

- calc_load_update += n * LOAD_FREQ;
- }
+ avenrun[0] = calc_load_n(avenrun[0], EXP_1, active, n);
+ avenrun[1] = calc_load_n(avenrun[1], EXP_5, active, n);
+ avenrun[2] = calc_load_n(avenrun[2], EXP_15, active, n);

- /*
- * Its possible the remainder of the above division also crosses
- * a LOAD_FREQ period, the regular check in calc_global_load()
- * which comes after this will take care of that.
- *
- * Consider us being 11 ticks before a cycle completion, and us
- * sleeping for 4*LOAD_FREQ + 22 ticks, then the above code will
- * age us 4 cycles, and the test in calc_global_load() will
- * pick up the final one.
- */
+ calc_load_update += n * LOAD_FREQ;
}
#else
void calc_load_account_idle(struct rq *this_rq) @@ -2320,7 +2311,7 @@ static inline long calc_load_fold_idle(v
return 0;
}

-static void calc_global_nohz(unsigned long ticks)
+static void calc_global_nohz(void)
{
}
#endif
@@ -2348,8 +2339,6 @@ void calc_global_load(unsigned long tick {
long active;

- calc_global_nohz(ticks);
-
if (time_before(jiffies, calc_load_update + 10))
return;

@@ -2361,6 +2350,16 @@ void calc_global_load(unsigned long tick
avenrun[2] = calc_load(avenrun[2], EXP_15, active);

calc_load_update += LOAD_FREQ;
+
+ /*
+ * Account one period with whatever state we found before
+ * folding in the nohz state and ageing the entire idle period.
+ *
+ * This avoids loosing a sample when we go idle between
+ * calc_load_account_active() (10 ticks ago) and now and thus
+ * under-accounting.
+ */
+ calc_global_nohz();
}

/*

Attachments:

commit_low_load_rev2.png (339.73 kB)