remove unnecessary IPI reading uncore events
Subsystems (2)
cover remove unnecessary IPI reading uncore events
This patch series adds a new flag to the struct perf_event (and a flag field to store it) to allow a PMU to tag a CPU or cgroup event as readable from any CPU in the same package and not just the CPU the event is attached to, even if the event is not currently active.
This capability is used with uncore events to potentially avoid an unnecessary IPI when executing perf_event_read.
A previous version of this change was introduced in the last Intel's CQM/CMT driver series (under review), but now we present it separately here since it is also useful for other uncore events.
The next version of Intel CQM/CMT will add 2 new flags that use the pmu_event_flags field (added in patch 01 in this series).
Patches rebased at peterz/queue/perf/core
David Carrillo-Cisneros (3):
perf/core: check return value of perf_event_read IPI perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG perf/x86: use PMUEF_READ_CPU_PKG in uncore events
arch/x86/events/intel/rapl.c | 2 ++ arch/x86/events/intel/uncore.c | 2 ++ arch/x86/events/intel/uncore_snb.c | 2 ++ include/linux/perf_event.h | 12 ++++++++ kernel/events/core.c | 60 ++++++++++++++++++++++++++++++-------- 5 files changed, 66 insertions(+), 12 deletions(-)
Signature · 1 line
2.8.0.rc3.226.g39d4020
PATCH 1/3 perf/core: check return value of perf_event_read IPI
The call to smp_call_function_single in perf_event_read() may fail and, when it does, its error value is the one to return.
--- kernel/events/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/events/core.c +3 -3
@@ -3469,9 +3469,9 @@ static int perf_event_read(struct perf_event *event, bool group) .group = group, .ret = 0, }; -smp_call_function_single (event->oncpu, - __perf_event_read, &data,1 ); + ret =smp_call_function_single (event->oncpu, + __perf_event_read, &data,1 ); - ret = data.ret; + ret = ret ? : data.ret; }else if (event->state ==PERF_EVENT_STATE_INACTIVE ) {struct perf_event_context *ctx = event->ctx;unsigned long flags;
Signature · 1 line
2.8.0.rc3.226.g39d4020
PATCH 2/3 perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG
In reply to David Carrillo-CisnerosIntroduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that allows a PMU to signal the generic perf code that an event is readable on the current CPU if the event is:
- active in a CPU in the same package as the current CPU (local CPU)
- not active but is attached to a CPU (i.e. event->cpu != -1) in the same package as the current CPU.
This is an optimization that avoids a unnecessary IPI for the common case where uncore events are run and read in the same package but in different CPUs.
As an example, the IPI removal speeds up perf_read in my Haswell system as follows:
- For event UNC_C_LLC_LOOKUP: From 260 us to 31 us.
- For event RAPL's power/energy-cores/: From to 255 us to 27 us.
To store the flag PMUEF_READ_CPU_PKG, a new field, pmu_event_flags is added to perf_event struct. The flags in this field are intended to be set by the PMU in a per-event basis, allowing different events in the same PMU to have different flags on. This patch does not demonstrate that events in a PMU can have distinct flags, but the Intel CQM/CMT patch series does makes use of this and uses the PMUEF_READ_CPU_PKG in a per-event basis
--- include/linux/perf_event.h | 12 ++++++++++ kernel/events/core.c | 55 +++++++++++++++++++++++++++++++++++++--------- 2 files changed, 57 insertions(+), 10 deletions(-)
include/linux/perf_event.h +12 -0
@@ -681,9 +681,21 @@ struct perf_event {#endif struct list_head sb_list; + +/* Per-event flags to generic code set by PMU. */ +int pmu_event_flags; +#endif /* CONFIG_PERF_EVENTS */ }; +/* +* Flags for pmu_event_flags. +* +* PMUEF_READ_CPU_PKG: A CPU event (or cgroup event) that can be read in +* any CPU in event->cpu's package, even if inactive. +*/ +#define PMUEF_READ_CPU_PKG BIT(0) +/** * struct perf_event_context - event context structure *
kernel/events/core.c +45 -10
97 lines not on this page · raw message
Signature · 1 line
2.8.0.rc3.226.g39d4020
Peter Zijlstra
maintainerRe: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG
Quoted 4 lines · David Carrillo-Cisneros
Introduce the flag
PMUEF_READ_CPU_PKG, useful for uncore events, that allows a PMU to signal the generic perf code that an event is readable on the current CPU if the event is:
- active in a CPU in the same package as the current CPU (local CPU)
Ok that I get..
Quoted 2 lines
- not active but is attached to a CPU (i.e.
event->cpu!= -1) in the same package as the current CPU.
but this, not so much. Why would you want to read an inactive counter?
Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG
Quoted 11 lines
Quoted 4 lines
Introduce the flag
PMUEF_READ_CPU_PKG, useful for uncore events, that allows a PMU to signal the generic perf code that an event is readable on the current CPU if the event is:
- active in a CPU in the same package as the current CPU (local CPU)Ok that I get..
Quoted 2 lines
- not active but is attached to a CPU (i.e.
event->cpu!= -1) in the same package as the current CPU.but this, not so much. Why would you want to read an inactive counter?
Uncore counters are active even if its event is not.
Peter Zijlstra
maintainerRe: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG
Quoted 5 lines · David Carrillo-Cisneros
Quoted 5 lines
Quoted 4 lines
Introduce the flag
PMUEF_READ_CPU_PKG, useful for uncore events, that allows a PMU to signal the generic perf code that an event is readable on the current CPU if the event is:
- active in a CPU in the same package as the current CPU (local CPU)
Quoted 8 lines
Quoted 6 lines
Ok that I get..
Quoted 2 lines
- not active but is attached to a CPU (i.e.
event->cpu!= -1) in the same package as the current CPU.but this, not so much. Why would you want to read an inactive counter?
Uncore counters are active even if its event is not.
Not in general they are not, and if they are (freerunning msr counters for example) we should not include the counts when the event is inactive.
Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · David Carrillo-Cisneros
Quoted 10 lines
Quoted 4 lines
Introduce the flag
PMUEF_READ_CPU_PKG, useful for uncore events, that allows a PMU to signal the generic perf code that an event is readable on the current CPU if the event is:
- active in a CPU in the same package as the current CPU (local CPU)Ok that I get..
Quoted 2 lines
- not active but is attached to a CPU (i.e.
event->cpu!= -1) in the same package as the current CPU.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
but this, not so much. Why would you want to read an inactive counter?
Uncore counters are active even if its event is not.
Not in general they are not, and if they are (freerunning msr counters for example) we should not include the counts when the event is inactive.
True that. I conflated Intel CQM specific behavior with other uncore (In CQM we want to read when the event is inactive). I'll fix that in next version.
Peter Zijlstra
maintainerRe: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG
In reply to David Carrillo-Cisnerosinclude/linux/perf_event.h struct perf_event { · line 687
Quoted 9 lines · David Carrillo-Cisneros
+++
b/include/linux/perf_event.hinclude/linux/perf_event.h +4
-0@@ -681,9 +681,21 @@ struct perf_event {#endif struct list_head sb_list; + +/* Per-event flags to generic code set by PMU. */ +int pmu_event_flags; +
It appears to me we already have group_flags and attach_state which both are serialized by ctx->lock.
Could we maybe merge the lot into one flags field?
Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG
Quoted 10 lines
include/linux/perf_event.h struct perf_event { · line 687
Quoted 5 lines
struct list_head sb_list;include/linux/perf_event.h +4
-0+ +/* Per-event flags to generic code set by PMU. */ +int pmu_event_flags; +It appears to me we already have
group_flagsandattach_statewhich both are serialized byctx->lock.Could we maybe merge the lot into one flags field?
Sounds good, I will do that.
Peter Zijlstra
maintainerRe: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG
In reply to David Carrillo-Cisnerosinclude/linux/perf_event.h struct perf_event { · line 697
Quoted 7 lines · David Carrillo-Cisneros
include/linux/perf_event.h +7
-0+/* +* Flags for pmu_event_flags. +* +* PMUEF_READ_CPU_PKG: A CPU event (or cgroup event) that can be read in +* any CPU in event->cpu's package, even if inactive. +*/ +#define PMUEF_READ_CPU_PKG BIT(0)
So I don't object to the idea, but this flags thing seems somewhat awkward to express 'random' topology constraints.
For example, the AMD Fam15 NB driver could use something similar, but I'm not sure their NB-id matches our topo package id.
PATCH 3/3 perf/x86: use PMUEF_READ_CPU_PKG in uncore events
In reply to David Carrillo-CisnerosAdd flag to Intel's uncore and RAPL.
--- arch/x86/events/intel/rapl.c | 2 ++ arch/x86/events/intel/uncore.c | 2 ++ arch/x86/events/intel/uncore_snb.c | 2 ++ 3 files changed, 6 insertions(+)
arch/x86/events/intel/rapl.c +2 -0
@@ -357,6 +357,8 @@ static int rapl_pmu_event_init(struct perf_event *event)if (event->cpu <0 )return -EINVAL ; + event->pmu_event_flags =PMUEF_READ_CPU_PKG ; +/* * check event is known (determines counter) */
arch/x86/events/intel/uncore.c +2 -0
@@ -662,6 +662,8 @@ static int uncore_pmu_event_init(struct perf_event *event) event->cpu = box->cpu; event->pmu_private = box; + event->pmu_event_flags =PMUEF_READ_CPU_PKG ; + event->hw.idx =-1 ; event->hw.last_tag = ~0ULL ; event->hw.extra_reg.idx =EXTRA_REG_NONE ;
arch/x86/events/intel/uncore_snb.c +2 -0
@@ -315,6 +315,8 @@ static int snb_uncore_imc_event_init(struct perf_event *event) event->cpu = box->cpu; event->pmu_private = box; + event->pmu_event_flags =PMUEF_READ_CPU_PKG ; + event->hw.idx =-1 ; event->hw.last_tag = ~0ULL ; event->hw.extra_reg.idx =EXTRA_REG_NONE ;
Signature · 1 line
2.8.0.rc3.226.g39d4020
Patches look good to me.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only