[PATCH 1/4] cpu: clean up register_cpu func

[PATCH 1/4] cpu: clean up register_cpu func

[Alex Shi]

This patch could reduce one branch in this function. Also
make the code more readble.

Signed-off-by: Alex Shi <alex.shi@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
To: linux-kernel@vger.kernel.org
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 691eeea..4c28e1a 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -371,12 +371,13 @@ int register_cpu(struct cpu *cpu, int num)
 	if (cpu->hotpluggable)
 		cpu->dev.groups = hotplugable_cpu_attr_groups;
 	error = device_register(&cpu->dev);
-	if (!error)
-		per_cpu(cpu_sys_devices, num) = &cpu->dev;
-	if (!error)
-		register_cpu_under_node(num, cpu_to_node(num));
+	if (error)
+		return error;
 
-	return error;
+	per_cpu(cpu_sys_devices, num) = &cpu->dev;
+	register_cpu_under_node(num, cpu_to_node(num));
+
+	return 0;
 }
 
 struct device *get_cpu_device(unsigned cpu)
-- 
2.8.1.101.g72d917a

[PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Alex Shi]

The cpu-dma PM QoS constraint impacts all the cpus in the system. There
is no way to let the user to choose a PM QoS constraint per cpu.

The following patch exposes to the userspace a per cpu based sysfs file
in order to let the userspace to change the value of the PM QoS latency
constraint.

This change is inoperative in its form and the cpuidle governors have to
take into account the per cpu latency constraint in addition to the
global cpu-dma latency constraint in order to operate properly.

BTW
The pm_qos_resume_latency usage defined in
Documentation/ABI/testing/sysfs-devices-power
The /sys/devices/.../power/pm_qos_resume_latency_us attribute
contains the PM QoS resume latency limit for the given device,
which is the maximum allowed time it can take to resume the
device, after it has been suspended at run time, from a resume
request to the moment the device will be ready to process I/O,
in microseconds.  If it is equal to 0, however, this means that
the PM QoS resume latency may be arbitrary.

Signed-off-by: Alex Shi <alex.shi@linaro.org>
To: linux-kernel@vger.kernel.org
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/cpu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 4c28e1a..ba11e23 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,8 @@ 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));
+	if (dev_pm_qos_expose_latency_limit(&cpu->dev, 0))
+		pr_debug("CPU%d: add resume latency failed\n", num);
 
 	return 0;
 }
-- 
2.8.1.101.g72d917a

[PATCH 3/4] cpuidle/menu: stop seeking deeper idle if current state is too deep

[Alex Shi]

The obsolete commit 71abbbf85 want to introduce a dynamic cstates,
but it was removed for long time. Just left the nonsense deeper cstate
checking.

Since all target_residency and exit_latency are going longer in deeper
idle state, no needs to waste some cpu cycle on useless seeking.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 03d38c2..bb58e2a 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -358,9 +358,9 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 		if (s->disabled || su->disable)
 			continue;
 		if (s->target_residency > data->predicted_us)
-			continue;
+			break;
 		if (s->exit_latency > latency_req)
-			continue;
+			break;
 
 		data->last_state_idx = i;
 	}
-- 
2.8.1.101.g72d917a

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

[Alex Shi]

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;
-- 
2.8.1.101.g72d917a

Re: [PATCH 1/4] cpu: clean up register_cpu func

[Alex Shi]

Hi,

Is there any concern on this patch?

Regards
Alex

On 08/25/2016 04:42 PM, Alex Shi wrote:
> This patch could reduce one branch in this function. Also
> make the code more readble.
> 
> Signed-off-by: Alex Shi <alex.shi@linaro.org>
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> To: linux-kernel@vger.kernel.org
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/cpu.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 691eeea..4c28e1a 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -371,12 +371,13 @@ int register_cpu(struct cpu *cpu, int num)
>  	if (cpu->hotpluggable)
>  		cpu->dev.groups = hotplugable_cpu_attr_groups;
>  	error = device_register(&cpu->dev);
> -	if (!error)
> -		per_cpu(cpu_sys_devices, num) = &cpu->dev;
> -	if (!error)
> -		register_cpu_under_node(num, cpu_to_node(num));
> +	if (error)
> +		return error;
>  
> -	return error;
> +	per_cpu(cpu_sys_devices, num) = &cpu->dev;
> +	register_cpu_under_node(num, cpu_to_node(num));
> +
> +	return 0;
>  }
>  
>  struct device *get_cpu_device(unsigned cpu)
> 

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Alex Shi]

