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

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

[Mike Galbraith]

Hi,

Do we really need a spinlock for that in the idle loop?

	-Mike

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

[Peter Zijlstra]

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 :/

Nasty code that..

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

[Mike Galbraith]

On Wed, 2017-02-22 at 14:12 +0100, 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.

That's what made me notice the obnoxious little bugger.

[   77.608340] BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:995
[   77.608342] in_atomic(): 1, irqs_disabled(): 1, pid: 0, name: swapper/1
[   77.608343] INFO: lockdep is turned off.
[   77.608344] irq event stamp: 59222
[   77.608353] hardirqs last  enabled at (59221): [<ffffffff81105a1f>] rcu_idle_exit+0x2f/0x50
[   77.608362] hardirqs last disabled at (59222): [<ffffffff810d4f1a>] do_idle+0x9a/0x290
[   77.608372] softirqs last  enabled at (0): [<ffffffff8107b8f1>] copy_process.part.34+0x5f1/0x22a0
[   77.608374] softirqs last disabled at (0): [<          (null)>]           (null)
[   77.608374] Preemption disabled at:
[   77.608383] [<ffffffff817282b2>] schedule_preempt_disabled+0x22/0x30
[   77.608387] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G        W   E   4.11.0-rt9-rt #163
[   77.608389] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRHSXSD1.86B.0056.R01.1409242327 09/24/2014
[   77.608390] Call Trace:
[   77.608399]  dump_stack+0x85/0xc8
[   77.608405]  ___might_sleep+0x15d/0x260
[   77.608409]  rt_spin_lock+0x24/0x80
[   77.608419]  dev_pm_qos_read_value+0x1e/0x40
[   77.608424]  menu_select+0x56/0x3e0
[   77.608426]  ? rcu_eqs_enter_common.isra.40+0x9d/0x160
[   77.608435]  cpuidle_select+0x13/0x20
[   77.608438]  do_idle+0x182/0x290
[   77.608445]  cpu_startup_entry+0x48/0x50
[   77.608450]  start_secondary+0x133/0x160
[   77.608453]  start_cpu+0x14/0x14

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

[Alex Shi]

On 02/22/2017 09:19 PM, Mike Galbraith wrote:
> On Wed, 2017-02-22 at 14:12 +0100, 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.
> 
> That's what made me notice the obnoxious little bugger.

Hi Mike,

Sorry for this bug. Guess the rt_spin_lock call some function in
rtmutex.c:995. Could you like to show me how to reproduce this bug?

> 
> [   77.608340] BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:995
> [   77.608342] in_atomic(): 1, irqs_disabled(): 1, pid: 0, name: swapper/1
> [   77.608343] INFO: lockdep is turned off.
> [   77.608344] irq event stamp: 59222
> [   77.608353] hardirqs last  enabled at (59221): [<ffffffff81105a1f>] rcu_idle_exit+0x2f/0x50
> [   77.608362] hardirqs last disabled at (59222): [<ffffffff810d4f1a>] do_idle+0x9a/0x290
> [   77.608372] softirqs last  enabled at (0): [<ffffffff8107b8f1>] copy_process.part.34+0x5f1/0x22a0
> [   77.608374] softirqs last disabled at (0): [<          (null)>]           (null)
> [   77.608374] Preemption disabled at:
> [   77.608383] [<ffffffff817282b2>] schedule_preempt_disabled+0x22/0x30
> [   77.608387] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G        W   E   4.11.0-rt9-rt #163
> [   77.608389] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRHSXSD1.86B.0056.R01.1409242327 09/24/2014
> [   77.608390] Call Trace:
> [   77.608399]  dump_stack+0x85/0xc8
> [   77.608405]  ___might_sleep+0x15d/0x260
> [   77.608409]  rt_spin_lock+0x24/0x80
> [   77.608419]  dev_pm_qos_read_value+0x1e/0x40
> [   77.608424]  menu_select+0x56/0x3e0
> [   77.608426]  ? rcu_eqs_enter_common.isra.40+0x9d/0x160
> [   77.608435]  cpuidle_select+0x13/0x20
> [   77.608438]  do_idle+0x182/0x290
> [   77.608445]  cpu_startup_entry+0x48/0x50
> [   77.608450]  start_secondary+0x133/0x160
> [   77.608453]  start_cpu+0x14/0x14
> 

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

[Peter Zijlstra]

On Wed, Feb 22, 2017 at 10:31:26PM +0800, Alex Shi wrote:
> 
> 
> On 02/22/2017 09:19 PM, Mike Galbraith wrote:
> > On Wed, 2017-02-22 at 14:12 +0100, 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.
> > 
> > That's what made me notice the obnoxious little bugger.
> 
> Hi Mike,
> 
> Sorry for this bug. Guess the rt_spin_lock call some function in
> rtmutex.c:995. Could you like to show me how to reproduce this bug?

