* Re: 9908859acaa9 cpuidle/menu: add per CPU PM QoS resume latency consideration [not found] ` <e8261472-4338-eab2-98e1-5dd6039899f7@linaro.org> @ 2017-02-23 22:34 ` Rafael J. Wysocki 2017-02-24 1:49 ` Alex Shi 0 siblings, 1 reply; 3+ messages in thread From: Rafael J. Wysocki @ 2017-02-23 22:34 UTC (permalink / raw) To: Alex Shi; +Cc: Peter Zijlstra, Mike Galbraith, LKML, Rik van Riel, Linux PM 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: 9908859acaa9 cpuidle/menu: add per CPU PM QoS resume latency consideration 2017-02-23 22:34 ` 9908859acaa9 cpuidle/menu: add per CPU PM QoS resume latency consideration Rafael J. Wysocki @ 2017-02-24 1:49 ` Alex Shi 2017-02-24 12:25 ` [PATCH] cpuidle: menu: Avoid taking spinlock for accessing QoS values Rafael J. Wysocki 0 siblings, 1 reply; 3+ messages in thread From: Alex Shi @ 2017-02-24 1:49 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Peter Zijlstra, Mike Galbraith, LKML, Rik van Riel, Linux PM > 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] cpuidle: menu: Avoid taking spinlock for accessing QoS values 2017-02-24 1:49 ` Alex Shi @ 2017-02-24 12:25 ` Rafael J. Wysocki 0 siblings, 0 replies; 3+ messages in thread From: Rafael J. Wysocki @ 2017-02-24 12:25 UTC (permalink / raw) To: Alex Shi, Linux PM; +Cc: Peter Zijlstra, Mike Galbraith, LKML, Rik van Riel 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-02-24 12:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1487768197.27533.5.camel@gmx.de>
[not found] ` <2050688.jCOqd8xbcD@aspire.rjw.lan>
[not found] ` <e8261472-4338-eab2-98e1-5dd6039899f7@linaro.org>
2017-02-23 22:34 ` 9908859acaa9 cpuidle/menu: add per CPU PM QoS resume latency consideration Rafael J. Wysocki
2017-02-24 1:49 ` Alex Shi
2017-02-24 12:25 ` [PATCH] cpuidle: menu: Avoid taking spinlock for accessing QoS values Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).