Messages in this thread | | | Date | Tue, 27 Oct 2015 10:53:40 +0800 | From | Huang Rui <> | Subject | Re: [PATCH v2 05/10] hwmon: (fam15h_power) Add compute unit accumulated power |
| |
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
| |