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 <b...@alien8.de>
> > Signed-off-by: Huang Rui <ray.hu...@amd.com>
> > Cc: Guenter Roeck <li...@roeck-us.net>
> > Cc: Peter Zijlstra <pet...@infradead.org>
> > Cc: Ingo Molnar <mi...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
