[PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL

[PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL

[Boris Ostrovsky]

# HG changeset patch
# User Boris Ostrovsky <boris.ostrovsky@amd.com>
# Date 1345135101 -7200
# Node ID 85190245a94d9945b7656c971ba36f7d1eff5c19
# Parent  6d56e31fe1e1dc793379d662a36ff1731760eb0c
AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL

When _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL (i.e. shared_type is
CPUFREQ_SHARED_TYPE_HW) which most often is the case on servers, there is no
reason to go into on_selected_cpus() code, we call call transition_pstate()
directly.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>

diff -r 6d56e31fe1e1 -r 85190245a94d xen/arch/x86/acpi/cpufreq/powernow.c
--- a/xen/arch/x86/acpi/cpufreq/powernow.c	Wed Aug 15 09:41:21 2012 +0100
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c	Thu Aug 16 18:38:21 2012 +0200
@@ -56,20 +56,9 @@
 
 static struct cpufreq_driver powernow_cpufreq_driver;
 
-struct drv_cmd {
-    unsigned int type;
-    const cpumask_t *mask;
-    u32 val;
-    int turbo;
-};
-
-static void transition_pstate(void *drvcmd)
+static void transition_pstate(void *pstate)
 {
-    struct drv_cmd *cmd;
-    cmd = (struct drv_cmd *) drvcmd;
-
-
-    wrmsrl(MSR_PSTATE_CTRL, cmd->val);
+    wrmsrl(MSR_PSTATE_CTRL, *(int *)pstate);
 }
 
 static void update_cpb(void *data)
@@ -106,13 +95,11 @@ static int powernow_cpufreq_target(struc
 {
     struct acpi_cpufreq_data *data = cpufreq_drv_data[policy->cpu];
     struct processor_performance *perf;
-    struct cpufreq_freqs freqs;
     cpumask_t online_policy_cpus;
-    struct drv_cmd cmd;
-    unsigned int next_state = 0; /* Index into freq_table */
-    unsigned int next_perf_state = 0; /* Index into perf table */
-    int result = 0;
-    int j = 0;
+    unsigned int next_state; /* Index into freq_table */
+    unsigned int next_perf_state; /* Index into perf table */
+    int result;
+    int j;
 
     if (unlikely(data == NULL ||
         data->acpi_data == NULL || data->freq_table == NULL)) {
@@ -125,9 +112,7 @@ static int powernow_cpufreq_target(struc
                                             target_freq,
                                             relation, &next_state);
     if (unlikely(result))
-        return -ENODEV;
-
-    cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
+        return result;
 
     next_perf_state = data->freq_table[next_state].index;
     if (perf->state == next_perf_state) {
@@ -137,26 +122,28 @@ static int powernow_cpufreq_target(struc
             return 0;
     }
 
-    if (policy->shared_type != CPUFREQ_SHARED_TYPE_ANY)
-        cmd.mask = &online_policy_cpus;
-    else
-        cmd.mask = cpumask_of(policy->cpu);
+    if (policy->shared_type == CPUFREQ_SHARED_TYPE_HW &&
+        likely(policy->cpu == smp_processor_id())) {
+        transition_pstate(&next_perf_state);
+        cpufreq_statistic_update(policy->cpu, perf->state, next_perf_state);
+    } else {
+        cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
 
-    freqs.old = perf->states[perf->state].core_frequency * 1000;
-    freqs.new = data->freq_table[next_state].frequency;
+        if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
+            unlikely(policy->cpu != smp_processor_id()))
+            on_selected_cpus(&online_policy_cpus, transition_pstate,
+                             &next_perf_state, 1);
+        else
+            transition_pstate(&next_perf_state);
 
-    cmd.val = next_perf_state;
-    cmd.turbo = policy->turbo;
-
-    on_selected_cpus(cmd.mask, transition_pstate, &cmd, 1);
-
-    for_each_cpu(j, &online_policy_cpus)
-        cpufreq_statistic_update(j, perf->state, next_perf_state);
+        for_each_cpu(j, &online_policy_cpus)
+            cpufreq_statistic_update(j, perf->state, next_perf_state);
+    }    
 
     perf->state = next_perf_state;
-    policy->cur = freqs.new;
+    policy->cur = data->freq_table[next_state].frequency;
 
-    return result;
+    return 0;
 }
 
 static int powernow_cpufreq_verify(struct cpufreq_policy *policy)

Re: [PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL

[Jan Beulich]

>>> On 16.08.12 at 18:41, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> AMD, powernow: Update P-state directly when _PSD's CoordType is 
> DOMAIN_COORD_TYPE_HW_ALL
> 
> When _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL (i.e. shared_type is
> CPUFREQ_SHARED_TYPE_HW) which most often is the case on servers, there is no
> reason to go into on_selected_cpus() code, we call call transition_pstate()
> directly.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>

Looks good to me in general (one minor comment below, but no
need to resubmit just because of this). But it's not really clear
to me whether this actually improves anything (knowing of which
is needed to decide whether to put this in now or after 4.2).

> --- a/xen/arch/x86/acpi/cpufreq/powernow.c	Wed Aug 15 09:41:21 2012 +0100
> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c	Thu Aug 16 18:38:21 2012 +0200
> @@ -56,20 +56,9 @@
>  
>  static struct cpufreq_driver powernow_cpufreq_driver;
>  
> -struct drv_cmd {
> -    unsigned int type;
> -    const cpumask_t *mask;
> -    u32 val;
> -    int turbo;
> -};
> -
> -static void transition_pstate(void *drvcmd)
> +static void transition_pstate(void *pstate)
>  {
> -    struct drv_cmd *cmd;
> -    cmd = (struct drv_cmd *) drvcmd;
> -
> -
> -    wrmsrl(MSR_PSTATE_CTRL, cmd->val);
> +    wrmsrl(MSR_PSTATE_CTRL, *(int *)pstate);

The variable a pointer to which gets passed in for this function
is "unsigned int", so you surely would need to cast to that type
instead of plain "int".

Jan

>  }
>  
>  static void update_cpb(void *data)
> @@ -106,13 +95,11 @@ static int powernow_cpufreq_target(struc
>  {
>      struct acpi_cpufreq_data *data = cpufreq_drv_data[policy->cpu];
>      struct processor_performance *perf;
> -    struct cpufreq_freqs freqs;
>      cpumask_t online_policy_cpus;
> -    struct drv_cmd cmd;
> -    unsigned int next_state = 0; /* Index into freq_table */
> -    unsigned int next_perf_state = 0; /* Index into perf table */
> -    int result = 0;
> -    int j = 0;
> +    unsigned int next_state; /* Index into freq_table */
> +    unsigned int next_perf_state; /* Index into perf table */
> +    int result;
> +    int j;
>  
>      if (unlikely(data == NULL ||
>          data->acpi_data == NULL || data->freq_table == NULL)) {
> @@ -125,9 +112,7 @@ static int powernow_cpufreq_target(struc
>                                              target_freq,
>                                              relation, &next_state);
>      if (unlikely(result))
> -        return -ENODEV;
> -
> -    cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
> +        return result;
>  
>      next_perf_state = data->freq_table[next_state].index;
>      if (perf->state == next_perf_state) {
> @@ -137,26 +122,28 @@ static int powernow_cpufreq_target(struc
>              return 0;
>      }
>  
> -    if (policy->shared_type != CPUFREQ_SHARED_TYPE_ANY)
> -        cmd.mask = &online_policy_cpus;
> -    else
> -        cmd.mask = cpumask_of(policy->cpu);
> +    if (policy->shared_type == CPUFREQ_SHARED_TYPE_HW &&
> +        likely(policy->cpu == smp_processor_id())) {
> +        transition_pstate(&next_perf_state);
> +        cpufreq_statistic_update(policy->cpu, perf->state, next_perf_state);
> +    } else {
> +        cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
>  
> -    freqs.old = perf->states[perf->state].core_frequency * 1000;
> -    freqs.new = data->freq_table[next_state].frequency;
> +        if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
> +            unlikely(policy->cpu != smp_processor_id()))
> +            on_selected_cpus(&online_policy_cpus, transition_pstate,
> +                             &next_perf_state, 1);
> +        else
> +            transition_pstate(&next_perf_state);
>  
> -    cmd.val = next_perf_state;
> -    cmd.turbo = policy->turbo;
> -
> -    on_selected_cpus(cmd.mask, transition_pstate, &cmd, 1);
> -
> -    for_each_cpu(j, &online_policy_cpus)
> -        cpufreq_statistic_update(j, perf->state, next_perf_state);
> +        for_each_cpu(j, &online_policy_cpus)
> +            cpufreq_statistic_update(j, perf->state, next_perf_state);
> +    }    
>  
>      perf->state = next_perf_state;
> -    policy->cur = freqs.new;
> +    policy->cur = data->freq_table[next_state].frequency;
>  
> -    return result;
> +    return 0;
>  }
>  
>  static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL

[Boris Ostrovsky]

On 08/17/2012 06:19 AM, Jan Beulich wrote:
>>>> On 16.08.12 at 18:41, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
>> AMD, powernow: Update P-state directly when _PSD's CoordType is
>> DOMAIN_COORD_TYPE_HW_ALL
>>
>> When _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL (i.e. shared_type is
>> CPUFREQ_SHARED_TYPE_HW) which most often is the case on servers, there is no
>> reason to go into on_selected_cpus() code, we call call transition_pstate()
>> directly.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
>
> Looks good to me in general (one minor comment below, but no
> need to resubmit just because of this). But it's not really clear
> to me whether this actually improves anything (knowing of which
> is needed to decide whether to put this in now or after 4.2).

I didn't see any noticeable improvement but then I may not have run the 
right tests.

I imagine this may be helpful, for example, in webserver-type 
environments with frequent P-state transitions when new connections are 
requested and short-lived threads/processes are created. But I haven't 
been able to set this up to observe this.

I think post-4.2 is fine.

-boris

>
>> --- a/xen/arch/x86/acpi/cpufreq/powernow.c	Wed Aug 15 09:41:21 2012 +0100
>> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c	Thu Aug 16 18:38:21 2012 +0200
>> @@ -56,20 +56,9 @@
>>
>>   static struct cpufreq_driver powernow_cpufreq_driver;
>>
>> -struct drv_cmd {
>> -    unsigned int type;
>> -    const cpumask_t *mask;
>> -    u32 val;
>> -    int turbo;
>> -};
>> -
>> -static void transition_pstate(void *drvcmd)
>> +static void transition_pstate(void *pstate)
>>   {
>> -    struct drv_cmd *cmd;
>> -    cmd = (struct drv_cmd *) drvcmd;
>> -
>> -
>> -    wrmsrl(MSR_PSTATE_CTRL, cmd->val);
>> +    wrmsrl(MSR_PSTATE_CTRL, *(int *)pstate);
>
> The variable a pointer to which gets passed in for this function
> is "unsigned int", so you surely would need to cast to that type
> instead of plain "int".
>
> Jan
>
>>   }
>>
>>   static void update_cpb(void *data)
>> @@ -106,13 +95,11 @@ static int powernow_cpufreq_target(struc
>>   {
>>       struct acpi_cpufreq_data *data = cpufreq_drv_data[policy->cpu];
>>       struct processor_performance *perf;
>> -    struct cpufreq_freqs freqs;
>>       cpumask_t online_policy_cpus;
>> -    struct drv_cmd cmd;
>> -    unsigned int next_state = 0; /* Index into freq_table */
>> -    unsigned int next_perf_state = 0; /* Index into perf table */
>> -    int result = 0;
>> -    int j = 0;
>> +    unsigned int next_state; /* Index into freq_table */
>> +    unsigned int next_perf_state; /* Index into perf table */
>> +    int result;
>> +    int j;
>>
>>       if (unlikely(data == NULL ||
>>           data->acpi_data == NULL || data->freq_table == NULL)) {
>> @@ -125,9 +112,7 @@ static int powernow_cpufreq_target(struc
>>                                               target_freq,
>>                                               relation, &next_state);
>>       if (unlikely(result))
>> -        return -ENODEV;
>> -
>> -    cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
>> +        return result;
>>
>>       next_perf_state = data->freq_table[next_state].index;
>>       if (perf->state == next_perf_state) {
>> @@ -137,26 +122,28 @@ static int powernow_cpufreq_target(struc
>>               return 0;
>>       }
>>
>> -    if (policy->shared_type != CPUFREQ_SHARED_TYPE_ANY)
>> -        cmd.mask = &online_policy_cpus;
>> -    else
>> -        cmd.mask = cpumask_of(policy->cpu);
>> +    if (policy->shared_type == CPUFREQ_SHARED_TYPE_HW &&
>> +        likely(policy->cpu == smp_processor_id())) {
>> +        transition_pstate(&next_perf_state);
>> +        cpufreq_statistic_update(policy->cpu, perf->state, next_perf_state);
>> +    } else {
>> +        cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
>>
>> -    freqs.old = perf->states[perf->state].core_frequency * 1000;
>> -    freqs.new = data->freq_table[next_state].frequency;
>> +        if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
>> +            unlikely(policy->cpu != smp_processor_id()))
>> +            on_selected_cpus(&online_policy_cpus, transition_pstate,
>> +                             &next_perf_state, 1);
>> +        else
>> +            transition_pstate(&next_perf_state);
>>
>> -    cmd.val = next_perf_state;
>> -    cmd.turbo = policy->turbo;
>> -
>> -    on_selected_cpus(cmd.mask, transition_pstate, &cmd, 1);
>> -
>> -    for_each_cpu(j, &online_policy_cpus)
>> -        cpufreq_statistic_update(j, perf->state, next_perf_state);
>> +        for_each_cpu(j, &online_policy_cpus)
>> +            cpufreq_statistic_update(j, perf->state, next_perf_state);
>> +    }
>>
>>       perf->state = next_perf_state;
>> -    policy->cur = freqs.new;
>> +    policy->cur = data->freq_table[next_state].frequency;
>>
>> -    return result;
>> +    return 0;
>>   }
>>
>>   static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
>

Re: [PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL

[Jan Beulich]

>>> On 16.08.12 at 18:41, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> @@ -137,26 +122,28 @@ static int powernow_cpufreq_target(struc
>              return 0;
>      }
>  
> -    if (policy->shared_type != CPUFREQ_SHARED_TYPE_ANY)
> -        cmd.mask = &online_policy_cpus;
> -    else
> -        cmd.mask = cpumask_of(policy->cpu);
> +    if (policy->shared_type == CPUFREQ_SHARED_TYPE_HW &&
> +        likely(policy->cpu == smp_processor_id())) {
> +        transition_pstate(&next_perf_state);
> +        cpufreq_statistic_update(policy->cpu, perf->state, next_perf_state);

Actually - is this enough? Doesn't this also need to be done based
on policy->cpus?

Jan

> +    } else {
> +        cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
>  
> -    freqs.old = perf->states[perf->state].core_frequency * 1000;
> -    freqs.new = data->freq_table[next_state].frequency;
> +        if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
> +            unlikely(policy->cpu != smp_processor_id()))
> +            on_selected_cpus(&online_policy_cpus, transition_pstate,
> +                             &next_perf_state, 1);
> +        else
> +            transition_pstate(&next_perf_state);
>  
> -    cmd.val = next_perf_state;
> -    cmd.turbo = policy->turbo;
> -
> -    on_selected_cpus(cmd.mask, transition_pstate, &cmd, 1);
> -
> -    for_each_cpu(j, &online_policy_cpus)
> -        cpufreq_statistic_update(j, perf->state, next_perf_state);
> +        for_each_cpu(j, &online_policy_cpus)
> +            cpufreq_statistic_update(j, perf->state, next_perf_state);
> +    }    
>  
>      perf->state = next_perf_state;
> -    policy->cur = freqs.new;
> +    policy->cur = data->freq_table[next_state].frequency;
>  
> -    return result;
> +    return 0;
>  }
>  
>  static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL

[Boris Ostrovsky]

On 08/17/2012 06:31 AM, Jan Beulich wrote:
>>>> On 16.08.12 at 18:41, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
>> @@ -137,26 +122,28 @@ static int powernow_cpufreq_target(struc
>>               return 0;
>>       }
>>
>> -    if (policy->shared_type != CPUFREQ_SHARED_TYPE_ANY)
>> -        cmd.mask = &online_policy_cpus;
>> -    else
>> -        cmd.mask = cpumask_of(policy->cpu);
>> +    if (policy->shared_type == CPUFREQ_SHARED_TYPE_HW &&
>> +        likely(policy->cpu == smp_processor_id())) {
>> +        transition_pstate(&next_perf_state);
>> +        cpufreq_statistic_update(policy->cpu, perf->state, next_perf_state);
>
> Actually - is this enough? Doesn't this also need to be done based
> on policy->cpus?

With HW-coordinated transitions there is a policy structure per CPU so 
policy->cpus is always 1 and policy->cpu is the same as policy->cpus. 
You can see this in cpufreq_add_cpu(), when hw_all is set.

(This is consistent with ACPI spec:
	When hardware coordinates transitions, OSPM continues to
	initiate state transitions as it would if there were no
	dependencies.
)

-boris


>
> Jan
>
>> +    } else {
>> +        cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
>>
>> -    freqs.old = perf->states[perf->state].core_frequency * 1000;
>> -    freqs.new = data->freq_table[next_state].frequency;
>> +        if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
>> +            unlikely(policy->cpu != smp_processor_id()))
>> +            on_selected_cpus(&online_policy_cpus, transition_pstate,
>> +                             &next_perf_state, 1);
>> +        else
>> +            transition_pstate(&next_perf_state);
>>
>> -    cmd.val = next_perf_state;
>> -    cmd.turbo = policy->turbo;
>> -
>> -    on_selected_cpus(cmd.mask, transition_pstate, &cmd, 1);
>> -
>> -    for_each_cpu(j, &online_policy_cpus)
>> -        cpufreq_statistic_update(j, perf->state, next_perf_state);
>> +        for_each_cpu(j, &online_policy_cpus)
>> +            cpufreq_statistic_update(j, perf->state, next_perf_state);
>> +    }
>>
>>       perf->state = next_perf_state;
>> -    policy->cur = freqs.new;
>> +    policy->cur = data->freq_table[next_state].frequency;
>>
>> -    return result;
>> +    return 0;
>>   }
>>
>>   static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
>
>

Re: [PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL

[Jan Beulich]

>>> On 17.08.12 at 15:50, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> On 08/17/2012 06:31 AM, Jan Beulich wrote:
>>>>> On 16.08.12 at 18:41, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
>>> @@ -137,26 +122,28 @@ static int powernow_cpufreq_target(struc
>>>               return 0;
>>>       }
>>>
>>> -    if (policy->shared_type != CPUFREQ_SHARED_TYPE_ANY)
>>> -        cmd.mask = &online_policy_cpus;
>>> -    else
>>> -        cmd.mask = cpumask_of(policy->cpu);
>>> +    if (policy->shared_type == CPUFREQ_SHARED_TYPE_HW &&
>>> +        likely(policy->cpu == smp_processor_id())) {
>>> +        transition_pstate(&next_perf_state);
>>> +        cpufreq_statistic_update(policy->cpu, perf->state, next_perf_state);
>>
>> Actually - is this enough? Doesn't this also need to be done based
>> on policy->cpus?
> 
> With HW-coordinated transitions there is a policy structure per CPU so 
> policy->cpus is always 1 and policy->cpu is the same as policy->cpus. 
> You can see this in cpufreq_add_cpu(), when hw_all is set.
> 
> (This is consistent with ACPI spec:
> 	When hardware coordinates transitions, OSPM continues to
> 	initiate state transitions as it would if there were no
> 	dependencies.
> )

Ah, okay, I didn't recall that (which means in this case the stats
simply can't be right, as the hardware may do as it pleases).

Jan