diff mbox

[1/4] cpu: clean up register_cpu func

Message ID 1472114562-2736-1-git-send-email-alex.shi@linaro.org
State Accepted
Commit 59fffa34069d80662b41438b11130771b4e2a897
Headers show

Commit Message

Alex Shi Aug. 25, 2016, 8:42 a.m. UTC
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(-)

-- 
2.8.1.101.g72d917a

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

Comments

Alex Shi Sept. 1, 2016, 3:39 a.m. UTC | #1
>> @@ -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

> 

--
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 Sept. 1, 2016, 3:45 a.m. UTC | #2
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

>> > 

--
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 Sept. 13, 2016, 2:01 p.m. UTC | #3
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;

>  	}

> 

--
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
Vincent Guittot Sept. 14, 2016, 8:28 a.m. UTC | #4
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
diff mbox

Patch

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)