On Thu, 3 Mar 2016, Huang Rui wrote:
> +/*
> + * The ratio of compute unit power accumulator sample period to the
> + * PTSC period.
> + */
> +static unsigned int cpu_pwr_sample_ratio;
> +static unsigned int cu_num;
Why do you need static storage for that information when the only purpose is
to printk it in init?
> +static void power_cpu_exit(int cpu)
> +{
> + int target = nr_cpumask_bits;
What's that initialization for?
> +
> + if (!cpumask_test_and_clear_cpu(cpu, &cpu_mask))
> + return;
> +
> + /*
> + * Find a new CPU on the same compute unit, if was set in cpumask
> + * and still some CPUs on compute unit. Then migrate event and
> + * context to new CPU.
> + */
> + target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu);
> + if (target < nr_cpumask_bits) {
> + cpumask_set_cpu(target, &cpu_mask);
> + perf_pmu_migrate_context(&pmu_class, cpu, target);
> + }
> +}
> +
> +static void power_cpu_init(int cpu)
> +{
> + /*
> + * 1) If any CPU is set at cpu_mask in the same compute unit, do
> + * nothing.
> + * 2) If no CPU is set at cpu_mask in the same compute unit,
> + * set current STARTING CPU.
> + *
> + * If cpu_mask and topology_sibling_cpumask has intersected
> + * bits, that means any CPU is set in the same compute unit.
> + * But cpumask_weight(topology_sibling_cpumask(cpu)) == 1
> + * means no CPU is set on cpu_mask in the same compute unit
> + * before init current STARTING CPU.
> + */
> + if (!cpumask_intersects(&cpu_mask, topology_sibling_cpumask(cpu)) &&
> + cpumask_weight(topology_sibling_cpumask(cpu)) == 1)
> + cpumask_set_cpu(cpu, &cpu_mask);
I don't think you need that complexity.
target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu);
if (target >= nr_cpumask_bits)
cpumask_set_cpu(cpu, &cpu_mask);
Simply because if there is a cpu aside of the new one already in the sibling
mask, then it is also in cpu_mask. Hmm?
> +static int
> +power_cpu_notifier(struct notifier_block *self, unsigned long action, void
> *hcpu)
> +{
> + unsigned int cpu = (long)hcpu;
> +
> + switch (action & ~CPU_TASKS_FROZEN) {
> + case CPU_STARTING:
> + power_cpu_init(cpu);
> + break;
> + case CPU_DOWN_PREPARE:
> + power_cpu_exit(cpu);
> + break;
And of course if CPU_DOWN_PREPARE fails and this is the last cpu in the
compute unit, nothing takes over the duty for this compute unit. So you need
to handle CPU_DOWN_FAILED ....
> +static int __init amd_power_pmu_init(void)
> +{
> + int i, ret;
> + u64 tmp;
> +
> + if (!x86_match_cpu(cpu_match))
> + return 0;
> +
> + if (!boot_cpu_has(X86_FEATURE_ACC_POWER))
> + return -ENODEV;
> +
> + cu_num = boot_cpu_data.x86_max_cores / smp_num_siblings;
> +
> + cpu_pwr_sample_ratio = cpuid_ecx(0x80000007);
> +
> + if (rdmsrl_safe(MSR_F15H_CU_MAX_PWR_ACCUMULATOR, &tmp)) {
> + pr_err("Failed to read max compute unit power accumulator
> MSR\n");
> + return -ENODEV;
> + }
> + max_cu_acc_power = tmp;
Why do you need an intermediate 'tmp' for this?
> + cpu_notifier_register_begin();
> +
> + /* Choose one online core of each compute unit. */
> + for (i = 0; i < boot_cpu_data.x86_max_cores; i += smp_num_siblings) {
> + WARN_ON(cpumask_empty(topology_sibling_cpumask(i)));
Err. What guarantees that in each compute unit is one sibling online? And what
value has that WARN_ON? We don't care about the stack trace here, because it's
known already.
> + cpumask_set_cpu(cpumask_any(topology_sibling_cpumask(i)),
> &cpu_mask);
Of course you just continue in that case and end up with:
cpumask_set_cpu(nr_cpu_ids, &cpu_mask);
i.e. you try to do that on an invalid bit, which will trigger a justified
warning in cpumask_set_cpu() if CONFIG_DEBUG_PER_CPU_MAPS is enabled.
Aside of that this only handles a single socket. And why do you do the above
if you handle the same thing in the loop below?
> + }
> +
> + for_each_online_cpu(i)
> + power_cpu_init(i);
> +
> + __register_cpu_notifier(&power_cpu_notifier_nb);
> +
> + ret = perf_pmu_register(&pmu_class, "power", -1);
> + if (WARN_ON(ret)) {
> + pr_warn("AMD Power PMU registration failed\n");
This still leaks the cpu notifier. .....
> + goto out;
> + }
> +
> + pr_info("AMD Power PMU detected, %d compute units\n", cu_num);
Why is the number of compute units interesting at all?
Thanks,
tglx
