Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1431016 > unrolled thread

Re: regression caused by bb6ab52f2bef ("intel_pstate: Do not set utilization update hook too early")

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: regression caused by bb6ab52f2bef ("intel_pstate: Do not set utilization update hook too early") "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-06-25 02:20 +0200
    Re: regression caused by bb6ab52f2bef ("intel_pstate: Do not set  utilization update hook too early") "Pandruvada, Srinivas" <srinivas.pandruvada@intel.com> - 2016-06-25 17:10 +0200
    RE: regression caused by bb6ab52f2bef ("intel_pstate: Do not set utilization update hook too early") "Doug Smythies" <dsmythies@telus.net> - 2016-06-26 02:30 +0200
    Re: regression caused by bb6ab52f2bef ("intel_pstate: Do not set  utilization update hook too early") Jisheng Zhang <jszhang@marvell.com> - 2016-06-27 08:20 +0200

#1431016 — Re: regression caused by bb6ab52f2bef ("intel_pstate: Do not set utilization update hook too early")

On Friday, June 17, 2016 04:09:33 PM Jisheng Zhang wrote:
> Dear all,
> 
> If using acpi-cpufreq instead, v4.6, v4.6-rc3, v4.7-rc3 can't reproduce the issue. It seems
> only intel_pstate is impacted.

Which is quite obvious, since the commit your bisection led to was
intel_pstate-specific. :-)

If the issue is what I'm thinking it is, the patch below should help, so
can you please test it?

Thanks,
Rafael

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] intel_pstate: Do not clear utilization update hooks on policy changes

intel_pstate_set_policy() is invoked by the cpufreq core during
driver initialization, on changes of policy attributes (minimim and
maximum frequency, for example) via sysfs and via CPU notifications
from the platform firmware.  On some platforms the latter may occur
relatively often.

Commit bb6ab52f2bef (intel_pstate: Do not set utilization update hook
too early) made intel_pstate_set_policy() clear the CPU's utilization
update hook before updating the policy attributes for it (and set the
hook again after doind that), but that involves invoking
synchronize_sched() and adds overhead to the CPU notifications
mentioned above and to the sched-RCU handling in general.

That extra overhead is arguably not necessary, because updating
policy attributes when the CPU's utilization update hook is active
should not lead to any adverse effects, so drop the clearing of
the hook from intel_pstate_set_policy() and make it check if
the hook has been set already when attempting to set it.

Fixes: bb6ab52f2bef (intel_pstate: Do not set utilization update hook too early)
Reported-by: Jisheng Zhang <jszhang@marvell.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1440,6 +1440,9 @@ static void intel_pstate_set_update_util
 {
 	struct cpudata *cpu = all_cpu_data[cpu_num];
 
+	if (cpu->update_util_set)
+		return;
+
 	/* Prevent intel_pstate_update_util() from using stale data. */
 	cpu->sample.time = 0;
 	cpufreq_add_update_util_hook(cpu_num, &cpu->update_util,
@@ -1480,8 +1483,6 @@ static int intel_pstate_set_policy(struc
 	if (!policy->cpuinfo.max_freq)
 		return -ENODEV;
 
-	intel_pstate_clear_update_util_hook(policy->cpu);
-
 	pr_debug("set_policy cpuinfo.max %u policy->max %u\n",
 		 policy->cpuinfo.max_freq, policy->max);
 

[toc] | [next] | [standalone]

#1431142 — Re: regression caused by bb6ab52f2bef ("intel_pstate: Do not set utilization update hook too early")

On Sat, 2016-06-25 at 02:14 +0200, Rafael J. Wysocki wrote:
> On Friday, June 17, 2016 04:09:33 PM Jisheng Zhang wrote:
> > Dear all,
> > 
> > If using acpi-cpufreq instead, v4.6, v4.6-rc3, v4.7-rc3 can't
> > reproduce the issue. It seems
> > only intel_pstate is impacted.
> 
> Which is quite obvious, since the commit your bisection led to was
> intel_pstate-specific. :-)
> 
We should also check why the set_policy callback is getting called
quite often. May be some thermal zone is tripping quite often.

echo 'file thermal_core.c +p' > /sys/kernel/debug/dynamic_debug/control

may give us some clue.

Thanks,
Srinivas

> If the issue is what I'm thinking it is, the patch below should help,
> so
> can you please test it?
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] intel_pstate: Do not clear utilization update hooks
> on policy changes
> 
> intel_pstate_set_policy() is invoked by the cpufreq core during
> driver initialization, on changes of policy attributes (minimim and
> maximum frequency, for example) via sysfs and via CPU notifications
> from the platform firmware.  On some platforms the latter may occur
> relatively often.
> 
> Commit bb6ab52f2bef (intel_pstate: Do not set utilization update hook
> too early) made intel_pstate_set_policy() clear the CPU's utilization
> update hook before updating the policy attributes for it (and set the
> hook again after doind that), but that involves invoking
> synchronize_sched() and adds overhead to the CPU notifications
> mentioned above and to the sched-RCU handling in general.
> 
> That extra overhead is arguably not necessary, because updating
> policy attributes when the CPU's utilization update hook is active
> should not lead to any adverse effects, so drop the clearing of
> the hook from intel_pstate_set_policy() and make it check if
> the hook has been set already when attempting to set it.
> 
> Fixes: bb6ab52f2bef (intel_pstate: Do not set utilization update hook
> too early)
> Reported-by: Jisheng Zhang <jszhang@marvell.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -1440,6 +1440,9 @@ static void intel_pstate_set_update_util
>  {
>  	struct cpudata *cpu = all_cpu_data[cpu_num];
>  
> +	if (cpu->update_util_set)
> +		return;
> +
>  	/* Prevent intel_pstate_update_util() from using stale data.
> */
>  	cpu->sample.time = 0;
>  	cpufreq_add_update_util_hook(cpu_num, &cpu->update_util,
> @@ -1480,8 +1483,6 @@ static int intel_pstate_set_policy(struc
>  	if (!policy->cpuinfo.max_freq)
>  		return -ENODEV;
>  
> -	intel_pstate_clear_update_util_hook(policy->cpu);
> -
>  	pr_debug("set_policy cpuinfo.max %u policy->max %u\n",
>  		 policy->cpuinfo.max_freq, policy->max);
>  
> 

[toc] | [prev] | [next] | [standalone]

#1431264

On 2016.06.24 16:09 Rafael J. Wysocki wrote:
> On Friday, June 17, 2016 04:09:33 PM Jisheng Zhang wrote:
>> Dear all,
>> 
>> If using acpi-cpufreq instead, v4.6, v4.6-rc3, v4.7-rc3 can't reproduce the issue. It seems
>> only intel_pstate is impacted.
>
> Which is quite obvious, since the commit your bisection led to was
> intel_pstate-specific. :-)
>
> If the issue is what I'm thinking it is, the patch below should help, so
> can you please test it?

Rafael, while you asked Jisheng to test, I tested it also, since I was
already setup for testing this stuff. Summary: works great, see further below
for details.

On 2016.06.25 08:10 Srinivas Pandruvada wrote:
> We should also check why the set_policy callback is getting called
> quite often. May be some thermal zone is tripping quite often.
>
> echo 'file thermal_core.c +p' > /sys/kernel/debug/dynamic_debug/control
>
> may give us some clue.

Srinivas, This part has me baffled, particularly with the new test data
(see further below). Note that my test sever never suffers from thermal events,
It can run flat out on all CPU's forever.

Details (some old test data repeated so as to provide context with the new data):

Powertop Wakeups/Second as a function of sample time:
Sample time	Kernel 4.7-rc4	+rjw patch
(seconds)	Wakeups/second	Wakeups/second
300			~20			~14
200			~20			~15
100			~22			~16
 50			~29			~14
 30			~33			~15
 20			~44			~15
  5						~17 (noisy)
  3			~155			~20 (noisy)

Manual timer stats method:
Kernel 4.7-rc4: ~20 Events/Second
Kernel 4.7-rc4+rjw patch: ~20 Events / Second
Kernel 4.4.0-24 (Ubuntu version numbering method): ~20 Events / Second

Note (to self): Do the timer stats method over a long period (say 300 seconds)
so as to reduce the localized influence from running the script itself.

Skipped samples in the intel_pstate driver while running powertop at 5 second sample time:
Kernel 4.7-rc4: ~200 / minute, or ~3.3 per second.
Kernel 4.7-rc4+rjw patch: 0.
Kernel 4.4.0-24: 0.
Other tests were done with Kernel 4.7-rc4 such as compile the kernel and a bunch of Phoronix tests,
and some skipped samples were observed (8 over a 3 hour trace session), but nothing like when running powertop.

Check for messing with the minimum frequency while running powertop at 5 second sample time:
Command: watch -n 0.3 -g cat /sys/devices/system/cpu/cpu*/cpufreq/scaling_min_freq
Kernel 4.7-rc4: Never takes more than a few second to get a hit.
Kernel 4.7-rc4+rjw patch: Ran for over 2 hours without a hit.
Kernel 4.4.0-24: Can not recall how long I ran the test for. No hit.
Srinivas, this is what has me baffled. If it wasn't powertop itself messing
with the minimum CPU clock frequency and setting it to maximum, then what was it?

On an otherwise "idle" system,
how many times does the intel-pstate driver run per unit time?
Kernel 4.7-rc4: (14341 + 0 skipped) times / 1000 seconds.
Kernel 4.7-rc4 + powertop --time=5: (38148 + 3075 skipped) times / 1000 seconds.
Kernel 4.7-rc4+rjw: (11947 + 0 skipped) times / 1000 seconds.
Kernel 4.7-rc4+rjw + powertop --time=5: (22657 + 0 skipped) times / 1000 seconds.
Kernel 4.4.0-24: (5725 + 0 skipped) times / 1000 seconds.
Kernel 4.4.0-24 + powertop --time=5: (16656 + 0 skipped) times / 1000 seconds.

Important note: It is a good thing that the number of driver passes per unit time increased
with the recent changes. The driver was not running often enough before,
often hitting the watchdog limits. Isn't it energy and performance that
matters (see next test)?

Energy consumption on an otherwise "idle" system (package power):
Kernel 4.7-rc4: 3.84 Watts
Kernel 4.7-rc4 + powertop --time=5: sometimes 4.92 watts, sometimes 6.2 watts (not sure why)
Kernel 4.7-rc4+rjw: 3.88 Watts
Kernel 4.7-rc4+rjw + powertop --time=5: 4.5 watts (did observe a 6.3 watts one)
Kernel 4.4.0-24: 3.92 Watts.
Kernel 4.4.0-24 + powertop --time=5: did not test.
While there are variations in the results, the 2 to 3% savings in idle energy seems
somewhat consistent (referring to between Kernel 4.4 and 4.7-rc4+rjw patch, without powertop).

... Doug

[toc] | [prev] | [next] | [standalone]

#1431742 — Re: regression caused by bb6ab52f2bef ("intel_pstate: Do not set utilization update hook too early")

Dear Rafael,

On Sat, 25 Jun 2016 02:14:28 +0200 "Rafael J. Wysocki" wrote:

> On Friday, June 17, 2016 04:09:33 PM Jisheng Zhang wrote:
> > Dear all,
> > 
> > If using acpi-cpufreq instead, v4.6, v4.6-rc3, v4.7-rc3 can't reproduce the issue. It seems
> > only intel_pstate is impacted.  
> 
> Which is quite obvious, since the commit your bisection led to was
> intel_pstate-specific. :-)
> 
> If the issue is what I'm thinking it is, the patch below should help, so
> can you please test it?
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] intel_pstate: Do not clear utilization update hooks on policy changes
> 
> intel_pstate_set_policy() is invoked by the cpufreq core during
> driver initialization, on changes of policy attributes (minimim and
> maximum frequency, for example) via sysfs and via CPU notifications
> from the platform firmware.  On some platforms the latter may occur
> relatively often.
> 
> Commit bb6ab52f2bef (intel_pstate: Do not set utilization update hook
> too early) made intel_pstate_set_policy() clear the CPU's utilization
> update hook before updating the policy attributes for it (and set the
> hook again after doind that), but that involves invoking
> synchronize_sched() and adds overhead to the CPU notifications
> mentioned above and to the sched-RCU handling in general.
> 
> That extra overhead is arguably not necessary, because updating
> policy attributes when the CPU's utilization update hook is active
> should not lead to any adverse effects, so drop the clearing of
> the hook from intel_pstate_set_policy() and make it check if
> the hook has been set already when attempting to set it.

This patch works! 

on a 32 snb cores server:

80 wakeups/s w/o the patch

7 wakeups/s w/ the patch


on a 4 snb cores laptop:

22 wakeups/s w/o the patch

6 wakeups/s w/ the patch

Thank you so much for fixing it ;)

> 
> Fixes: bb6ab52f2bef (intel_pstate: Do not set utilization update hook too early)
> Reported-by: Jisheng Zhang <jszhang@marvell.com>

So feel free to add

Tested-by: Jisheng Zhang <jszhang@marvell.com>

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -1440,6 +1440,9 @@ static void intel_pstate_set_update_util
>  {
>  	struct cpudata *cpu = all_cpu_data[cpu_num];
>  
> +	if (cpu->update_util_set)
> +		return;
> +
>  	/* Prevent intel_pstate_update_util() from using stale data. */
>  	cpu->sample.time = 0;
>  	cpufreq_add_update_util_hook(cpu_num, &cpu->update_util,
> @@ -1480,8 +1483,6 @@ static int intel_pstate_set_policy(struc
>  	if (!policy->cpuinfo.max_freq)
>  		return -ENODEV;
>  
> -	intel_pstate_clear_update_util_hook(policy->cpu);
> -
>  	pr_debug("set_policy cpuinfo.max %u policy->max %u\n",
>  		 policy->cpuinfo.max_freq, policy->max);
>  
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web