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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxx>]

On 08/17/2012 06:19 AM, Jan Beulich wrote:
> > > > On 16.08.12 at 18:41, Boris Ostrovsky <boris.ostrovsky@xxxxxxx> 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@xxxxxxx>
> 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@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel