Re: [PATCH v3 3/4] perf,x86: add Intel RAPL PMU support

[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@xxxxxxxxxx> wrote:
> On Fri, Oct 25, 2013 at 1:14 PM, Jiri Olsa <jolsa@xxxxxxxxxx> 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);
>>> +}
>>> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/