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

[Boris Ostrovsky <boris.ostrovsky@amd.com>]

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