Re: 9908859acaa9 cpuidle/menu: add per CPU PM QoS resume latency consideration

[Mike Galbraith]

On Wed, 2017-02-22 at 22:53 +0800, Alex Shi wrote:
> cc Rafael.
> 
> 
> On 02/22/2017 09:12 PM, Peter Zijlstra wrote:
> > On Wed, Feb 22, 2017 at 01:56:37PM +0100, Mike Galbraith wrote:
> > > Hi,
> > > 
> > > Do we really need a spinlock for that in the idle loop?
> > 
> > Urgh, that's broken on RT, you cannot schedule the idle loop.
> > 
> > Also, yeah, reading a s32 should not need no locking, but there's a
> > bunch of pointer chases in between :/
> 
> Do you mean s/should not/should/ ? :)
> 
> Yes, the dev_pm_qos_read_value() using a power.lock, that is right for normal device. 
> But as to this cpu here, the lock isn't necessary.
> 
> Hi Rafael,
> Is this fix ok?

That's what I was gonna do, but then figured RT users will take full
control when it really matters, so took the zero added cycles option
for RT instead.

> ===========
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 8d6d25c..957c56d 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -273,6 +273,14 @@ static unsigned int get_typical_interval(struct menu_device *data)
>  > 	> goto again;
>  }
>  
> +int read_this_cpu_resume_latency(int cpu)
> +{
> +> 	> struct device *dev = get_cpu_device(cpu);
> +
> +> 	> return IS_ERR_OR_NULL(dev->power.qos) ?
> +> 	> 	> 0 : pm_qos_read_value(&dev->power.qos->resume_latency);
> +}
> +
>  /**
>   * menu_select - selects the next idle state to enter
>   * @drv: cpuidle driver containing state data
> @@ -281,13 +289,12 @@ static unsigned int get_typical_interval(struct menu_device *data)
>  static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  {
>  > 	> struct menu_device *data = this_cpu_ptr(&menu_devices);
> -> 	> struct device *device = get_cpu_device(dev->cpu);
>  > 	> int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
>  > 	> int i;
>  > 	> unsigned int interactivity_req;
>  > 	> unsigned int expected_interval;
>  > 	> unsigned long nr_iowaiters, cpu_load;
> -> 	> int resume_latency = dev_pm_qos_read_value(device);
> +> 	> int resume_latency = read_this_cpu_resume_latency(dev->cpu);
>  
>  > 	> if (data->needs_update) {
>  > 	> 	> menu_update(drv, dev);