Hi,

Is there any concern on this patch?

Regards
Alex


On 08/25/2016 04:42 PM, Alex Shi wrote:
> The cpu-dma PM QoS constraint impacts all the cpus in the system. There
> is no way to let the user to choose a PM QoS constraint per cpu.
> 
> The following patch exposes to the userspace a per cpu based sysfs file
> in order to let the userspace to change the value of the PM QoS latency
> constraint.
> 
> This change is inoperative in its form and the cpuidle governors have to
> take into account the per cpu latency constraint in addition to the
> global cpu-dma latency constraint in order to operate properly.
> 
> BTW
> The pm_qos_resume_latency usage defined in
> Documentation/ABI/testing/sysfs-devices-power
> The /sys/devices/.../power/pm_qos_resume_latency_us attribute
> contains the PM QoS resume latency limit for the given device,
> which is the maximum allowed time it can take to resume the
> device, after it has been suspended at run time, from a resume
> request to the moment the device will be ready to process I/O,
> in microseconds.  If it is equal to 0, however, this means that
> the PM QoS resume latency may be arbitrary.
> 
> Signed-off-by: Alex Shi <alex.shi@linaro.org>
> To: linux-kernel@vger.kernel.org
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 4c28e1a..ba11e23 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,8 @@ 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));
> +	if (dev_pm_qos_expose_latency_limit(&cpu->dev, 0))
> +		pr_debug("CPU%d: add resume latency failed\n", num);
>  
>  	return 0;
>  }
> 

Re: [PATCH 3/4] cpuidle/menu: stop seeking deeper idle if current state is too deep

[Alex Shi]

Hi,

Is there any concern on this patch?

Regards
Alex


On 08/25/2016 04:42 PM, Alex Shi wrote:
> The obsolete commit 71abbbf85 want to introduce a dynamic cstates,
> but it was removed for long time. Just left the nonsense deeper cstate
> checking.
> 
> Since all target_residency and exit_latency are going longer in deeper
> idle state, no needs to waste some cpu cycle on useless seeking.
> 
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 03d38c2..bb58e2a 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -358,9 +358,9 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  		if (s->disabled || su->disable)
>  			continue;
>  		if (s->target_residency > data->predicted_us)
> -			continue;
> +			break;
>  		if (s->exit_latency > latency_req)
> -			continue;
> +			break;
>  
>  		data->last_state_idx = i;
>  	}
> 

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

[Alex Shi]

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

Re: [PATCH 1/4] cpu: clean up register_cpu func

[Greg Kroah-Hartman]

On Wed, Aug 31, 2016 at 06:45:35PM +0800, Alex Shi wrote:
> Hi,
> 
> Is there any concern on this patch?

For minor cleanup patches like this, relax, they will be reviewed
eventually...

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Greg Kroah-Hartman]

