Xen devel - Jun 2013 - [PATCH V2 1/2] cpufreq, xenpm: fix cpufreq and xenpm mismatch

Currently cpufreq and xenpm are out of sync. Fix cpufreq reporting of
if turbo mode is enabled or not. Fix xenpm to not decode for tristate,
but a boolean.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>
---
 tools/misc/xenpm.c            |   14 +++-----------
 xen/drivers/cpufreq/utility.c |    2 +-
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index b5f1383..2e57f1f 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -31,10 +31,6 @@
 
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
 
-#define CPUFREQ_TURBO_DISABLED      -1
-#define CPUFREQ_TURBO_UNSUPPORTED   0
-#define CPUFREQ_TURBO_ENABLED       1
-
 static xc_interface *xc_handle;
 static unsigned int max_cpu_nr;
 
@@ -699,13 +695,9 @@ static void print_cpufreq_para(int cpuid, struct
xc_get_cpufreq_para *p_cpufreq)
            p_cpufreq->scaling_max_freq,
            p_cpufreq->scaling_min_freq,
            p_cpufreq->scaling_cur_freq);
-    if (p_cpufreq->turbo_enabled != CPUFREQ_TURBO_UNSUPPORTED) {
-           printf("turbo mode           : ");
-           if (p_cpufreq->turbo_enabled == CPUFREQ_TURBO_ENABLED)
-               printf("enabled\n");
-           else
-               printf("disabled\n");
-    }
+
+    printf("turbo mode           : %s\n",
+           p_cpufreq->turbo_enabled ? "enabled" : "disabled
or n/a");
     printf("\n");
 }
 
diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index 3dd70e2..519f862 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -428,7 +428,7 @@ int cpufreq_get_turbo_status(int cpuid)
     struct cpufreq_policy *policy;
 
     policy = per_cpu(cpufreq_cpu_policy, cpuid);
