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

Submitted by Aaro Koskinen on July 23, 2013, 8:24 p.m.

Details

Message ID 1374611079-28091-1-git-send-email-aaro.koskinen@iki.fi
State Superseded
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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");