diff mbox

[2/3] cpu: expose pm_qos_resume_latency for each cpu

Message ID 1483630187-29622-3-git-send-email-alex.shi@linaro.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Alex Shi Jan. 5, 2017, 3:29 p.m. UTC
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>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
---
 drivers/base/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Greg KH Jan. 11, 2017, 8:09 a.m. UTC | #1
On Thu, Jan 05, 2017 at 11:29:46PM +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>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/cpu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 4c28e1a..29cf3459 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,9 @@ 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));
> +#ifdef CONFIG_CPU_IDLE_GOV_MENU
> +	dev_pm_qos_expose_latency_limit(&cpu->dev, 0);
> +#endif

No way to do this without the #ifdef?  That's really not recommended for
.c code :(

thanks,

greg k-h
--
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
Alex Shi Jan. 11, 2017, 2:33 p.m. UTC | #2
>> >  #include "base.h"
>> >  
>> > @@ -376,6 +377,9 @@ 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));
>> > +#ifdef CONFIG_CPU_IDLE_GOV_MENU
>> > +	dev_pm_qos_expose_latency_limit(&cpu->dev, 0);
>> > +#endif
> No way to do this without the #ifdef?  That's really not recommended for
> .c code :(
> 

Hi Greg,

Thanks for comments!

The function dev_pm_qos_expose_latency_limit() is null if no CONFIG_PM.
So when CONFIG_PM enabled, may we could consider the cpu idle is also
wanted. In this assumption the #ifdef could be removed. If user want to
use this feature, she/he should understand the feature only work on menu
gov only currently. So consider this, I guess we could remove this
#ifdef. :)

Any different concerns on this?

Regards
Alex

BTW,
Although I did try this patch on other platform, but it clearly other
multi core system, like x86 could also get the same benefit.
--
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
Rafael J. Wysocki Jan. 11, 2017, 6:40 p.m. UTC | #3
On Wed, Jan 11, 2017 at 3:33 PM, Alex Shi <alex.shi@linaro.org> wrote:
>
>>> >  #include "base.h"
>>> >
>>> > @@ -376,6 +377,9 @@ 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));
>>> > +#ifdef CONFIG_CPU_IDLE_GOV_MENU
>>> > +  dev_pm_qos_expose_latency_limit(&cpu->dev, 0);
>>> > +#endif
>> No way to do this without the #ifdef?  That's really not recommended for
>> .c code :(
>>
>
> Hi Greg,
>
> Thanks for comments!
>
> The function dev_pm_qos_expose_latency_limit() is null if no CONFIG_PM.
> So when CONFIG_PM enabled, may we could consider the cpu idle is also
> wanted. In this assumption the #ifdef could be removed. If user want to
> use this feature, she/he should understand the feature only work on menu
> gov only currently. So consider this, I guess we could remove this
> #ifdef. :)

But instead of putting the #ifdef into the function body, you can use
a wrapper function defined to be empty for CONFIG_PM unset.

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
Alex Shi Jan. 12, 2017, 1:04 p.m. UTC | #4
On 01/12/2017 02:40 AM, Rafael J. Wysocki wrote:
>> >
>> > Hi Greg,
>> >
>> > Thanks for comments!
>> >
>> > The function dev_pm_qos_expose_latency_limit() is null if no CONFIG_PM.
>> > So when CONFIG_PM enabled, may we could consider the cpu idle is also
>> > wanted. In this assumption the #ifdef could be removed. If user want to
>> > use this feature, she/he should understand the feature only work on menu
>> > gov only currently. So consider this, I guess we could remove this
>> > #ifdef. :)
> But instead of putting the #ifdef into the function body, you can use
> a wrapper function defined to be empty for CONFIG_PM unset.

Thanks Rafael!

The function dev_pm_qos_expose_latency_limit() is empty now when
CONFIG_PM disabled. :)
I will resend the patch without the #ifdef.

Thanks!
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 mbox

Patch

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 4c28e1a..29cf3459 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,9 @@  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));
+#ifdef CONFIG_CPU_IDLE_GOV_MENU
+	dev_pm_qos_expose_latency_limit(&cpu->dev, 0);
+#endif
 
 	return 0;
 }