Re: [PATCH v6 2/2] perf/x86/amd/power: Add AMD accumulated power reporting mechanism

[Huang Rui]

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