| Message ID | 1472114562-2736-4-git-send-email-alex.shi@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| 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; > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > 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. 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 Regards, Vincent > > Thanks, > Rafael > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. >> >> 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. > 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. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(+)