Its not hard; spinlock_t ends up being a mutex, and this is ran from the
idle thread. What thread do you think we ought to run when we block
idle?

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

[Alex Shi]

> 
> Its not hard; spinlock_t ends up being a mutex, and this is ran from the
> idle thread. What thread do you think we ought to run when we block
> idle?
> 

Straight right.
Thanks for explanations! :)

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

[Rafael J. Wysocki]

On Wednesday, February 22, 2017 10:55:04 PM Alex Shi wrote:
> > 
> > Its not hard; spinlock_t ends up being a mutex, and this is ran from the
> > idle thread. What thread do you think we ought to run when we block
> > idle?
> > 
> 
> Straight right.
> Thanks for explanations! :)

I overlooked that, sorry.

Shall we revert?

I don't want RT to be broken because of this.

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

[Mike Galbraith]

On Thu, 2017-02-23 at 13:15 +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 22, 2017 10:55:04 PM Alex Shi wrote:
> > > 
> > > Its not hard; spinlock_t ends up being a mutex, and this is ran
> > > from the
> > > idle thread. What thread do you think we ought to run when we
> > > block
> > > idle?
> > > 
> > 
> > Straight right.
> > Thanks for explanations! :)
> 
> I overlooked that, sorry.
> 
> Shall we revert?
> 
> I don't want RT to be broken because of this.

Just whacking the lock would take care of that.  The question is who is
gonna use this, and what does it really buy them?  When I look at that
commit, an eyebrow raises, lock or no lock.

	-Mike

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

[Alex Shi]

On 02/23/2017 09:08 PM, Mike Galbraith wrote:
> On Thu, 2017-02-23 at 13:15 +0100, Rafael J. Wysocki wrote:
>> On Wednesday, February 22, 2017 10:55:04 PM Alex Shi wrote:
>>>>
>>>> Its not hard; spinlock_t ends up being a mutex, and this is ran
>>>> from the
>>>> idle thread. What thread do you think we ought to run when we
>>>> block
>>>> idle?
>>>>
>>>
>>> Straight right.
>>> Thanks for explanations! :)
>>
>> I overlooked that, sorry.
>>
>> Shall we revert?
>>
>> I don't want RT to be broken because of this.
> 
> Just whacking the lock would take care of that.  The question is who is
> gonna use this, and what does it really buy them?  When I look at that
> commit, an eyebrow raises, lock or no lock.
> 

Right per cpu lock for per cpu data is unnecessary.

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

[Alex Shi]

On 02/23/2017 08:15 PM, Rafael J. Wysocki wrote:
> On Wednesday, February 22, 2017 10:55:04 PM Alex Shi wrote:
>>>
>>> Its not hard; spinlock_t ends up being a mutex, and this is ran from the
>>> idle thread. What thread do you think we ought to run when we block
>>> idle?
>>>
>>
>> Straight right.
>> Thanks for explanations! :)
> 
> I overlooked that, sorry.
> 
> Shall we revert?
> 
> I don't want RT to be broken because of this.
> 

The RT kernel had a fix already. :)
This feature is very useful to save power of cpu. We could have a better fix
to keep this feature and good for RT.


>From cfe82c555628f7197ecd91d3b8092a98b34a371a Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@linaro.org>
Date: Thu, 23 Feb 2017 21:27:09 +0800
Subject: [PATCH] cpuidle/menu: skip lock in per cpu resume latency reading

dev_pm_qos_read_value using a lock to proctect the concurrent device
latency reading, that is useful for multiple cpu access a global device.
But it's not necessary for a per cpu data reading here. And furthermore,
RT kernel using mutex to replace spinlock causes fake panic here.

So, skip the lock using is nice for this per cpu value reading.

Signed-off-by: Alex Shi <alex.shi@linaro.org>
---
 drivers/cpuidle/governors/menu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 8d6d25c..b852d99 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_per_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_per_cpu_resume_latency(dev->cpu);
 
 	if (data->needs_update) {
 		menu_update(drv, dev);
-- 
2.8.1.101.g72d917a

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

[Rafael J. Wysocki]

On Thursday, February 23, 2017 09:55:17 PM Alex Shi wrote:
> 
> On 02/23/2017 08:15 PM, Rafael J. Wysocki wrote:
> > On Wednesday, February 22, 2017 10:55:04 PM Alex Shi wrote:
> >>>
> >>> Its not hard; spinlock_t ends up being a mutex, and this is ran from the
> >>> idle thread. What thread do you think we ought to run when we block
> >>> idle?
> >>>
> >>
> >> Straight right.
> >> Thanks for explanations! :)
> > 
> > I overlooked that, sorry.
> > 
> > Shall we revert?
> > 
> > I don't want RT to be broken because of this.
> > 
> 
> The RT kernel had a fix already. :)
> This feature is very useful to save power of cpu. We could have a better fix
> to keep this feature and good for RT.

