Re: [PATCH 2/9] perf/x86/intel: Basic support for metrics counters

From: Liang, Kan
Date: Tue May 28 2019 - 14:24:19 EST




On 5/28/2019 8:05 AM, Peter Zijlstra wrote:
On Tue, May 21, 2019 at 02:40:48PM -0700, kan.liang@xxxxxxxxxxxxxxx wrote:
From: Andi Kleen <ak@xxxxxxxxxxxxxxx>

Metrics counters (hardware counters containing multiple metrics)
are modeled as separate registers for each TopDown metric events,
with an extra reg being used for coordinating access to the
underlying register in the scheduler.

This patch adds the basic infrastructure to separate the scheduler
register indexes from the actual hardware register indexes. In
most cases the MSR address is already used correctly, but for
code using indexes we need a separate reg_idx field in the event
to indicate the correct underlying register.

That doesn't parse. What exactly is the difference between reg_idx and
idx? AFAICT there is a fixed relation like:

reg_idx = is_metric_idx(idx) ? INTEL_PMC_IDX_FIXED_SLOTS : idx;

Do we really need a variable for that?

It may save the calculation. But, right, a variable is not necessary.


Also, why do we need that whole enabled_events[] array. Do we really not
have that information elsewhere?

No. We don't have a case that several events share a counter at the same time. We don't need to check if other events are enabled when we try to disable a counter. So we don't save such information.
But we have to do it for metrics events.

Thanks,
Kan

I shouldn've have to reverse engineer patches :/