Messages in this thread | | | Date | Mon, 28 Oct 2013 11:33:50 +0100 | Subject | Re: [PATCH v3 3/4] perf,x86: add Intel RAPL PMU support | From | Stephane Eranian <> |
| |
Hi,
I was thinking about the scaling issue over the week-end.
We agreed that it was necessary to export the scaling via sysfs per event. The RAPL v3 has an implementation of that including in the perf tool.
If we have that, then it may not be necessary anymore to express the raw count in the 1/2^32 J unit like we are currently doing. This loses a bit of precision. We could as well expose the actual raw count and export the actual unit via sysfs. For instance, on SNB/IVB the unit is 1/2^16, but on Haswell it is 1/2^14.
I see two issues with that approach though:
- the interpretation of the raw count changes from machine to machine and needs to ALWAYS be used with the scaling. It is not always possible to just compare raw counts directly.
- we would need a way to express that ratio without actually calculating it in the kernel. There are 6 ratios possible. So we either have a lookup table with the floating point values already computed and encoded as strings. Or we need to add a calculator style parsing in the perf tool (or any other) tool to parse a ratio: 1 / 65536 or any basic mathematical expression. After all, the scaling support has to be generic. Other events may use a different form of scaling ratios. But that seems overkill to me.
So in the end, it may be the case that what we have not in RAPLv3 is the simplest approach.
Any opinion?
On Sat, Oct 26, 2013 at 7:00 PM, Stephane Eranian <eranian@google.com> wrote: > On Fri, Oct 25, 2013 at 1:14 PM, Jiri Olsa <jolsa@redhat.com> wrote: >> On Wed, Oct 23, 2013 at 02:58:04PM +0200, Stephane Eranian wrote: >> >> SNIP >> >>> + >>> +static void rapl_init_cpu(int cpu) >>> +{ >>> + int i, phys_id = topology_physical_package_id(cpu); >>> + >>> + spin_lock(&rapl_hotplug_lock); >>> + >>> + /* check if phys_is is already covered */ >>> + for_each_cpu(i, &rapl_cpu_mask) { >>> + if (phys_id == topology_physical_package_id(i)) >>> + return; >> >> missing 'spin_unlock(&rapl_hotplug_lock)' above >> > Good catch. I fixed that now. > >>> + } >>> + /* was not found, so add it */ >>> + cpumask_set_cpu(cpu, &rapl_cpu_mask); >>> + >>> + spin_unlock(&rapl_hotplug_lock); >>> +} >>> +
| |