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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Mar 2016 16:26:46 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Huang Rui <ray.huang@....com>
cc:	Borislav Petkov <bp@...e.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	Andy Lutomirski <luto@...capital.net>,
	Robert Richter <rric@...nel.org>,
	Jacob Shin <jacob.w.shin@...il.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Kan Liang <kan.liang@...el.com>, linux-kernel@...r.kernel.org,
	spg_linux_kernel@....com, x86@...nel.org,
	Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
	Aravind Gopalakrishnan <Aravind.Gopalakrishnan@....com>,
	Borislav Petkov <bp@...en8.de>,
	Fengguang Wu <fengguang.wu@...el.com>,
	Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v6 2/2] perf/x86/amd/power: Add AMD accumulated power
 reporting mechanism

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