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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.06.13 at 19:04, Jacob Shin <jacob.shin@xxxxxxx> 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


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