On Thu, Aug 25, 2016 at 04:42:40PM +0800, Alex Shi wrote:
> The cpu-dma PM QoS constraint impacts all the cpus in the system. There
> is no way to let the user to choose a PM QoS constraint per cpu.
> 
> The following patch exposes to the userspace a per cpu based sysfs file
> in order to let the userspace to change the value of the PM QoS latency
> constraint.
> 
> This change is inoperative in its form and the cpuidle governors have to
> take into account the per cpu latency constraint in addition to the
> global cpu-dma latency constraint in order to operate properly.
> 
> BTW
> The pm_qos_resume_latency usage defined in
> Documentation/ABI/testing/sysfs-devices-power
> The /sys/devices/.../power/pm_qos_resume_latency_us attribute
> contains the PM QoS resume latency limit for the given device,
> which is the maximum allowed time it can take to resume the
> device, after it has been suspended at run time, from a resume
> request to the moment the device will be ready to process I/O,
> in microseconds.  If it is equal to 0, however, this means that
> the PM QoS resume latency may be arbitrary.
> 
> Signed-off-by: Alex Shi <alex.shi@linaro.org>
> To: linux-kernel@vger.kernel.org
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 4c28e1a..ba11e23 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,8 @@ 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));
> +	if (dev_pm_qos_expose_latency_limit(&cpu->dev, 0))
> +		pr_debug("CPU%d: add resume latency failed\n", num);

Why is this a debug message?  And you have a struct device, why not use
it?

Also, what userspace changes does this require, or affect?  Any new
sysfs files?

thanks,

greg k-h

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Alex Shi]

>> @@ -376,6 +377,8 @@ 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));
>> +	if (dev_pm_qos_expose_latency_limit(&cpu->dev, 0))
>> +		pr_debug("CPU%d: add resume latency failed\n", num);
> 
> Why is this a debug message?  And you have a struct device, why not use
> it?
> 

Looks no one care about the dev_pm_qos_expose_latency_limit's return
value, so this debug message like gilding the lily. will remove it.

> Also, what userspace changes does this require, or affect?  Any new
> sysfs files?

User can set values on each of cpu, like limit 100ms on cpu0, that means
the cpu0 response time should be in 100ms in possible idle. It similar
with DMA_LATENCY, but that request is for all cpu. This is just for
particular cpu, like a interrupt pined CPU.

echo 100 > /sys/devices/system/cpu/cpu0/power/pm_qos_resume_latency_us
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Alex Shi]

On 09/01/2016 11:39 AM, Alex Shi wrote:
> User can set values on each of cpu, like limit 100ms on cpu0, that means
> the cpu0 response time should be in 100ms in possible idle. It similar
> with DMA_LATENCY, but that request is for all cpu. This is just for
> particular cpu, like a interrupt pined CPU.

Sorry for typo!
s/100ms/100us/

> 
> echo 100 > /sys/devices/system/cpu/cpu0/power/pm_qos_resume_latency_us
>> > 

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Alex Shi]

Few commits and patch changed according to Greg's comments.

Regards
Alex

====

>From 186c534b0b8b9649fbfce05b0b4f90f764c571a4 Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@linaro.org>
Date: Tue, 16 Aug 2016 15:29:01 +0800
Subject: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

Adding /sys/devices/system/cpu/cpux/power/pm_qos_resume_latency_us for
each of cpus. The pm_qos_resume_latency usage defined in
Documentation/ABI/testing/sysfs-devices-power

The cpu-dma PM QoS constraint impacts all the cpus in the system. There
is no way to let the user to choose a PM QoS constraint per cpu.

The following patch exposes to the userspace a per cpu based sysfs file
in order to let the userspace to change the value of the PM QoS latency
constraint.

This change is inoperative in its form and the cpuidle governors have to
take into account the per cpu latency constraint in addition to the
global cpu-dma latency constraint in order to operate properly.

Signed-off-by: Alex Shi <alex.shi@linaro.org>
To: linux-kernel@vger.kernel.org
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/cpu.c | 2 ++
 1 file changed, 2 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;
 }
-- 
2.8.1.101.g72d917a


 

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Ulf Hansson]

On 1 September 2016 at 05:50, Alex Shi <alex.shi@linaro.org> wrote:
>
> Few commits and patch changed according to Greg's comments.
>
> Regards
> Alex
>
> ====
>
> From 186c534b0b8b9649fbfce05b0b4f90f764c571a4 Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@linaro.org>
> Date: Tue, 16 Aug 2016 15:29:01 +0800
> Subject: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu
>
> Adding /sys/devices/system/cpu/cpux/power/pm_qos_resume_latency_us for
> each of cpus. The pm_qos_resume_latency usage defined in
> Documentation/ABI/testing/sysfs-devices-power
>
> The cpu-dma PM QoS constraint impacts all the cpus in the system. There
> is no way to let the user to choose a PM QoS constraint per cpu.
>
> The following patch exposes to the userspace a per cpu based sysfs file
> in order to let the userspace to change the value of the PM QoS latency
> constraint.
>
> This change is inoperative in its form and the cpuidle governors have to
> take into account the per cpu latency constraint in addition to the
> global cpu-dma latency constraint in order to operate properly.

