Make the "which C state line" logic better

From 2e88a61859db0592707d1a0a35e33408a0327951 Mon Sep 17 00:00:00 2001

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

src/cpu/abstract_cpu.cpp +1 -1

index cd4eba0..8b4c650 100644
@@ -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;

src/cpu/abstract_cpu.cpp +2 -0

@@ -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) {

src/cpu/abstract_cpu.cpp +1 -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 */

src/cpu/abstract_cpu.cpp +13 -1

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

src/cpu/abstract_cpu.cpp +1 -1

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

src/cpu/cpu.h +2 -2

index b48ada9..d51e3b2 100644
@@ -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

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.

Quoted 2 lines · Rajagopal Venkat

On 5 August 2012 22:43, Arjan van de Ven <arjan(a)linux.intel.com <mailto:

Quoted 2 lines · arjan(a)linux.intel.com>> wrote:

    From 2e88a61859db0592707d1a0a35e33408a0327951 Mon Sep 17 00:00:00

2001

Quoted 1 line

From: Arjan van de Ven <arjan(a)linux.intel.com <mailto:

arjan(a)linux.intel.com>>

Quoted 4 lines

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

Quoted 2 lines

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

Quoted 4 lines

side of the logic.

I fail to understand how using "linux_name" for parsing C states would

mess up

Quoted 1 line

x86 logic. Each supported C state will have corresponding 'stateX'

directory

Quoted 2 lines

(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

Quoted 2 lines

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.