Xen devel - Jun 2013 - [PATCH 1/2] cpufreq, powernow: enable/disable core performance boost for all cpus in policy

Currently, enable/disable turbo mode on AMD is broken:

$ xenpm enable-turbo-mode 0 <-- works and proper CPU MSR bit is set
$ xenpm enable-turbo-mode 1 <-- silently broken, MSR bit not set

Since ->turbo is per policy, when user requests to enable/disable
turbo mode, we need to set the bit in all of the ->cpus that this
policy affects.
---
 xen/arch/x86/acpi/cpufreq/powernow.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c
b/xen/arch/x86/acpi/cpufreq/powernow.c
index 2c9fea2..81ba17f 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -85,7 +85,7 @@ static int powernow_cpufreq_update (int cpuid,
     if (!cpumask_test_cpu(cpuid, &cpu_online_map))
         return -EINVAL;
 
-    on_selected_cpus(cpumask_of(cpuid), update_cpb, policy, 1);
+    on_selected_cpus(policy->cpus, update_cpb, policy, 1);
 
     return 0;
 }
-- 
1.7.9.5

Currently we report back 0 or 1, which is broken since xenpm expects
CPUFREQ_TURBO_DISABLED, CPUFREQ_TURBO_UNSUPPORTED, or
CPUFREQ_TURBO_ENABLED. Report back actual policy->turbo value instead.
---
 xen/drivers/cpufreq/utility.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index 3dd70e2..1ff5e29 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -428,7 +428,10 @@ int cpufreq_get_turbo_status(int cpuid)
     struct cpufreq_policy *policy;
 
     policy = per_cpu(cpufreq_cpu_policy, cpuid);
