* [Powertop] [PATCH 2/4] Make the "which C state line" logic better
@ 2012-08-05 17:13 Arjan van de Ven
0 siblings, 0 replies; 4+ messages in thread
From: Arjan van de Ven @ 2012-08-05 17:13 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 3897 bytes --]
>From 2e88a61859db0592707d1a0a35e33408a0327951 Mon Sep 17 00:00:00 2001
From: Arjan van de Ven <arjan(a)linux.intel.com>
Date: Sun, 5 Aug 2012 09:57:49 -0700
Subject: [PATCH 2/4] Make the "which C state line" logic better
the ARM guys complained that their human-readable C state names didn't have
numbers in them, and that as a result, the output is all messed up.
Using the "linux_name" instead is only a partial solution; it messes up the x86
side of the logic.
This patch fixes the logic to make the code use the human readable logic first,
but if there's no numbers there, fall back to the Linux name.
In addition, the patch allows callers to specify the line directly, overriding
both sets of logic.
---
src/cpu/abstract_cpu.cpp | 21 ++++++++++++++++++---
src/cpu/cpu.h | 4 ++--
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/cpu/abstract_cpu.cpp b/src/cpu/abstract_cpu.cpp
index cd4eba0..8b4c650 100644
--- a/src/cpu/abstract_cpu.cpp
+++ b/src/cpu/abstract_cpu.cpp
@@ -130,7 +130,7 @@ void abstract_cpu::measurement_end(void)
}
}
-void abstract_cpu::insert_cstate(const char *linux_name, const char *human_name, uint64_t usage, uint64_t duration, int count)
+void abstract_cpu::insert_cstate(const char *linux_name, const char *human_name, uint64_t usage, uint64_t duration, int count, int level)
{
struct idle_state *state;
const char *c;
@@ -147,6 +147,8 @@ void abstract_cpu::insert_cstate(const char *linux_name, const char *human_name,
strcpy(state->linux_name, linux_name);
strcpy(state->human_name, human_name);
+ state->line_level = -1;
+
c = human_name;
while (*c) {
if (strcmp(linux_name, "active")==0) {
@@ -160,6 +162,19 @@ void abstract_cpu::insert_cstate(const char *linux_name, const char *human_name,
c++;
}
+ /* some architectures (ARM) don't have good numbers in thier 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;
+
state->usage_before = usage;
state->duration_before = duration;
state->before_count = count;
@@ -187,7 +202,7 @@ void abstract_cpu::finalize_cstate(const char *linux_name, uint64_t usage, uint6
state->after_count += count;
}
-void abstract_cpu::update_cstate(const char *linux_name, const char *human_name, uint64_t usage, uint64_t duration, int count)
+void abstract_cpu::update_cstate(const char *linux_name, const char *human_name, uint64_t usage, uint64_t duration, int count, int level)
{
unsigned int i;
struct idle_state *state = NULL;
@@ -200,7 +215,7 @@ void abstract_cpu::update_cstate(const char *linux_name, const char *human_name,
}
if (!state) {
- insert_cstate(linux_name, human_name, usage, duration, count);
+ insert_cstate(linux_name, human_name, usage, duration, count, level);
return;
}
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index b48ada9..d51e3b2 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -111,8 +111,8 @@ public:
/* C state related methods */
- void insert_cstate(const char *linux_name, const char *human_name, uint64_t usage, uint64_t duration, int count);
- void update_cstate(const char *linux_name, const char *human_name, uint64_t usage, uint64_t duration, int count);
+ void insert_cstate(const char *linux_name, const char *human_name, uint64_t usage, uint64_t duration, int count, int level = -1);
+ void update_cstate(const char *linux_name, const char *human_name, uint64_t usage, uint64_t duration, int count, int level = -1);
void finalize_cstate(const char *linux_name, uint64_t usage, uint64_t duration, int count);
virtual int has_cstate_level(int level);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Powertop] [PATCH 2/4] Make the "which C state line" logic better
@ 2012-08-06 7:19 Rajagopal Venkat
0 siblings, 0 replies; 4+ messages in thread
From: Rajagopal Venkat @ 2012-08-06 7:19 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 5095 bytes --]
On 5 August 2012 22:43, Arjan van de Ven <arjan(a)linux.intel.com> wrote:
> From 2e88a61859db0592707d1a0a35e33408a0327951 Mon Sep 17 00:00:00 2001
> From: Arjan van de Ven <arjan(a)linux.intel.com>
> Date: Sun, 5 Aug 2012 09:57:49 -0700
> Subject: [PATCH 2/4] Make the "which C state line" logic better
>
> the ARM guys complained that their human-readable C state names didn't have
> numbers in them, and that as a result, the output is all messed up.
> Using the "linux_name" instead is only a partial solution; it messes up
> the x86
> side of the logic.
>
>
I fail to understand how using "linux_name" for parsing C states would mess
up
x86 logic. Each supported C state will have corresponding 'stateX' directory
(linux_name) which contain numbers in them. Also there are few hard coded
states in intel_cpus.cpp file, in which linux_name contains numbers in them
as well. In both the cases linux_name contains numbers and hence safe to
parse. Please let me know if I am missing something here.
Thanks.
> This patch fixes the logic to make the code use the human readable logic
> first,
> but if there's no numbers there, fall back to the Linux name.
>
> In addition, the patch allows callers to specify the line directly,
> overriding
> both sets of logic.
> ---
> src/cpu/abstract_cpu.cpp | 21 ++++++++++++++++++---
> src/cpu/cpu.h | 4 ++--
> 2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/src/cpu/abstract_cpu.cpp b/src/cpu/abstract_cpu.cpp
> index cd4eba0..8b4c650 100644
> --- a/src/cpu/abstract_cpu.cpp
> +++ b/src/cpu/abstract_cpu.cpp
> @@ -130,7 +130,7 @@ void abstract_cpu::measurement_end(void)
> }
> }
>
> -void abstract_cpu::insert_cstate(const char *linux_name, const char
> *human_name, uint64_t usage, uint64_t duration, int count)
> +void abstract_cpu::insert_cstate(const char *linux_name, const char
> *human_name, uint64_t usage, uint64_t duration, int count, int level)
> {
> struct idle_state *state;
> const char *c;
> @@ -147,6 +147,8 @@ void abstract_cpu::insert_cstate(const char
> *linux_name, const char *human_name,
> strcpy(state->linux_name, linux_name);
> strcpy(state->human_name, human_name);
>
> + state->line_level = -1;
> +
> c = human_name;
> while (*c) {
> if (strcmp(linux_name, "active")==0) {
> @@ -160,6 +162,19 @@ void abstract_cpu::insert_cstate(const char
> *linux_name, const char *human_name,
> c++;
> }
>
> + /* some architectures (ARM) don't have good numbers in thier 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;
> +
> state->usage_before = usage;
> state->duration_before = duration;
> state->before_count = count;
> @@ -187,7 +202,7 @@ void abstract_cpu::finalize_cstate(const char
> *linux_name, uint64_t usage, uint6
> state->after_count += count;
> }
>
> -void abstract_cpu::update_cstate(const char *linux_name, const char
> *human_name, uint64_t usage, uint64_t duration, int count)
> +void abstract_cpu::update_cstate(const char *linux_name, const char
> *human_name, uint64_t usage, uint64_t duration, int count, int level)
> {
> unsigned int i;
> struct idle_state *state = NULL;
> @@ -200,7 +215,7 @@ void abstract_cpu::update_cstate(const char
> *linux_name, const char *human_name,
> }
>
> if (!state) {
> - insert_cstate(linux_name, human_name, usage, duration,
> count);
> + insert_cstate(linux_name, human_name, usage, duration,
> count, level);
> return;
> }
>
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index b48ada9..d51e3b2 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -111,8 +111,8 @@ public:
>
> /* C state related methods */
>
> - void insert_cstate(const char *linux_name, const char
> *human_name, uint64_t usage, uint64_t duration, int count);
> - void update_cstate(const char *linux_name, const char
> *human_name, uint64_t usage, uint64_t duration, int count);
> + void insert_cstate(const char *linux_name, const char
> *human_name, uint64_t usage, uint64_t duration, int count, int level = -1);
> + void update_cstate(const char *linux_name, const char
> *human_name, uint64_t usage, uint64_t duration, int count, int level = -1);
> void finalize_cstate(const char *linux_name, uint64_t
> usage, uint64_t duration, int count);
>
> virtual int has_cstate_level(int level);
> --
> 1.7.7.6
>
>
--
Regards,
Rajagopal
[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 5827 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Powertop] [PATCH 2/4] Make the "which C state line" logic better
@ 2012-08-06 13:22 Arjan van de Ven
0 siblings, 0 replies; 4+ messages in thread
From: Arjan van de Ven @ 2012-08-06 13:22 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]
On 8/6/2012 12:19 AM, Rajagopal Venkat wrote:
>
> On 5 August 2012 22:43, Arjan van de Ven <arjan(a)linux.intel.com <mailto:arjan(a)linux.intel.com>> wrote:
>
> From 2e88a61859db0592707d1a0a35e33408a0327951 Mon Sep 17 00:00:00 2001
> From: Arjan van de Ven <arjan(a)linux.intel.com <mailto:arjan(a)linux.intel.com>>
> Date: Sun, 5 Aug 2012 09:57:49 -0700
> Subject: [PATCH 2/4] Make the "which C state line" logic better
>
> the ARM guys complained that their human-readable C state names didn't have
> numbers in them, and that as a result, the output is all messed up.
> Using the "linux_name" instead is only a partial solution; it messes up the x86
> side of the logic.
>
>
> I fail to understand how using "linux_name" for parsing C states would mess up
> x86 logic. Each supported C state will have corresponding 'stateX' directory
> (linux_name) which contain numbers in them. Also there are few hard coded
> states in intel_cpus.cpp file, in which linux_name contains numbers in them
> as well. In both the cases linux_name contains numbers and hence safe to
> parse. Please let me know if I am missing something here.
it contains numbers, but not the right ones
so on x86, the package, core and cpu states do not line up properly if I only use linux_name.
(e.g. package C6 and core C6 are on a different line than CPU C6)
with this patch that is kept correctly, while hopefully also fixing your issue.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Powertop] [PATCH 2/4] Make the "which C state line" logic better
@ 2012-08-06 13:28 Rajagopal Venkat
0 siblings, 0 replies; 4+ messages in thread
From: Rajagopal Venkat @ 2012-08-06 13:28 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 1741 bytes --]
On 6 August 2012 18:52, Arjan van de Ven <arjan(a)linux.intel.com> wrote:
> On 8/6/2012 12:19 AM, Rajagopal Venkat wrote:
> >
> > On 5 August 2012 22:43, Arjan van de Ven <arjan(a)linux.intel.com <mailto:
> arjan(a)linux.intel.com>> wrote:
> >
> > From 2e88a61859db0592707d1a0a35e33408a0327951 Mon Sep 17 00:00:00
> 2001
> > From: Arjan van de Ven <arjan(a)linux.intel.com <mailto:
> arjan(a)linux.intel.com>>
> > Date: Sun, 5 Aug 2012 09:57:49 -0700
> > Subject: [PATCH 2/4] Make the "which C state line" logic better
> >
> > the ARM guys complained that their human-readable C state names
> didn't have
> > numbers in them, and that as a result, the output is all messed up.
> > Using the "linux_name" instead is only a partial solution; it messes
> up the x86
> > side of the logic.
> >
> >
> > I fail to understand how using "linux_name" for parsing C states would
> mess up
> > x86 logic. Each supported C state will have corresponding 'stateX'
> directory
> > (linux_name) which contain numbers in them. Also there are few hard coded
> > states in intel_cpus.cpp file, in which linux_name contains numbers in
> them
> > as well. In both the cases linux_name contains numbers and hence safe to
> > parse. Please let me know if I am missing something here.
>
> it contains numbers, but not the right ones
> so on x86, the package, core and cpu states do not line up properly if I
> only use linux_name.
> (e.g. package C6 and core C6 are on a different line than CPU C6)
> with this patch that is kept correctly, while hopefully also fixing your
> issue.
>
>
Ok. Thanks for the clarification. Yes, this patch fixes my problem as well.
--
Regards,
Rajagopal
[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 2339 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-08-06 13:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-06 13:22 [Powertop] [PATCH 2/4] Make the "which C state line" logic better Arjan van de Ven
-- strict thread matches above, loose matches on Subject: below --
2012-08-06 13:28 Rajagopal Venkat
2012-08-06 7:19 Rajagopal Venkat
2012-08-05 17:13 Arjan van de Ven
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.