| Message ID | 1472114562-2736-4-git-send-email-alex.shi@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, Is there any concern on this patch? Regards Alex On 08/25/2016 04:42 PM, Alex Shi wrote: > Kernel or user may have special requirement on cpu response time, like > if a interrupt is pinned to a cpu, we don't want the cpu goes too deep > sleep. This patch can prevent this thing happen by consider per cpu > resume_latency setting in cpu sleep state selection in menu governor. > > The pm_qos_resume_latency ask device to give reponse in this time. That's > similar with cpu cstates' entry_latency + exit_latency. But since > most of cpu cstate either has no entry_latency or add it into exit_latency > So, we just can restrict this time requirement as states exit_latency. > > The 0 value of pm_qos_resume_latency is for no limitation according ABI > definition. So set the value 1 could get the 0 latency if it's needed. > > Signed-off-by: Alex Shi <alex.shi@linaro.org> > To: linux-kernel@vger.kernel.org > Cc: linux-pm@vger.kernel.org > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Alex Shi <alex.shi@linaro.org> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Cc: Arjan van de Ven <arjan@linux.intel.com> > Cc: Rik van Riel <riel@redhat.com> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > --- > drivers/cpuidle/governors/menu.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c > index bb58e2a..e354880 100644 > --- a/drivers/cpuidle/governors/menu.c > +++ b/drivers/cpuidle/governors/menu.c > @@ -12,6 +12,7 @@ > > #include <linux/kernel.h> > #include <linux/cpuidle.h> > +#include <linux/cpu.h> > #include <linux/pm_qos.h> > #include <linux/time.h> > #include <linux/ktime.h> > @@ -281,17 +282,23 @@ again: > 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); > > if (data->needs_update) { > menu_update(drv, dev); > data->needs_update = 0; > } > > + /* resume_latency is 0 means no restriction */ > + if (resume_latency && resume_latency < latency_req) > + latency_req = resume_latency; > + > /* Special case when user has set very strict latency requirement */ > if (unlikely(latency_req == 0)) > return 0; >
Hi Daniel & Rafael, Any comments on this patch? On 08/25/2016 04:42 PM, Alex Shi wrote: > Kernel or user may have special requirement on cpu response time, like > if a interrupt is pinned to a cpu, we don't want the cpu goes too deep > sleep. This patch can prevent this thing happen by consider per cpu > resume_latency setting in cpu sleep state selection in menu governor. > > The pm_qos_resume_latency ask device to give reponse in this time. That's > similar with cpu cstates' entry_latency + exit_latency. But since > most of cpu cstate either has no entry_latency or add it into exit_latency > So, we just can restrict this time requirement as states exit_latency. > > The 0 value of pm_qos_resume_latency is for no limitation according ABI > definition. So set the value 1 could get the 0 latency if it's needed. > > Signed-off-by: Alex Shi <alex.shi@linaro.org> > To: linux-kernel@vger.kernel.org > Cc: linux-pm@vger.kernel.org > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Alex Shi <alex.shi@linaro.org> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Cc: Arjan van de Ven <arjan@linux.intel.com> > Cc: Rik van Riel <riel@redhat.com> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > --- > drivers/cpuidle/governors/menu.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c > index bb58e2a..e354880 100644 > --- a/drivers/cpuidle/governors/menu.c > +++ b/drivers/cpuidle/governors/menu.c > @@ -12,6 +12,7 @@ > > #include <linux/kernel.h> > #include <linux/cpuidle.h> > +#include <linux/cpu.h> > #include <linux/pm_qos.h> > #include <linux/time.h> > #include <linux/ktime.h> > @@ -281,17 +282,23 @@ again: > 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); > > if (data->needs_update) { > menu_update(drv, dev); > data->needs_update = 0; > } > > + /* resume_latency is 0 means no restriction */ > + if (resume_latency && resume_latency < latency_req) > + latency_req = resume_latency; > + > /* Special case when user has set very strict latency requirement */ > if (unlikely(latency_req == 0)) > return 0; >
On 09/23/2016 09:36 AM, Rafael J. Wysocki wrote: > On 9/14/2016 10:28 AM, Vincent Guittot wrote: >> Hi Rafael, >> >> On 14 September 2016 at 00:10, Rafael J. Wysocki <rjw@rjwysocki.net> >> wrote: >>> On Tuesday, September 13, 2016 10:02:49 PM Alex Shi wrote: >>>> Hi Daniel & Rafael, >>>> >>>> Any comments on this patch? >>> I actually am not sure about the whole series. >>> >>> I know your motivation, but honestly the changes here may not be the >>> best way >>> to achieve what you need. Is there some idea for better way? >>> >>> You may think that the changes are trivial, but in fact they are >>> not. There >>> are side effects and I'm not sure about the resulting user space >>> interface >>> at all. What's concern is abuse of user interface? If the request come from user level, is there other ways to set a value for this? >> This patchset has got 2 parts: >> - one part is about taking into account per-device resume latency >> constraint when selecting the idle state of a CPU. This value can >> already be set by kernel (even if it's probably not done yet) but this >> constraint is never taken into account >> - the other part is about exposing the resume latency to userspace. >> This part might raise more discussion but I see one example that could >> take advantage of this. When you have several clusters of CPUs and you >> want to dedicate some CPUs to latency sensitive activity and prevent >> deep sleep state on these CPUs but you want to let the other CPUs >> using all C-state > > The first very basic question about this I have is whether or not the > device PM QoS mechanism is suitable for the task at hand at all. > > It certainly hasn't been invented with it in mind. > Look though the device PM QoS, this kind of usage seems sensible. So why not? Regards Alex
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c index bb58e2a..e354880 100644 --- a/drivers/cpuidle/governors/menu.c +++ b/drivers/cpuidle/governors/menu.c @@ -12,6 +12,7 @@ #include <linux/kernel.h> #include <linux/cpuidle.h> +#include <linux/cpu.h> #include <linux/pm_qos.h> #include <linux/time.h> #include <linux/ktime.h> @@ -281,17 +282,23 @@ again: 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); if (data->needs_update) { menu_update(drv, dev); data->needs_update = 0; } + /* resume_latency is 0 means no restriction */ + if (resume_latency && resume_latency < latency_req) + latency_req = resume_latency; + /* Special case when user has set very strict latency requirement */ if (unlikely(latency_req == 0)) return 0;
Kernel or user may have special requirement on cpu response time, like if a interrupt is pinned to a cpu, we don't want the cpu goes too deep sleep. This patch can prevent this thing happen by consider per cpu resume_latency setting in cpu sleep state selection in menu governor. The pm_qos_resume_latency ask device to give reponse in this time. That's similar with cpu cstates' entry_latency + exit_latency. But since most of cpu cstate either has no entry_latency or add it into exit_latency So, we just can restrict this time requirement as states exit_latency. The 0 value of pm_qos_resume_latency is for no limitation according ABI definition. So set the value 1 could get the 0 latency if it's needed. Signed-off-by: Alex Shi <alex.shi@linaro.org> To: linux-kernel@vger.kernel.org Cc: linux-pm@vger.kernel.org Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> Cc: Alex Shi <alex.shi@linaro.org> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Arjan van de Ven <arjan@linux.intel.com> Cc: Rik van Riel <riel@redhat.com> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> --- drivers/cpuidle/governors/menu.c | 7 +++++++ 1 file changed, 7 insertions(+) -- 2.8.1.101.g72d917a