[PATCH V2 1/2] cpufreq, xenpm: fix cpufreq and xenpm mismatch

[PATCH V2 1/2] cpufreq, xenpm: fix cpufreq and xenpm mismatch

[Jacob Shin]

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

[PATCH V2 2/2] cpufreq, powernow: enable/disable core performance boost for all CPUs in the Node

[Jacob Shin]

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

Re: [PATCH V2 2/2] cpufreq, powernow: enable/disable core performance boost for all CPUs in the Node

[Liu, Jinsong]

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

Re: [PATCH V2 2/2] cpufreq, powernow: enable/disable core performance boost for all CPUs in the Node

[Jan Beulich]

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