In general I think the change makes sense, although it's this last
piece here that I wonder about.

Is it okay that we expose sysfs attributes to userspace that don't
have any effect if they change the values? Perhaps it should be the
responsibility of the menu governor somehow to expose the sysfs nodes
instead? Unless there are some difficulties that prevents us from that
of course.


Kind regards
Uffe

>
> Signed-off-by: Alex Shi <alex.shi@linaro.org>
> To: linux-kernel@vger.kernel.org
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/cpu.c | 2 ++
>  1 file changed, 2 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;
>  }
> --
> 2.8.1.101.g72d917a
>
>
>

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Alex Shi]

Cc Rafael.

On 09/01/2016 05:26 PM, Ulf Hansson wrote:
> In general I think the change makes sense, although it's this last
> piece here that I wonder about.
> 
> Is it okay that we expose sysfs attributes to userspace that don't
> have any effect if they change the values? Perhaps it should be the
> responsibility of the menu governor somehow to expose the sysfs nodes
> instead? Unless there are some difficulties that prevents us from that
> of course.
> 

Hi Ulf,

Sorry for response so late. The pm QoS designed to expose this interface
in userspace. Root user can change this value and made effect on device
sleeping status. That's required.

Since this is per device interface, set it on menu governor isn't so good.

Regards
Alex

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Ulf Hansson]

On 13 September 2016 at 03:04, Alex Shi <alex.shi@linaro.org> wrote:
> Cc Rafael.
>
> On 09/01/2016 05:26 PM, Ulf Hansson wrote:
>> In general I think the change makes sense, although it's this last
>> piece here that I wonder about.
>>
>> Is it okay that we expose sysfs attributes to userspace that don't
>> have any effect if they change the values? Perhaps it should be the
>> responsibility of the menu governor somehow to expose the sysfs nodes
>> instead? Unless there are some difficulties that prevents us from that
>> of course.
>>
>
> Hi Ulf,
>
> Sorry for response so late. The pm QoS designed to expose this interface
> in userspace. Root user can change this value and made effect on device
> sleeping status. That's required.

Sure, I understand that. Although, my point is that it's only when the
menu governor is enabled, that it would make sense for userspace to
modify the PM QoS values for the cpu device.

>
> Since this is per device interface, set it on menu governor isn't so good.

You may be right, and it's not a big deal for me! It was just an idea.

Kind regards
Uffe

Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu

[Alex Shi]

Cc Rafael,

This patchset is quit simple and straight. Is there any concern or
hesitate on it?

Thanks
Alex

On 09/01/2016 11:50 AM, Alex Shi wrote:
> 
> Few commits and patch changed according to Greg's comments.
> 
> Regards
> Alex
> 
> ====
> 
> From 186c534b0b8b9649fbfce05b0b4f90f764c571a4 Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@linaro.org>
> Date: Tue, 16 Aug 2016 15:29:01 +0800
> Subject: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu
> 
> Adding /sys/devices/system/cpu/cpux/power/pm_qos_resume_latency_us for
> each of cpus. The pm_qos_resume_latency usage defined in
> Documentation/ABI/testing/sysfs-devices-power
> 
> The cpu-dma PM QoS constraint impacts all the cpus in the system. There
> is no way to let the user to choose a PM QoS constraint per cpu.
> 
> The following patch exposes to the userspace a per cpu based sysfs file
> in order to let the userspace to change the value of the PM QoS latency
> constraint.
> 
> This change is inoperative in its form and the cpuidle governors have to
> take into account the per cpu latency constraint in addition to the
> global cpu-dma latency constraint in order to operate properly.
> 
> Signed-off-by: Alex Shi <alex.shi@linaro.org>
> To: linux-kernel@vger.kernel.org
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/cpu.c | 2 ++
>  1 file changed, 2 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;
>  }
> 