Well, first, please submit this properly (with a proper subject and CC to linux-pm)
if I'm expected to apply it.

Second ->

> From cfe82c555628f7197ecd91d3b8092a98b34a371a Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@linaro.org>
> Date: Thu, 23 Feb 2017 21:27:09 +0800
> Subject: [PATCH] cpuidle/menu: skip lock in per cpu resume latency reading
> 
> dev_pm_qos_read_value using a lock to proctect the concurrent device
> latency reading, that is useful for multiple cpu access a global device.
> But it's not necessary for a per cpu data reading here. And furthermore,
> RT kernel using mutex to replace spinlock causes fake panic here.
> 
> So, skip the lock using is nice for this per cpu value reading.
> 
> Signed-off-by: Alex Shi <alex.shi@linaro.org>
> ---
>  drivers/cpuidle/governors/menu.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 8d6d25c..b852d99 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_per_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);

-> This essentially duplicates __dev_pm_qos_read_value() except for the lockdep assertion.

What about the (untested) patch below instead?

Thanks,
Rafael


---
 drivers/base/power/qos.c         |    3 +--
 drivers/cpuidle/governors/menu.c |    2 +-
 include/linux/pm_qos.h           |    7 +++++++
 3 files changed, 9 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/base/power/qos.c
===================================================================
--- linux-pm.orig/drivers/base/power/qos.c
+++ linux-pm/drivers/base/power/qos.c
@@ -108,8 +108,7 @@ s32 __dev_pm_qos_read_value(struct devic
 {
 	lockdep_assert_held(&dev->power.lock);
 
-	return IS_ERR_OR_NULL(dev->power.qos) ?
-		0 : pm_qos_read_value(&dev->power.qos->resume_latency);
+	return dev_pm_qos_raw_read_value(dev);
 }
 
 /**
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -287,7 +287,7 @@ static int menu_select(struct cpuidle_dr
 	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 = dev_pm_qos_raw_read_value(device);
 
 	if (data->needs_update) {
 		menu_update(drv, dev);
Index: linux-pm/include/linux/pm_qos.h
===================================================================
--- linux-pm.orig/include/linux/pm_qos.h
+++ linux-pm/include/linux/pm_qos.h
@@ -172,6 +172,12 @@ static inline s32 dev_pm_qos_requested_f
 {
 	return dev->power.qos->flags_req->data.flr.flags;
 }
+
+static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
+{
+	return IS_ERR_OR_NULL(dev->power.qos) ?
+		0 : pm_qos_read_value(&dev->power.qos->resume_latency);
+}
 #else
 static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
 							  s32 mask)
@@ -236,6 +242,7 @@ static inline void dev_pm_qos_hide_laten
 
 static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev) { return 0; }
 static inline s32 dev_pm_qos_requested_flags(struct device *dev) { return 0; }
+static inline s32 dev_pm_qos_raw_read_value(struct device *dev) { return 0; }
 #endif
 
 #endif

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

[Alex Shi]

> Well, first, please submit this properly (with a proper subject and CC to linux-pm)
> if I'm expected to apply it.
> 

Hi Rafael,

Thanks for reminder!
A raw read function looks better. And maybe useful for others.

Acked-by: Alex Shi <alex.shi@linaro.org>

> 
> 
> 
> ---
>  drivers/base/power/qos.c         |    3 +--
>  drivers/cpuidle/governors/menu.c |    2 +-
>  include/linux/pm_qos.h           |    7 +++++++
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/base/power/qos.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/qos.c
> +++ linux-pm/drivers/base/power/qos.c
> @@ -108,8 +108,7 @@ s32 __dev_pm_qos_read_value(struct devic
>  {
>  	lockdep_assert_held(&dev->power.lock);
>  
> -	return IS_ERR_OR_NULL(dev->power.qos) ?
> -		0 : pm_qos_read_value(&dev->power.qos->resume_latency);
> +	return dev_pm_qos_raw_read_value(dev);
>  }
>  
>  /**
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -287,7 +287,7 @@ static int menu_select(struct cpuidle_dr
>  	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 = dev_pm_qos_raw_read_value(device);
>  
>  	if (data->needs_update) {
>  		menu_update(drv, dev);
> Index: linux-pm/include/linux/pm_qos.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_qos.h
> +++ linux-pm/include/linux/pm_qos.h
> @@ -172,6 +172,12 @@ static inline s32 dev_pm_qos_requested_f
>  {
>  	return dev->power.qos->flags_req->data.flr.flags;
>  }
> +
> +static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
> +{
> +	return IS_ERR_OR_NULL(dev->power.qos) ?
> +		0 : pm_qos_read_value(&dev->power.qos->resume_latency);
> +}
>  #else
>  static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
>  							  s32 mask)
> @@ -236,6 +242,7 @@ static inline void dev_pm_qos_hide_laten
>  
>  static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev) { return 0; }
>  static inline s32 dev_pm_qos_requested_flags(struct device *dev) { return 0; }
> +static inline s32 dev_pm_qos_raw_read_value(struct device *dev) { return 0; }
>  #endif
>  
>  #endif
> 

[PATCH] cpuidle: menu: Avoid taking spinlock for accessing QoS values

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit 9908859acaa9 (cpuidle/menu: add per CPU PM QoS resume
latency consideration) the cpuidle menu governor calls
dev_pm_qos_read_value() on CPU devices to read the current resume
latency QoS constraint values for them.  That function takes a spinlock
to prevent the device's power.qos pointer from becoming NULL during
the access which is a problem for the RT patchset where spinlocks are
converted into mutexes and the idle loop stops working.

However, it is not even necessary for the menu governor to take
that spinlock, because the power.qos pointer accessed under it
cannot be modified during the access anyway.

For this reason, introduce a "raw" routine for accessing device
QoS resume latency constraints without locking and use it in the
menu governor.

Fixes: 9908859acaa9 (cpuidle/menu: add per CPU PM QoS resume latency consideration)
Acked-by: Alex Shi <alex.shi@linaro.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

OK

This doesn't break my test machine outright, so here it goes officially.

Thanks,
Rafael

---
 drivers/base/power/qos.c         |    3 +--
 drivers/cpuidle/governors/menu.c |    2 +-
 include/linux/pm_qos.h           |    7 +++++++
 3 files changed, 9 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/base/power/qos.c
===================================================================
--- linux-pm.orig/drivers/base/power/qos.c
+++ linux-pm/drivers/base/power/qos.c
@@ -108,8 +108,7 @@ s32 __dev_pm_qos_read_value(struct devic
 {
 	lockdep_assert_held(&dev->power.lock);
 
-	return IS_ERR_OR_NULL(dev->power.qos) ?
-		0 : pm_qos_read_value(&dev->power.qos->resume_latency);
+	return dev_pm_qos_raw_read_value(dev);
 }
 
 /**
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -287,7 +287,7 @@ static int menu_select(struct cpuidle_dr
 	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 = dev_pm_qos_raw_read_value(device);
 
 	if (data->needs_update) {
 		menu_update(drv, dev);
Index: linux-pm/include/linux/pm_qos.h
===================================================================
--- linux-pm.orig/include/linux/pm_qos.h
+++ linux-pm/include/linux/pm_qos.h
@@ -172,6 +172,12 @@ static inline s32 dev_pm_qos_requested_f
 {
 	return dev->power.qos->flags_req->data.flr.flags;
 }
+
+static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
+{
+	return IS_ERR_OR_NULL(dev->power.qos) ?
+		0 : pm_qos_read_value(&dev->power.qos->resume_latency);
+}
 #else
 static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
 							  s32 mask)
@@ -236,6 +242,7 @@ static inline void dev_pm_qos_hide_laten
 
 static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev) { return 0; }
 static inline s32 dev_pm_qos_requested_flags(struct device *dev) { return 0; }
+static inline s32 dev_pm_qos_raw_read_value(struct device *dev) { return 0; }
 #endif
 
 #endif

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

[Mike Galbraith]

On Wed, 2017-02-22 at 22:31 +0800, Alex Shi wrote:
> 
> On 02/22/2017 09:19 PM, Mike Galbraith wrote:
> > On Wed, 2017-02-22 at 14:12 +0100, 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.
> > 
> > That's what made me notice the obnoxious little bugger.
> 
> Hi Mike,
> 
> Sorry for this bug. Guess the rt_spin_lock call some function in
> rtmutex.c:995. Could you like to show me how to reproduce this bug?

Yeah, spinlock_t is transmogrified into an rtmutex in RT trees.  No
need to reproduce, if you just make the lock go away, RT trees will
become happy camper again, and NONRT trees won't have to feed yet
another an cycle sucking lock in the fastpath.

	-Mike

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

[Alex Shi]

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?

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

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

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

[Alex Shi]

On 02/22/2017 11:03 PM, Mike Galbraith wrote:
>> > 
>> > 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.
> 


Sorry. Mike.
What you mean of 'took the zero added cycles option'? :)

Regards
Alex

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

[Mike Galbraith]

On Wed, 2017-02-22 at 23:36 +0800, Alex Shi wrote:

> Sorry. Mike.
> What you mean of 'took the zero added cycles option'? :)

#ifndef CONFIG_PREEMPT_RT_FULL
...
#endif

I waved my magic ifdef wand, and poof, they disappeared :)

	-Mike