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
| ||
|
Message-ID: <20151027025339.GF8036@hr-amur2> Date: Tue, 27 Oct 2015 10:53:40 +0800 From: Huang Rui <ray.huang@....com> To: Borislav Petkov <bp@...e.de> CC: Guenter Roeck <linux@...ck-us.net>, Peter Zijlstra <peterz@...radead.org>, Jean Delvare <jdelvare@...e.de>, Andy Lutomirski <luto@...capital.net>, Andreas Herrmann <herrmann.der.user@...il.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...nel.org>, "Rafael J. Wysocki" <rjw@...ysocki.net>, Len Brown <lenb@...nel.org>, John Stultz <john.stultz@...aro.org>, Frédéric Weisbecker <fweisbec@...il.com>, <lm-sensors@...sensors.org>, <linux-kernel@...r.kernel.org>, <x86@...nel.org>, Andreas Herrmann <herrmann.der.user@...glemail.com>, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@....com>, Fengguang Wu <fengguang.wu@...el.com>, Aaron Lu <aaron.lu@...el.com>, Tony Li <tony.li@....com> 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@...en8.de> > > Signed-off-by: Huang Rui <ray.huang@....com> > > Cc: Guenter Roeck <linux@...ck-us.net> > > Cc: Peter Zijlstra <peterz@...radead.org> > > Cc: Ingo Molnar <mingo@...nel.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 majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists