Patchwork [1/3] cpufreq: pmac64: speed up frequency switch

login
register
mail settings
Submitter Aaro Koskinen
Date July 23, 2013, 8:24 p.m.
Message ID <1374611079-28091-1-git-send-email-aaro.koskinen@iki.fi>
Download mbox | patch
Permalink /patch/261201/
State Superseded
Headers show

Comments

Aaro Koskinen - July 23, 2013, 8:24 p.m.
Some functions on switch path use msleep() which is inaccurate, and
depends on HZ. With HZ=100 msleep(1) takes actually over ten times longer.
Using usleep_range() we get more accurate sleeps.

I measured the "pfunc_slewing_done" polling to take 300us at max (on
2.3GHz dual-processor Xserve G5), so using 500us sleep there should
be fine.

With the patch, g5_switch_freq() duration drops from ~50ms to ~10ms on
Xserve with HZ=100.

Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
 drivers/cpufreq/pmac64-cpufreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Benjamin Herrenschmidt - July 23, 2013, 9:14 p.m.
On Tue, 2013-07-23 at 23:20 +0200, Rafael J. Wysocki wrote:
> All looks good in the patchset from 10000 feet (or more), but I need
> Ben to speak here.

I want to give it a quick spin on the HW here, I'll ack then. But yes,
it looks good.

Cheers,
Ben.
Rafael J. Wysocki - July 23, 2013, 9:20 p.m.
On Tuesday, July 23, 2013 11:24:37 PM Aaro Koskinen wrote:
> Some functions on switch path use msleep() which is inaccurate, and
> depends on HZ. With HZ=100 msleep(1) takes actually over ten times longer.
> Using usleep_range() we get more accurate sleeps.
> 
> I measured the "pfunc_slewing_done" polling to take 300us at max (on
> 2.3GHz dual-processor Xserve G5), so using 500us sleep there should
> be fine.
> 
> With the patch, g5_switch_freq() duration drops from ~50ms to ~10ms on
> Xserve with HZ=100.
> 
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>

All looks good in the patchset from 10000 feet (or more), but I need Ben to
speak here.

Thanks,
Rafael