-    return policy && policy->turbo;
+    return policy && policy->turbo == CPUFREQ_TURBO_ENABLED;
 }
 
 /*********************************************************************
-- 
1.7.9.5

Since disabling turbo mode on one CPU also affect all other sibling
CPUs in the same Node, we need to call update_cpb on all CPUs in the
same node.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>
---
 xen/arch/x86/acpi/cpufreq/powernow.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c
b/xen/arch/x86/acpi/cpufreq/powernow.c
index 2c9fea2..3b5507a 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -29,6 +29,7 @@
 #include <xen/cpumask.h>
 #include <xen/timer.h>
 #include <xen/xmalloc.h>
+#include <xen/numa.h>
 #include <asm/bug.h>
 #include <asm/msr.h>
 #include <asm/io.h>
@@ -82,10 +83,20 @@ static void update_cpb(void *data)
 static int powernow_cpufreq_update (int cpuid,
 				     struct cpufreq_policy *policy)
 {
-    if (!cpumask_test_cpu(cpuid, &cpu_online_map))
-        return -EINVAL;
-
-    on_selected_cpus(cpumask_of(cpuid), update_cpb, policy, 1);
+    unsigned int cpu;
+    cpumask_t cpus;
+
+    cpumask_and(&cpus, &cpu_online_map,
&node_to_cpumask[cpu_to_node[cpuid]]);
+    on_selected_cpus(&cpus, update_cpb, policy, 1);
+
+    if (!cpumask_equal(policy->cpus, &cpus)) {
+        ASSERT(cpumask_subset(policy->cpus, &cpus));
+        for_each_cpu(cpu, &cpus) {
+            struct cpufreq_policy *p;
+            p = per_cpu(cpufreq_cpu_policy, cpu);
+            p->turbo = policy->turbo;
+        }
+    }
 
     return 0;
 }
-- 
1.7.9.5

Jacob Shin wrote:
> Since disabling turbo mode on one CPU also affect all other sibling
> CPUs in the same Node, we need to call update_cpb on all CPUs in the
> same node.
> 
> Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> ---
>  xen/arch/x86/acpi/cpufreq/powernow.c |   19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c
> b/xen/arch/x86/acpi/cpufreq/powernow.c index 2c9fea2..3b5507a 100644
> --- a/xen/arch/x86/acpi/cpufreq/powernow.c
> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c
> @@ -29,6 +29,7 @@
>  #include <xen/cpumask.h>
>  #include <xen/timer.h>
>  #include <xen/xmalloc.h>
> +#include <xen/numa.h>
>  #include <asm/bug.h>
>  #include <asm/msr.h>
>  #include <asm/io.h>
> @@ -82,10 +83,20 @@ static void update_cpb(void *data)
>  static int powernow_cpufreq_update (int cpuid,
>  				     struct cpufreq_policy *policy)
>  {
> -    if (!cpumask_test_cpu(cpuid, &cpu_online_map))
> -        return -EINVAL;
> -
> -    on_selected_cpus(cpumask_of(cpuid), update_cpb, policy, 1);
> +    unsigned int cpu;
> +    cpumask_t cpus;
> +
> +    cpumask_and(&cpus, &cpu_online_map,
> &node_to_cpumask[cpu_to_node[cpuid]]); +    on_selected_cpus(&cpus,
> update_cpb, policy, 1); +
> +    if (!cpumask_equal(policy->cpus, &cpus)) {
> +        ASSERT(cpumask_subset(policy->cpus, &cpus));
ASSERT here seems overkilled, considering buggy bios (i.e. buggy node affinity
or buggy cpufreq dom) may crash system.
Simply abort w/ warning and err return may be better.

Thanks,
Jinsong
> +        for_each_cpu(cpu, &cpus) {
> +            struct cpufreq_policy *p;
> +            p = per_cpu(cpufreq_cpu_policy, cpu);
> +            p->turbo = policy->turbo;
> +        }
> +    }
> 
>      return 0;
>  }

>>> On 20.06.13 at 19:04, Jacob Shin <jacob.shin@amd.com> wrote:
> Since disabling turbo mode on one CPU also affect all other sibling
> CPUs in the same Node, we need to call update_cpb on all CPUs in the
> same node.
Same node? Hardly - if anything, than same package.
> @@ -82,10 +83,20 @@ static void update_cpb(void *data)
>  static int powernow_cpufreq_update (int cpuid,
>  				     struct cpufreq_policy *policy)
>  {
> -    if (!cpumask_test_cpu(cpuid, &cpu_online_map))
> -        return -EINVAL;
You''re entirely losing that check.
> -
> -    on_selected_cpus(cpumask_of(cpuid), update_cpb, policy, 1);
> +    unsigned int cpu;
> +    cpumask_t cpus;
Please avoid on stack cpumask_t variables is at all possible. It
shouldn''t be too difficult to use cpumask_var_t here.
> +
> +    cpumask_and(&cpus, &cpu_online_map,
&node_to_cpumask[cpu_to_node[cpuid]]);
While per the above I expect this to go away anyway, if nevertheless
this needs to stay, then please use the macros, not the raw arrays.
> +    on_selected_cpus(&cpus, update_cpb, policy, 1);
> +
> +    if (!cpumask_equal(policy->cpus, &cpus)) {
This is wrong - the left side ought to be cpumask_of(cpuid), or
else you may again fail to update some of the policy structures.
> +        ASSERT(cpumask_subset(policy->cpus, &cpus));
> +        for_each_cpu(cpu, &cpus) {
> +            struct cpufreq_policy *p;
> +            p = per_cpu(cpufreq_cpu_policy, cpu);
> +            p->turbo = policy->turbo;
> +        }
Couldn''t this be done in update_cpb() instead?

And with update_cpb() being a no-op when policy->turbo
=CPUFREQ_TURBO_UNSUPPORTED, and since you''re re-writing
this function anyway - the better change then would be to not
even call on_selected_cpus() in that case (and of course the
loop here wouldn''t need to be gone through in that case either,
as long as we''re permitted to assume that CPUs in a package as
well as CPUs covered by the same policy have the same turbo
mode availability).

Jan