-    return policy && policy->turbo;
+    if (!policy)
+        return CPUFREQ_TURBO_UNSUPPORTED;
+
+    return policy->turbo;
 }
 
 /*********************************************************************
-- 
1.7.9.5

On Wed, Jun 19, 2013 at 03:14:46PM -0500, Jacob Shin
wrote:
> Currently, enable/disable turbo mode on AMD is broken:
> 
> $ xenpm enable-turbo-mode 0 <-- works and proper CPU MSR bit is set
> $ xenpm enable-turbo-mode 1 <-- silently broken, MSR bit not set
> 
> Since ->turbo is per policy, when user requests to enable/disable
> turbo mode, we need to set the bit in all of the ->cpus that this
> policy affects.
Sorry, I''ll have to re-do this [PATCH 1/2], as it does not solve the
problem on platforms with a separate policy per CPU.

1. We''ll have to either globally enable/disable boost for all CPUs, or
2. [if there is a easy way to do so] find all Node siblings and set
their bits and policy->turbo as well.

[PATCH 2/2] is still valid.

Thanks,
> ---
>  xen/arch/x86/acpi/cpufreq/powernow.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c
b/xen/arch/x86/acpi/cpufreq/powernow.c
> index 2c9fea2..81ba17f 100644
> --- a/xen/arch/x86/acpi/cpufreq/powernow.c
> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c
> @@ -85,7 +85,7 @@ static int powernow_cpufreq_update (int cpuid,
>      if (!cpumask_test_cpu(cpuid, &cpu_online_map))
>          return -EINVAL;
>  
> -    on_selected_cpus(cpumask_of(cpuid), update_cpb, policy, 1);
> +    on_selected_cpus(policy->cpus, update_cpb, policy, 1);
>  
>      return 0;
>  }
> -- 
> 1.7.9.5
>

>>> On 20.06.13 at 01:07, Jacob Shin <jacob.shin@amd.com> wrote:
> On Wed, Jun 19, 2013 at 03:14:46PM -0500, Jacob Shin wrote:
>> Currently, enable/disable turbo mode on AMD is broken:
>> 
>> $ xenpm enable-turbo-mode 0 <-- works and proper CPU MSR bit is set
>> $ xenpm enable-turbo-mode 1 <-- silently broken, MSR bit not set
>> 
>> Since ->turbo is per policy, when user requests to enable/disable
>> turbo mode, we need to set the bit in all of the ->cpus that this
>> policy affects.
> 
> Sorry, I''ll have to re-do this [PATCH 1/2], as it does not solve
the
> problem on platforms with a separate policy per CPU.
But isn''t a separate policy per CPU meaning that indeed you want
to only fiddle with the one CPU that the policy is getting changed
for?
> 1. We''ll have to either globally enable/disable boost for all
CPUs, or
That''s clearly not a good path, as the interface is specifically
intending to allow per-policy control.
> 2. [if there is a easy way to do so] find all Node siblings and set
> their bits and policy->turbo as well.
That would be the way to go, pending clarification on the above.

Jan

>>> On 19.06.13 at 22:14, Jacob Shin <jacob.shin@amd.com> wrote:
> Currently we report back 0 or 1, which is broken since xenpm expects
> CPUFREQ_TURBO_DISABLED, CPUFREQ_TURBO_UNSUPPORTED, or
> CPUFREQ_TURBO_ENABLED. Report back actual policy->turbo value instead.
I think that it''s xenpm that''s wrong here - the three
constants above
aren''t exposed in public headers, and hence aren''t part of the
ABI.
Furthermore the structure member name that the result of this
function gets stored into is "turbo_enabled", which also suggests a
boolean value to me. I.e. if we want to change the value set that
the hypervisor may return here, the field should also get renamed,
and the value set exposed.
> --- a/xen/drivers/cpufreq/utility.c
> +++ b/xen/drivers/cpufreq/utility.c
> @@ -428,7 +428,10 @@ int cpufreq_get_turbo_status(int cpuid)
>      struct cpufreq_policy *policy;
>  
>      policy = per_cpu(cpufreq_cpu_policy, cpuid);
> -    return policy && policy->turbo;
> +    if (!policy)
> +        return CPUFREQ_TURBO_UNSUPPORTED;
> +
> +    return policy->turbo;
Nevertheless this would need adjustment even for the boolean
value case:

-    return policy && policy->turbo;
+    return policy && policy->turbo == CPUFREQ_TURBO_ENABLED;

Jan

On Thu, Jun 20, 2013 at 09:01:22AM +0100, Jan Beulich
wrote:
> >>> On 19.06.13 at 22:14, Jacob Shin <jacob.shin@amd.com>
wrote:
> > Currently we report back 0 or 1, which is broken since xenpm expects
> > CPUFREQ_TURBO_DISABLED, CPUFREQ_TURBO_UNSUPPORTED, or
> > CPUFREQ_TURBO_ENABLED. Report back actual policy->turbo value
instead.
> 
> I think that it''s xenpm that''s wrong here - the three
constants above
> aren''t exposed in public headers, and hence aren''t part
of the ABI.
> Furthermore the structure member name that the result of this
> function gets stored into is "turbo_enabled", which also suggests
a
> boolean value to me. I.e. if we want to change the value set that
> the hypervisor may return here, the field should also get renamed,
> and the value set exposed.
Okay I''ll fix xenpm instead.
> 
> > --- a/xen/drivers/cpufreq/utility.c
> > +++ b/xen/drivers/cpufreq/utility.c
> > @@ -428,7 +428,10 @@ int cpufreq_get_turbo_status(int cpuid)
> >      struct cpufreq_policy *policy;
> >  
> >      policy = per_cpu(cpufreq_cpu_policy, cpuid);
> > -    return policy && policy->turbo;
> > +    if (!policy)
> > +        return CPUFREQ_TURBO_UNSUPPORTED;
> > +
> > +    return policy->turbo;
> 
> Nevertheless this would need adjustment even for the boolean
> value case:
> 
> -    return policy && policy->turbo;
> +    return policy && policy->turbo == CPUFREQ_TURBO_ENABLED;
Yup that''s correct.

Thanks,

On Thu, Jun 20, 2013 at 08:54:00AM +0100, Jan Beulich
wrote:
> >>> On 20.06.13 at 01:07, Jacob Shin <jacob.shin@amd.com>
wrote:
> > On Wed, Jun 19, 2013 at 03:14:46PM -0500, Jacob Shin wrote:
> >> Currently, enable/disable turbo mode on AMD is broken:
> >> 
> >> $ xenpm enable-turbo-mode 0 <-- works and proper CPU MSR bit is
set
> >> $ xenpm enable-turbo-mode 1 <-- silently broken, MSR bit not
set
> >> 
> >> Since ->turbo is per policy, when user requests to
enable/disable
> >> turbo mode, we need to set the bit in all of the ->cpus that
this
> >> policy affects.
> > 
> > Sorry, I''ll have to re-do this [PATCH 1/2], as it does not
solve the
> > problem on platforms with a separate policy per CPU.
> 
> But isn''t a separate policy per CPU meaning that indeed you want
> to only fiddle with the one CPU that the policy is getting changed
> for?
Yes, this is okay if the end user realizes that on current AMD parts,
disabling turbo on one core means that it affects all other sibling
cores that are on the same node.

$ xenpm disable-turbo-mode 0 </-- hardware bit gets set on CPU0
$ xenpm get-cpufreq-para
...
CPU0
turbo mode: disabled
...
CPU1
turbo mode: enabled

But since CPU0 and CPU1 are in the same Node, in reality CPU1 also in
affect has turbo mode disabled.

If this is okay, then I think the above patch will be fine.
> 
> > 1. We''ll have to either globally enable/disable boost for all
CPUs, or
> 
> That''s clearly not a good path, as the interface is specifically
> intending to allow per-policy control.
Okay.
> 
> > 2. [if there is a easy way to do so] find all Node siblings and set
> > their bits and policy->turbo as well.
> 
> That would be the way to go, pending clarification on the above.
Let me know what you think about my blurb above.

Thanks!

>>> On 20.06.13 at 17:32, Jacob Shin <jacob.shin@amd.com> wrote:
> On Thu, Jun 20, 2013 at 08:54:00AM +0100, Jan Beulich wrote:
>> >>> On 20.06.13 at 01:07, Jacob Shin
<jacob.shin@amd.com> wrote:
>> > On Wed, Jun 19, 2013 at 03:14:46PM -0500, Jacob Shin wrote:
>> >> Currently, enable/disable turbo mode on AMD is broken:
>> >> 
>> >> $ xenpm enable-turbo-mode 0 <-- works and proper CPU MSR
bit is set
>> >> $ xenpm enable-turbo-mode 1 <-- silently broken, MSR bit
not set
>> >> 
>> >> Since ->turbo is per policy, when user requests to
enable/disable
>> >> turbo mode, we need to set the bit in all of the ->cpus
that this
>> >> policy affects.
>> > 
>> > Sorry, I''ll have to re-do this [PATCH 1/2], as it does
not solve the
>> > problem on platforms with a separate policy per CPU.
>> 
>> But isn''t a separate policy per CPU meaning that indeed you
want
>> to only fiddle with the one CPU that the policy is getting changed
>> for?
> 
> Yes, this is okay if the end user realizes that on current AMD parts,
> disabling turbo on one core means that it affects all other sibling
> cores that are on the same node.
> 
> $ xenpm disable-turbo-mode 0 </-- hardware bit gets set on CPU0
> $ xenpm get-cpufreq-para
> ...
> CPU0
> turbo mode: disabled
> ...
> CPU1
> turbo mode: enabled
> 
> But since CPU0 and CPU1 are in the same Node, in reality CPU1 also in
> affect has turbo mode disabled.
> 
> If this is okay, then I think the above patch will be fine.
No, this is not okay, but it is also not okay to reflect such a setup
with a per-core policy. Of course things are going to become
difficult if frequency selection can be per-core, but turbo mode
enabling requires that all cores have it enabled in order for it to
actually be used.

Jan

Sorry for late reply, just notice this thread. Some comments inline below.

Jan Beulich wrote:
>>>> On 19.06.13 at 22:14, Jacob Shin <jacob.shin@amd.com>
wrote:
>> Currently we report back 0 or 1, which is broken since xenpm expects
>> CPUFREQ_TURBO_DISABLED, CPUFREQ_TURBO_UNSUPPORTED, or
>> CPUFREQ_TURBO_ENABLED. Report back actual policy->turbo value
>> instead. 
> 
> I think that it''s xenpm that''s wrong here - the three
constants above
> aren''t exposed in public headers, and hence aren''t part
of the ABI.
> Furthermore the structure member name that the result of this
> function gets stored into is "turbo_enabled", which also suggests
a
> boolean value to me. I.e. if we want to change the value set that
> the hypervisor may return here, the field should also get renamed,
> and the value set exposed.
> 
IMHO this patch itself is OK, just need minor update at xenpm and add the 3
constants to public headers.

Tristate (cap, enable/disable) exposes more info to user than bool
(enable/disable), and the original semantics of the ABI is tristate (though bug
at Xen cpufreq side). As for ''turbo_enabled'', rename it as
''turbo'' is OK.

Thanks,
Jinsong
>> --- a/xen/drivers/cpufreq/utility.c
>> +++ b/xen/drivers/cpufreq/utility.c
>> @@ -428,7 +428,10 @@ int cpufreq_get_turbo_status(int cpuid)
>>      struct cpufreq_policy *policy;
>> 
>>      policy = per_cpu(cpufreq_cpu_policy, cpuid);
>> -    return policy && policy->turbo;
>> +    if (!policy)
>> +        return CPUFREQ_TURBO_UNSUPPORTED;
>> +
>> +    return policy->turbo;
> 
> Nevertheless this would need adjustment even for the boolean
> value case:
> 
> -    return policy && policy->turbo;
> +    return policy && policy->turbo == CPUFREQ_TURBO_ENABLED;
> 
> Jan