Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560406 > unrolled thread

Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency consideration

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency  consideration Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-01-17 10:50 +0100
    Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency  consideration Alex Shi <alex.shi@linaro.org> - 2017-01-19 10:40 +0100
      Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency  consideration Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-01-19 11:30 +0100
        Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency consideration "Rafael J. Wysocki" <rafael@kernel.org> - 2017-01-19 22:50 +0100
          Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency  consideration Alex Shi <alex.shi@linaro.org> - 2017-01-20 09:40 +0100
          Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency  consideration Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-01-20 12:00 +0100
            Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency  consideration Alex Shi <alex.shi@linaro.org> - 2017-01-22 02:40 +0100
              Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency  consideration Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-01-23 14:00 +0100
                Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency  consideration Alex Shi <alex.shi@linaro.org> - 2017-01-23 16:00 +0100

#1560406 — Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency consideration

On Thu, Jan 12, 2017 at 09:27:04PM +0800, 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.
> 
> We can set a wanted latency value according to the value of
> /sys/devices/system/cpu/cpuX/cpuidle/stateX/latency. to just a bit
> less than related state's latency value. Then cpu can get to this state
> or higher.
> 
> 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: 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 07e36bb..8d6d25c 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -19,6 +19,7 @@
>  #include <linux/tick.h>
>  #include <linux/sched.h>
>  #include <linux/math64.h>
> +#include <linux/cpu.h>
>  
>  /*
>   * Please note when changing the tuning values:
> @@ -280,17 +281,23 @@ 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);
>  
>  	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;
> +

Calling dev_pm_qos_read_value() after checking latency_req is different from
zero would make more sense. If a zero latency is expected, no need to add an
overhead as we will return zero in all the cases.

	if (unlikely(latency_req == 0))
		return 0;

	device = get_cpu_device(dev->cpu);

	resume_latency = dev_pm_qos_read_value(device);
	if (resume_latency)
		latency_req = min(latency_req, resume_latency);

That said, I have the feeling that is taking the wrong direction. Each time we
are entering idle, we check the latencies. Entering idle can be done thousand
of times per second. Wouldn't make sense to disable the states not fulfilling
the constraints at the moment the latencies are changed ? As the idle states
have increasing exit latencies, setting an idle state limit to disable all
states after that limit may be more efficient than checking again and again in
the idle path, no ?

For example, a zero PM_QOS_CPU_DMA_LATENCY latency should prevent to enter the
select's idle routine.

>  	/* Special case when user has set very strict latency requirement */
>  	if (unlikely(latency_req == 0))
>  		return 0;
> -- 
> 2.8.1.101.g72d917a
> 

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [next] | [standalone]

#1562515

> That said, I have the feeling that is taking the wrong direction. Each time we
> are entering idle, we check the latencies. Entering idle can be done thousand
> of times per second. Wouldn't make sense to disable the states not fulfilling
> the constraints at the moment the latencies are changed ? As the idle states
> have increasing exit latencies, setting an idle state limit to disable all
> states after that limit may be more efficient than checking again and again in
> the idle path, no ?

You'r right. save some checking is good thing to do.


From 9e1cc3e02b8d954e606dd5a0f6466a8d5b3efab7 Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@linaro.org>
Date: Wed, 26 Oct 2016 15:26:22 +0800
Subject: [PATCH 2/2] cpuidle/menu: add per cpu pm_qos_resume_latency
 consideration

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.

We can set a wanted latency value according to the value of
/sys/devices/system/cpu/cpuX/cpuidle/stateX/latency. to just a bit
less than related state's latency value. Then cpu can get to this state
or higher.

Signed-off-by: Alex Shi <alex.shi@linaro.org>
Acked-by: Rik van Riel <riel@redhat.com>
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: 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/base/cpu.c               |  2 ++
 drivers/cpuidle/governors/menu.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 4c28e1a..2c3b359 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/cpufeature.h>
 #include <linux/tick.h>
+#include <linux/pm_qos.h>
 
 #include "base.h"
 
@@ -376,6 +377,7 @@ int register_cpu(struct cpu *cpu, int num)
 
 	per_cpu(cpu_sys_devices, num) = &cpu->dev;
 	register_cpu_under_node(num, cpu_to_node(num));
+	dev_pm_qos_expose_latency_limit(&cpu->dev, 0);
 
 	return 0;
 }
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 07e36bb..cc7d873 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -19,6 +19,7 @@
 #include <linux/tick.h>
 #include <linux/sched.h>
 #include <linux/math64.h>
+#include <linux/cpu.h>
 
 /*
  * Please note when changing the tuning values:
@@ -280,11 +281,13 @@ 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;
 	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;
 
 	if (data->needs_update) {
 		menu_update(drv, dev);
@@ -295,6 +298,13 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	if (unlikely(latency_req == 0))
 		return 0;
 
+	device = get_cpu_device(dev->cpu);
+
+	/* resume_latency is 0 means no restriction */
+	resume_latency = dev_pm_qos_read_value(device);
+	if (resume_latency)
+		latency_req = min(latency_req, resume_latency);
+
 	/* determine the expected residency time, round up */
 	data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length());
 
