Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1348917 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 2/2] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Thomas Gleixner <tglx@linutronix.de> - 2016-03-03 10:00 +0100
    Re: [PATCH v6 2/2] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Thomas Gleixner <tglx@linutronix.de> - 2016-03-03 16:30 +0100
      Re: [PATCH v6 2/2] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Thomas Gleixner <tglx@linutronix.de> - 2016-03-03 19:00 +0100

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

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

[toc] | [next] | [standalone]

#1349286

On Thu, 3 Mar 2016, Huang Rui wrote:
> On Thu, Mar 03, 2016 at 09:50:11AM +0100, Thomas Gleixner wrote:
> > On Thu, 3 Mar 2016, Huang Rui wrote:
> > 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?

Yes.
 
> > > +	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);
> }

Why? You do a full for_each_online_cpu(i) loop after that, which does
exactly the same thing, right?
 
> > > +	}
> > > +
> > > +	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.

You can register the notifier after perf_pmu_register succeeded, right?
 
> > > +	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?

No, because it's completely non intuitive. How on earth am I supposed to get
the connection between /sys/devices/power/cpumask and number of compute units
without staring at the code? So this is only interesting for a developer who
can deduce that number from /proc/cpuinfo or dmesg as well.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1349459

On Fri, 4 Mar 2016, Huang Rui wrote:
> On Thu, Mar 03, 2016 at 04:26:46PM +0100, Thomas Gleixner wrote:
> > Why? You do a full for_each_online_cpu(i) loop after that, which does
> > exactly the same thing, right?
> >  
> 
> But looks like power_cpu_init cannot handle it if we don't take any
> action here.
> 
> e. g. 
> cpu_mask: 0000 and online mask: 1111 -> power_cpu_init(0) -> cpu_mask is still: 0000
> 
> topology_sibling_cpumask(0): 0011
> target: 1 (i. e. we cannot do cpumask_set_cpu(0, &cpu_mask))

Fair enough, but then you don't need the power_cpu_init() call at all.

But your loop does not cover anything beyond the first socket. So you need a
separate init function which does:

   for_each_online_cpu(cpu) {
   	target = cpumask_first(topology_sibling_cpumask(cpu));
	if (!cpumask_test_cpu(target, cpumask))
	   	cpumask_set_cpu(target, cpumask);
   }	      
	   	
Thanks,

	tglx

   

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web