Re: [PATCH 3/4] cpuidle/menu: stop seeking deeper idle if current state is too deep

[Alex Shi]

Ping..

On 08/25/2016 04:42 PM, Alex Shi wrote:
> The obsolete commit 71abbbf85 want to introduce a dynamic cstates,
> but it was removed for long time. Just left the nonsense deeper cstate
> checking.
> 
> Since all target_residency and exit_latency are going longer in deeper
> idle state, no needs to waste some cpu cycle on useless seeking.
> 
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 03d38c2..bb58e2a 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -358,9 +358,9 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  		if (s->disabled || su->disable)
>  			continue;
>  		if (s->target_residency > data->predicted_us)
> -			continue;
> +			break;
>  		if (s->exit_latency > latency_req)
> -			continue;
> +			break;
>  
>  		data->last_state_idx = i;
>  	}
> 

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

[Alex Shi]

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

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

[Rafael J. Wysocki]

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

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

[Vincent Guittot]

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

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

[Rafael J. Wysocki]

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

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

[Alex Shi]

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

Re: [PATCH 1/4] cpu: clean up register_cpu func

[Daniel Lezcano]

On 08/19/2016 10:25 AM, Alex Shi wrote:
> This patch could reduce one branch in this function. Also
> make the code more readble.
> 
> Signed-off-by: Alex Shi <alex.shi@linaro.org>
> To: linux-kernel@vger.kernel.org
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

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

Re: [PATCH 1/4] cpu: clean up register_cpu func

[Alex Shi]

Is there any comments for this patch series?

Thanks!

On 08/19/2016 04:25 PM, Alex Shi wrote:
> This patch could reduce one branch in this function. Also
> make the code more readble.
> 
> Signed-off-by: Alex Shi <alex.shi@linaro.org>
> To: linux-kernel@vger.kernel.org
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/cpu.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 691eeea..4c28e1a 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -371,12 +371,13 @@ int register_cpu(struct cpu *cpu, int num)
>  	if (cpu->hotpluggable)
>  		cpu->dev.groups = hotplugable_cpu_attr_groups;
>  	error = device_register(&cpu->dev);
> -	if (!error)
> -		per_cpu(cpu_sys_devices, num) = &cpu->dev;
> -	if (!error)
> -		register_cpu_under_node(num, cpu_to_node(num));
> +	if (error)
> +		return error;
>  
> -	return error;
> +	per_cpu(cpu_sys_devices, num) = &cpu->dev;
> +	register_cpu_under_node(num, cpu_to_node(num));
> +
> +	return 0;
>  }
>  
>  struct device *get_cpu_device(unsigned cpu)
> 

[PATCH 1/4] cpu: clean up register_cpu func

[Alex Shi]

This patch could reduce one branch in this function. Also
make the code more readble.

Signed-off-by: Alex Shi <alex.shi@linaro.org>
To: linux-kernel@vger.kernel.org
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 691eeea..4c28e1a 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -371,12 +371,13 @@ int register_cpu(struct cpu *cpu, int num)
 	if (cpu->hotpluggable)
 		cpu->dev.groups = hotplugable_cpu_attr_groups;
 	error = device_register(&cpu->dev);
-	if (!error)
-		per_cpu(cpu_sys_devices, num) = &cpu->dev;
-	if (!error)
-		register_cpu_under_node(num, cpu_to_node(num));
+	if (error)
+		return error;
 
-	return error;
+	per_cpu(cpu_sys_devices, num) = &cpu->dev;
+	register_cpu_under_node(num, cpu_to_node(num));
+
+	return 0;
 }
 
 struct device *get_cpu_device(unsigned cpu)
-- 
2.8.1.101.g72d917a