Re: [PATCH V7 08/14] perf/x86/intel: Generic support for hardware TopDown metrics

From: Liang, Kan
Date: Tue Jul 28 2020 - 10:01:30 EST

On 7/28/2020 9:44 AM, peterz@xxxxxxxxxxxxx wrote:
On Tue, Jul 28, 2020 at 09:28:39AM -0400, Liang, Kan wrote:

On 7/28/2020 9:09 AM, Peter Zijlstra wrote:
On Fri, Jul 24, 2020 at 03:10:52PM -0400, Liang, Kan wrote:

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 6cb079e0c9d9..010ac74afc09 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2405,27 +2405,18 @@ static u64 icl_update_topdown_event(struct
perf_event *event)
return slots;

-static void intel_pmu_read_topdown_event(struct perf_event *event)
+static void intel_pmu_read_event(struct perf_event *event)
- struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
- /* Only need to call update_topdown_event() once for group read. */
- if ((cpuc->txn_flags & PERF_PMU_TXN_READ) &&
- !is_slots_event(event))

- perf_pmu_disable(event->pmu);
- x86_pmu.update_topdown_event(event);
- perf_pmu_enable(event->pmu);
-static void intel_pmu_read_event(struct perf_event *event)
if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)
- else if (is_topdown_count(event) && x86_pmu.update_topdown_event)
- intel_pmu_read_topdown_event(event);
- else
+ else if (is_slots_count(event) && x86_pmu.update_topdown_event) {
+ perf_pmu_disable(event->pmu);
+ x86_pmu.update_topdown_event(event);
+ perf_pmu_enable(event->pmu);
+ } else

I'm a little puzzled by this; what happens if you:

fd = sys_perf_event_open(&attr_slots);
fd1 = sys_perf_event_open(&attr_metric, .group_fd=fd);



I did a quick test. It depends on the .read_format of attr_metric.
If PERF_FORMAT_GROUP is applied for attr_metric, perf_read_group() will be
invoked. The value of fd1 is updated correctly.
If the flag is not applied, 0 will be returned.

Exactly :-), was that intentional?

Kind of, because a metric event must be in a group with the leader slots event. If a user reads (treats) the metric event as a singleton event, an invalid value should be expected.

Because prior to this change that
would've worked just fine.

Now, I agree it's a daft thing, because that value is pretty useless
without also having the slots value, but I feel we should be explicit in
our choices here.

If for example, we would've had hardware provide us the raw metric
counters, instead of us having to reconstruct them, this would've been a
semi useful thing.

So I'm tempted to leave things as it, and keep this 'working'.

I will update the perf tool document to force the PERF_FORMAT_GROUP flag for each metric events.