-- 
2.8.1.101.g72d917a

> 
> For example, a zero PM_QOS_CPU_DMA_LATENCY latency should prevent to enter the
> select's idle routine.

That's a good idea. I will give a draft change to review! :)

[toc] | [prev] | [next] | [standalone]

#1562572

On Thu, Jan 19, 2017 at 05:25:37PM +0800, Alex Shi wrote:
> 
> > That said, I have the feeling that is taking the wrong direction. Each time we
> > are entering idle, we check the latencies. Entering idle can be done thousand
> > of times per second. Wouldn't make sense to disable the states not fulfilling
> > the constraints at the moment the latencies are changed ? As the idle states
> > have increasing exit latencies, setting an idle state limit to disable all
> > states after that limit may be more efficient than checking again and again in
> > the idle path, no ?
> 
> You'r right. save some checking is good thing to do.

Hi Alex,

I think you missed the point.

What I am proposing is to change the current approach by disabling all the
states after a specific latency.

We add a specific internal function:

static int cpuidle_set_latency(struct cpuidle_driver *drv,
				struct cpuidle_device *dev,
				int latency)
{
	int i, idx;

	for (i = 0, idx = 0; i < drv->state_count; i++) {

		struct cpuidle_state *s = &drv->states[i];			

		if (s->latency > latency)
			break;

		idx = i;
	}

	dev->state_count = idx;

	return 0;
}

This function is called from the notifier callback:

static int cpuidle_latency_notify(struct notifier_block *b,
                unsigned long l, void *v)
 {
-       wake_up_all_idle_cpus();
+       struct cpuidle_device *dev;
+       struct cpuidle_driver *drv;
+
+       cpuidle_pause_and_lock();
+       for_each_possible_cpu(cpu) {
+               dev = &per_cpu(cpuidle_dev, cpu);
+               drv = = cpuidle_get_cpu_driver(dev);    
+               cpuidle_set_latency(drv, dev, l)
+       }
+       cpuidle_resume_and_unlock();
+
        return NOTIFY_OK;
 }

-----------------------------------------------------------------------------

The menu governor becomes:

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index bba3c2af..87e58e3 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -352,7 +352,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
         * Find the idle state with the lowest power while satisfying
         * our constraints.
         */
