Messages in this thread | | | Date | Thu, 3 Mar 2016 23:13:58 +0800 | From | Huang Rui <> | Subject | Re: [PATCH v6 2/2] perf/x86/amd/power: Add AMD accumulated power reporting mechanism |
| |
On Thu, Mar 03, 2016 at 09:50:11AM +0100, Thomas Gleixner wrote: > On Thu, 3 Mar 2016, Huang Rui wrote: > > + > > +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? >
Make sense. Thanks. Will update.
> > +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 .... >
OK, so I need to do power_cpu_init when notified CPU_DOWN_FAILED, am I right?
> > +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? >
Will use max_cu_acc_power directly.
> > + 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. >
When this driver is not as module before, I think there should be one sibling online at least at initialization phase. But now, you're right, we cannot guarantee it.
> > + 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? >
Because the sibling online shouldn't be empty at initialization phase if the driver is not module before. So...
Thanks to catch it.
How about below update:
for (i = 0; i < boot_cpu_data.x86_max_cores; i += smp_num_siblings) { if (!cpumask_empty(topology_sibling_cpumask(i))) cpumask_set_cpu(cpumask_any(topology_sibling_cpumask(i)), &cpu_mask); }
> > + } > > + > > + 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. ..... >
OK, so I should do __unregister_cpu_notifier(&power_cpu_notifier_nb) here.
> > + goto out; > > + } > > + > > + pr_info("AMD Power PMU detected, %d compute units\n", cu_num); > > Why is the number of compute units interesting at all? >
Because the accumulated power bases on compute units. We can see the mask from /sys/devices/power/cpumask and number of compute units to know if all compute units are set at cpumask. So I add a printk here, does it make sense?
Thanks, Rui
| |