> ---
>  drivers/cpufreq/pmac64-cpufreq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
> index 7ba4234..674807d 100644
> --- a/drivers/cpufreq/pmac64-cpufreq.c
> +++ b/drivers/cpufreq/pmac64-cpufreq.c
> @@ -141,7 +141,7 @@ static void g5_vdnap_switch_volt(int speed_mode)
>  		pmf_call_one(pfunc_vdnap0_complete, &args);
>  		if (done)
>  			break;
> -		msleep(1);
> +		usleep_range(1000, 1000);
>  	}
>  	if (done == 0)
>  		printk(KERN_WARNING "cpufreq: Timeout in clock slewing !\n");
> @@ -240,7 +240,7 @@ static void g5_pfunc_switch_volt(int speed_mode)
>  		if (pfunc_cpu1_volt_low)
>  			pmf_call_one(pfunc_cpu1_volt_low, NULL);
>  	}
> -	msleep(10); /* should be faster , to fix */
> +	usleep_range(10000, 10000); /* should be faster , to fix */
>  }
>  
>  /*
> @@ -285,7 +285,7 @@ static int g5_pfunc_switch_freq(int speed_mode)
>  		pmf_call_one(pfunc_slewing_done, &args);
>  		if (done)
>  			break;
> -		msleep(1);
> +		usleep_range(500, 500);
>  	}
>  	if (done == 0)
>  		printk(KERN_WARNING "cpufreq: Timeout in clock slewing !\n");
>
viresh kumar - July 24, 2013, 5:32 a.m.
On 24 July 2013 01:54, Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> Some functions on switch path use msleep() which is inaccurate, and
> depends on HZ. With HZ=100 msleep(1) takes actually over ten times longer.
> Using usleep_range() we get more accurate sleeps.
>
> I measured the "pfunc_slewing_done" polling to take 300us at max (on
> 2.3GHz dual-processor Xserve G5), so using 500us sleep there should
> be fine.
>
> With the patch, g5_switch_freq() duration drops from ~50ms to ~10ms on
> Xserve with HZ=100.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> ---
>  drivers/cpufreq/pmac64-cpufreq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Looks fine to me as well..

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Benjamin Herrenschmidt - Aug. 12, 2013, 1:07 a.m.
On Wed, 2013-07-24 at 07:14 +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2013-07-23 at 23:20 +0200, Rafael J. Wysocki wrote:
> > All looks good in the patchset from 10000 feet (or more), but I need
> > Ben to speak here.
> 
> I want to give it a quick spin on the HW here, I'll ack then. But yes,
> it looks good.

Seems to work here on the quad G5.

However, If I use on-demand, there's a huge latency of switch as far as
I can tell (about 10s) after I start/stop a bunch of CPU eaters... I
quite like how the userspace "powernowd" which I used to use switches
more aggressively.

Is that expected ?

Cheers,
Ben.
Aaro Koskinen - Aug. 15, 2013, 8:10 p.m.
Hi,

On Mon, Aug 12, 2013 at 11:07:48AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2013-07-24 at 07:14 +1000, Benjamin Herrenschmidt wrote:
> > On Tue, 2013-07-23 at 23:20 +0200, Rafael J. Wysocki wrote:
> > > All looks good in the patchset from 10000 feet (or more), but I need
> > > Ben to speak here.
> > 
> > I want to give it a quick spin on the HW here, I'll ack then. But yes,
> > it looks good.
> 
> Seems to work here on the quad G5.
> 
> However, If I use on-demand, there's a huge latency of switch as far as
> I can tell (about 10s) after I start/stop a bunch of CPU eaters... I
> quite like how the userspace "powernowd" which I used to use switches
> more aggressively.
> 
> Is that expected ?

I guess we should keep the current 12us latency in g5_neo2_cpufreq_init()
(although I doubt it's correct...), and only add the new 10ms latency
value to g5_pm72_cpufreq_init() - that way we can enable the older systems
to use ondemand (despite long latencies), while not risking regressing
any of the current functionality.

I'll resend the series with the changes.

A.
Benjamin Herrenschmidt - Aug. 15, 2013, 10:14 p.m.
On Thu, 2013-08-15 at 23:10 +0300, Aaro Koskinen wrote:

> I guess we should keep the current 12us latency in g5_neo2_cpufreq_init()
> (although I doubt it's correct...), and only add the new 10ms latency
> value to g5_pm72_cpufreq_init() - that way we can enable the older systems
> to use ondemand (despite long latencies), while not risking regressing
> any of the current functionality.
> 
> I'll resend the series with the changes.

Well, mine is a 11,2, it's not a neo2.

I suppose we could try to measure the latency :-)

Cheers,
Ben.

Patch

diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
index 7ba4234..674807d 100644
--- a/drivers/cpufreq/pmac64-cpufreq.c
+++ b/drivers/cpufreq/pmac64-cpufreq.c
@@ -141,7 +141,7 @@  static void g5_vdnap_switch_volt(int speed_mode)
 		pmf_call_one(pfunc_vdnap0_complete, &args);
 		if (done)
 			break;
-		msleep(1);
+		usleep_range(1000, 1000);
 	}
 	if (done == 0)
 		printk(KERN_WARNING "cpufreq: Timeout in clock slewing !\n");
@@ -240,7 +240,7 @@  static void g5_pfunc_switch_volt(int speed_mode)
 		if (pfunc_cpu1_volt_low)
 			pmf_call_one(pfunc_cpu1_volt_low, NULL);
 	}
-	msleep(10); /* should be faster , to fix */
+	usleep_range(10000, 10000); /* should be faster , to fix */
 }
 
 /*
@@ -285,7 +285,7 @@  static int g5_pfunc_switch_freq(int speed_mode)
 		pmf_call_one(pfunc_slewing_done, &args);
 		if (done)
 			break;
-		msleep(1);
+		usleep_range(500, 500);
 	}
 	if (done == 0)
 		printk(KERN_WARNING "cpufreq: Timeout in clock slewing !\n");