-       for (i = data->last_state_idx + 1; i < drv->state_count; i++) {
+       for (i = data->last_state_idx + 1; i < dev->state_count; i++) {
                struct cpuidle_state *s = &drv->states[i];
                struct cpuidle_state_usage *su = &dev->states_usage[i];


... with a cleanup around latency_req.

-----------------------------------------------------------------------------

And the cpuidle_device structure is changed to:

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index b923c32..2fc966cb 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -88,6 +88,7 @@ struct cpuidle_device {
        cpumask_t               coupled_cpus;
        struct cpuidle_coupled  *coupled;
 #endif
+       int state_count;
 };
 
 DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);


At init time, the drv->state_count and all cpu's dev->state_count are the same.

Well, that is the rough idea: instead of reading the latency when entering
idle, let's disable/enable the idle states when we set a new latency.

I did not check how that fits with the per cpu latency, but I think it will be
cleaner to change the approach rather than spreading latencies dances around.

[toc] | [prev] | [next] | [standalone]

#1563117 — Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency consideration

On Thu, Jan 19, 2017 at 11:21 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Thu, Jan 19, 2017 at 05:25:37PM +0800, Alex Shi wrote:
>>
>> > That said, I have the feeling that is taking the wrong direction. Each time we
>> > are entering idle, we check the latencies. Entering idle can be done thousand
>> > of times per second. Wouldn't make sense to disable the states not fulfilling
>> > the constraints at the moment the latencies are changed ? As the idle states
>> > have increasing exit latencies, setting an idle state limit to disable all
>> > states after that limit may be more efficient than checking again and again in
>> > the idle path, no ?
>>
>> You'r right. save some checking is good thing to do.
>
> Hi Alex,
>
> I think you missed the point.
>
> What I am proposing is to change the current approach by disabling all the
> states after a specific latency.
>
> We add a specific internal function:
>
> static int cpuidle_set_latency(struct cpuidle_driver *drv,
>                                 struct cpuidle_device *dev,
>                                 int latency)
> {
>         int i, idx;
>
>         for (i = 0, idx = 0; i < drv->state_count; i++) {
>
>                 struct cpuidle_state *s = &drv->states[i];
>
>                 if (s->latency > latency)
>                         break;
>
>                 idx = i;
>         }
>
>         dev->state_count = idx;
>
>         return 0;
> }
>
> This function is called from the notifier callback:
>
> static int cpuidle_latency_notify(struct notifier_block *b,
>                 unsigned long l, void *v)
>  {
> -       wake_up_all_idle_cpus();
> +       struct cpuidle_device *dev;
> +       struct cpuidle_driver *drv;
> +
> +       cpuidle_pause_and_lock();
> +       for_each_possible_cpu(cpu) {
> +               dev = &per_cpu(cpuidle_dev, cpu);
> +               drv = = cpuidle_get_cpu_driver(dev);
> +               cpuidle_set_latency(drv, dev, l)
> +       }
> +       cpuidle_resume_and_unlock();
> +
>         return NOTIFY_OK;
>  }

The above may be problematic if the constraints change relatively
often.  It is global and it will affect all of the CPUs in the system
every time and now think about systems with hundreds of them.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1563377

On 01/20/2017 05:43 AM, Rafael J. Wysocki wrote:
> The above may be problematic if the constraints change relatively
> often.  It is global and it will affect all of the CPUs in the system
> every time and now think about systems with hundreds of them.

Yes, the disadvantage is waking up all idle cpus when value changed. As
to the multi core concern, maybe a per cpu notifier way is better? But
that's another story of pm_qos...

So Rafael, any comments for this patch version?

Regards
Alex

[toc] | [prev] | [next] | [standalone]

#1563484

On Thu, Jan 19, 2017 at 10:43:23PM +0100, Rafael J. Wysocki wrote:

[ ... ]

> > This function is called from the notifier callback:
> >
> > static int cpuidle_latency_notify(struct notifier_block *b,
> >                 unsigned long l, void *v)
> >  {
> > -       wake_up_all_idle_cpus();
> > +       struct cpuidle_device *dev;
> > +       struct cpuidle_driver *drv;
> > +
> > +       cpuidle_pause_and_lock();
> > +       for_each_possible_cpu(cpu) {
> > +               dev = &per_cpu(cpuidle_dev, cpu);
> > +               drv = = cpuidle_get_cpu_driver(dev);
> > +               cpuidle_set_latency(drv, dev, l)
> > +       }
> > +       cpuidle_resume_and_unlock();
> > +
> >         return NOTIFY_OK;
> >  }
> 
> The above may be problematic if the constraints change relatively
> often.  It is global and it will affect all of the CPUs in the system
> every time and now think about systems with hundreds of them.

Yeah, that could be problematic. The code snippet gives the general idea but it
could be changed by for example by a flag telling the cpus when they enter idle
to update their state_count. Or something like that.

But if you think the patchset is fine, it is ok, we can improve things afterwards.

  -- Daniel

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1564272

> Yeah, that could be problematic. The code snippet gives the general idea but it
> could be changed by for example by a flag telling the cpus when they enter idle
> to update their state_count. Or something like that.

Yes, this idea could be helpful.

But since the idle path isn't a hot path. and a few memory access won't 
cost a lot. So I doubt if the benefit could be measurable.


>
> But if you think the patchset is fine, it is ok, we can improve things afterwards.
>

[toc] | [prev] | [next] | [standalone]

#1564965

On Sun, Jan 22, 2017 at 09:31:44AM +0800, Alex Shi wrote:
> 
> >Yeah, that could be problematic. The code snippet gives the general idea but it
> >could be changed by for example by a flag telling the cpus when they enter idle
> >to update their state_count. Or something like that.
> 
> Yes, this idea could be helpful.
> 
> But since the idle path isn't a hot path. and a few memory access won't cost
> a lot. So I doubt if the benefit could be measurable.

It won't be measurable, as well as reading the cpu device latency before
checking the latency req is zero, but it makes sense.

The idle routine is not a hot path but a very special place where the interrupt
are disabled, the rcu is not usable, tick is disabled etc ...

Perhaps it is not a problem for the moment, but it is probably worth to mention that
using API from other subsystems in the idle select path could be problematic
and perhaps it is time to think about another approach for the future.

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1565017

On 01/23/2017 08:50 PM, Daniel Lezcano wrote:
> On Sun, Jan 22, 2017 at 09:31:44AM +0800, Alex Shi wrote:
>>
>>> Yeah, that could be problematic. The code snippet gives the general idea but it
>>> could be changed by for example by a flag telling the cpus when they enter idle
>>> to update their state_count. Or something like that.
>>
>> Yes, this idea could be helpful.
>>
>> But since the idle path isn't a hot path. and a few memory access won't cost
>> a lot. So I doubt if the benefit could be measurable.
> 
> It won't be measurable, as well as reading the cpu device latency before
> checking the latency req is zero, but it makes sense.

Just simple change the cpu state may make it looks unnatural. :)
> 
> The idle routine is not a hot path but a very special place where the interrupt
> are disabled, the rcu is not usable, tick is disabled etc ...
> 
> Perhaps it is not a problem for the moment, but it is probably worth to mention that
> using API from other subsystems in the idle select path could be problematic
> and perhaps it is time to think about another approach for the future.
> 

Yes, before idle, it did consider lots of parts, included pm qos for
long time... :)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web