[Powertop] [PATCH] Assign line level based on linux name

[Powertop] [PATCH] Assign line level based on linux name

[Radhakrishna Sripada]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

C-states having the same number in their human names have the same line
levels. Switching to linux names assigns seperate levels to each C-state.

Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada(a)intel.com>
---
 src/cpu/abstract_cpu.cpp | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/src/cpu/abstract_cpu.cpp b/src/cpu/abstract_cpu.cpp
index 17acb71..12dcb7d 100644
--- a/src/cpu/abstract_cpu.cpp
+++ b/src/cpu/abstract_cpu.cpp
@@ -210,7 +210,7 @@ void abstract_cpu::insert_cstate(const char *linux_name, const char *human_name,
 
 	state->line_level = -1;
 
-	c = human_name;
+	c = linux_name;
 	while (*c) {
 		if (strcmp(linux_name, "active")==0) {
 			state->line_level = LEVEL_C0;
@@ -223,15 +223,6 @@ void abstract_cpu::insert_cstate(const char *linux_name, const char *human_name,
 		c++;
 	}
 
-	/* some architectures (ARM) don't have good numbers in their human name.. fall back to the linux name for those */
-	c = linux_name;
-	while (*c && state->line_level < 0) {
-		if (*c >= '0' && *c <='9') {
-			state->line_level = strtoull(c, NULL, 10);
-			break;
-		}
-		c++;
-	}
 
 	if (level >= 0)
 		state->line_level = level;
-- 
1.9.1

Re: [Powertop] [PATCH] Assign line level based on linux name

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

On 9/21/2015 11:34 AM, Radhakrishna Sripada wrote:
> C-states having the same number in their human names have the same line
> levels. Switching to linux names assigns seperate levels to each C-state.
>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada(a)intel.com>

hmm do you have a before/after kind of thing?
(your description is a bit cryptic in that sense)

also of course this code isn't used for Intel cpus normally...

Re: [Powertop] [PATCH] Assign line level based on linux name

[Sripada, Radhakrishna]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

intel_idle driver exposes the sates C1-SKL and C1E-SKL. Powertop Idle Stats only show the statistics of C1E-SKL and not C1-SKL. By parsing the human name for the two states, the same line level '1' gets assigned to both C1-SKL and C1E-SKL. Hence the statistics of C1-SKL are always overwritten by the statistics of C1E-SKL. To eliminate this parsing linux name would assign different line levels to different cstates irrespective of their human names.

-----Original Message-----
From: Arjan van de Ven [mailto:arjan(a)linux.intel.com] 
Sent: Monday, September 21, 2015 12:49 PM
To: Sripada, Radhakrishna; powertop(a)lists.01.org
Subject: Re: [Powertop] [PATCH] Assign line level based on linux name

On 9/21/2015 11:34 AM, Radhakrishna Sripada wrote:
> C-states having the same number in their human names have the same 
> line levels. Switching to linux names assigns seperate levels to each C-state.
>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada(a)intel.com>

hmm do you have a before/after kind of thing?
(your description is a bit cryptic in that sense)

also of course this code isn't used for Intel cpus normally...

Re: [Powertop] [PATCH] Assign line level based on linux name

[Alexandra Yates]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]

Hello,


On 09/21/2015 02:26 PM, Sripada, Radhakrishna wrote:
> intel_idle driver exposes the sates C1-SKL and C1E-SKL. Powertop Idle Stats only show the statistics of C1E-SKL and not C1-SKL. By parsing the human name for the two states, the same line level '1' gets assigned to both C1-SKL and C1E-SKL. Hence the statistics of C1-SKL are always overwritten by the statistics of C1E-SKL. To eliminate this parsing linux name would assign different line levels to different cstates irrespective of their human names.
>
> -----Original Message-----
> From: Arjan van de Ven [mailto:arjan(a)linux.intel.com]
> Sent: Monday, September 21, 2015 12:49 PM
> To: Sripada, Radhakrishna; powertop(a)lists.01.org
> Subject: Re: [Powertop] [PATCH] Assign line level based on linux name
>
> On 9/21/2015 11:34 AM, Radhakrishna Sripada wrote:
>> C-states having the same number in their human names have the same
>> line levels. Switching to linux names assigns seperate levels to each C-state.
>>
>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada(a)intel.com>
>
> hmm do you have a before/after kind of thing?
> (your description is a bit cryptic in that sense)
>
> also of course this code isn't used for Intel cpus normally...
>
>
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
>

Does anyone from the ARM community have an issue with this patch?

RD,
Please re-send your patch with a more clear description on what the 
patch fixes.  like you do in this message.

-- 
Thank you,
<Alexandra>

[Powertop] [PATCH] Assign line level based on linux name

[Radhakrishna Sripada]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

Powertop Idle Stats only show the statistics of C1E-SKL and not C1-SKL while
intel_idle driver exposes both the states. Parsing the human names of the
states, the same line level '1' gets assigned to both C1-SKL and C1E-SKL. The
statistics of C1-SKL are always overwritten by the statistics of C1E-SKL. To
eliminate this parsing linux name would assign different line levels to
different cstates irrespective of their human names.

Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada(a)intel.com>
---
 src/cpu/abstract_cpu.cpp | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/src/cpu/abstract_cpu.cpp b/src/cpu/abstract_cpu.cpp
index 17acb71..12dcb7d 100644
--- a/src/cpu/abstract_cpu.cpp
+++ b/src/cpu/abstract_cpu.cpp
@@ -210,7 +210,7 @@ void abstract_cpu::insert_cstate(const char *linux_name, const char *human_name,
 
 	state->line_level = -1;
 
-	c = human_name;
+	c = linux_name;
 	while (*c) {
 		if (strcmp(linux_name, "active")==0) {
 			state->line_level = LEVEL_C0;
@@ -223,15 +223,6 @@ void abstract_cpu::insert_cstate(const char *linux_name, const char *human_name,
 		c++;
 	}
 
-	/* some architectures (ARM) don't have good numbers in their human name.. fall back to the linux name for those */
-	c = linux_name;
-	while (*c && state->line_level < 0) {
-		if (*c >= '0' && *c <='9') {
-			state->line_level = strtoull(c, NULL, 10);
-			break;
-		}
-		c++;
-	}
 
 	if (level >= 0)
 		state->line_level = level;
-- 
1.9.1