Re: [PATCH v2 05/10] hwmon: (fam15h_power) Add compute unit accumulated power

[Huang Rui <ray.huang@amd.com>]

On Fri, Oct 23, 2015 at 03:27:02PM +0200, Borislav Petkov wrote:
> On Tue, Oct 20, 2015 at 10:28:24AM +0800, Huang Rui wrote:
> > This patch adds a member in fam15h_power_data which specifies the
> > compute unit accumulated power. It adds do_read_registers_on_cu to do
> > all the read to all MSRs and run it on one of the online cores on each
> > compute unit with smp_call_function_many(). This behavior can decrease
> > IPI numbers.
> > 
> > Suggested-by: Borislav Petkov <bp@alien8.de>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > ---
> >  drivers/hwmon/fam15h_power.c | 68 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 67 insertions(+), 1 deletion(-)
> > 

<snip>

> > +
> > +static int read_registers(struct fam15h_power_data *data)
> > +{
> > +	int this_cpu, ret;
> > +	int cu_num, cores_per_cu, cpu, cu;
> > +	cpumask_var_t mask;
> > +
> > +	cores_per_cu = amd_get_cores_per_cu();
> > +	cu_num = boot_cpu_data.x86_max_cores / cores_per_cu;
> > +
> > +	WARN_ON_ONCE(cu_num > MAX_CUS);
> > +
> > +	ret = zalloc_cpumask_var(&mask, GFP_KERNEL);
> > +	if (!ret)
> > +		return -ENOMEM;
> > +
> > +	this_cpu = get_cpu();
> 
> This should be get_online_cpus() and its counterpart below should be
> put_online_cpus().
> 

Preemption must be disabled when calling smp_call_function_many,
get_cpu would did that. Will get_online_cpus have the same behavior
like that?

> > +
> > +	/*
> > +	 * Choose the first online core of each compute unit, and then
> > +	 * read their MSR value of power and ptsc in one time of IPI,
> 
> 						in a single IPI.
> 
> > +	 * because the MSR value of cpu core represent the compute
> 
> s/cpu/CPU/
> 
> do that in *all* your text.
> 
> > +	 * unit's. This behavior can decrease IPI numbers between the
> 
> 	  unit's ?
> 
> What does that sentence even mean?
> 

That means "the value(cu_acc_power) of the compute unit", which does
not represent the value of one CPU core.

> > +	 * cores.
> > +	 */
> > +	cpu = cpumask_first(cpu_online_mask);
> > +	cu = cpu / cores_per_cu;
> > +	while (cpu < boot_cpu_data.x86_max_cores) {
> > +		if (cu <= cpu / cores_per_cu) {
> > +			cu = cpu / cores_per_cu + 1;
> > +			cpumask_set_cpu(cpu, mask);
> > +		}
> > +		cpu = cpumask_next(cu * cores_per_cu - 1, cpu_online_mask);
> > +	}
> 
> This is hard to parse - I *think* you're setting a bit in mask for a
> core in each CU...
> 

Yes, that's right.

My codes' behavior is below:

Assumed cores_per_cu is 4 and cu_number is 6, and the online cpumask
is:

        cu5  cu4  cu3  cu2  cu1  cu0
        1000_1100_0110_1011_0000_1111

After setting bits of the mask:

        1000_0100_0010_0001_0000_0001
        on   on   on   on   off  on

> If so, I think you can simplify it by generating a tmp mask which
> contains the cores of CU0, i.e. something like that:
> 
> 	11_00_00_...
> 
> and then do cpumask_and(res, ...) to find the online cores on that CU
> and then do cpumask_set_cpu(cpumask_any(res), mask) to select one CPU on
> that CU.
> 
> And then shift to the next CU:
> 
> 	cpumask_shift_right(dst, src_mask, cores_per_cu);
> 
> I think this should be cleaner and less error prone, without the
> conditionals...
> 

OK, how about below codes:

---
for (i = 0; i <= cores_per_cu / BITS_PER_LONG; i++) {
	offset = cores_per_cu % BITS_PER_LONG;
	if (i == cores_per_cu / BITS_PER_LONG) {
		cpumask_bits(src_mask)[i] = GENMASK(offset -1, 0);
		break;
	}
	cpumask_bits(src_mask)[i] = GENMASK(BITS_PER_LONG - 1, 0);
}

for (i = 0; i < cu_num; i++) {
	cpumask_shift_left(dst, src_mask, cores_per_cu * i);
	cpumask_and(res, dst, cpu_online_mask);
	cpumask_set_cpu(cpumask_any(res), mask);
}
---

Thanks,
Rui