[PATCH 0/3] remove unnecessary IPI reading uncore events

[PATCH 0/3] remove unnecessary IPI reading uncore events

[David Carrillo-Cisneros]

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(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/3] perf/core: check return value of perf_event_read IPI

[David Carrillo-Cisneros]

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.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Reviewed-by: Stephane Eranian <eranian@google.com>
---
 kernel/events/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9345028..ebc187f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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;
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[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)
  - 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

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Reviewed-by: Stephane Eranian <eranian@google.com>
---
 include/linux/perf_event.h | 12 ++++++++++
 kernel/events/core.c       | 55 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 7921f4f..d026166 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -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
  *
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1c30f52..c2c6ad0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3336,6 +3336,39 @@ struct perf_read_data {
 	int ret;
 };
 
+static inline bool can_read_inactive(struct perf_event *event)
+{
+	return event->cpu >= 0 &&
+		(event->pmu_event_flags & PMUEF_READ_CPU_PKG);
+}
+
+static int find_cpu_to_read(struct perf_event *event)
+{
+	bool active = event->state == PERF_EVENT_STATE_ACTIVE;
+	int event_cpu = active ? event->oncpu : event->cpu;
+	int local_cpu = smp_processor_id();
+	u16 local_pkg, event_pkg;
+
+	if (event_cpu < 0)
+		return -1;
+
+	if (event->pmu_event_flags & PMUEF_READ_CPU_PKG) {
+		/*
+		 * Event with PMUEF_READ_CPU_PKG can be read in local CPU if:
+		 *   - CPU where event runs is on local CPU's pkg, or
+		 *   - event is bound to a CPU on local CPU's pkg
+		 *     (even if event is not in active state).
+		 */
+		event_pkg =  topology_physical_package_id(event_cpu);
+		local_pkg =  topology_physical_package_id(local_cpu);
+
+		if (event_pkg == local_pkg)
+			return local_cpu;
+	}
+
+	return event_cpu;
+}
+
 /*
  * Cross CPU call to read the hardware event
  */
@@ -3346,6 +3379,7 @@ static void __perf_event_read(void *info)
 	struct perf_event_context *ctx = event->ctx;
 	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
 	struct pmu *pmu = event->pmu;
+	bool active, read_inactive = can_read_inactive(event);
 
 	/*
 	 * If this is a task context, we need to check whether it is
@@ -3354,7 +3388,7 @@ static void __perf_event_read(void *info)
 	 * event->count would have been updated to a recent sample
 	 * when the event was scheduled out.
 	 */
-	if (ctx->task && cpuctx->task_ctx != ctx)
+	if (ctx->task && cpuctx->task_ctx != ctx && !read_inactive)
 		return;
 
 	raw_spin_lock(&ctx->lock);
@@ -3364,7 +3398,9 @@ static void __perf_event_read(void *info)
 	}
 
 	update_event_times(event);
-	if (event->state != PERF_EVENT_STATE_ACTIVE)
+
+	active = event->state == PERF_EVENT_STATE_ACTIVE;
+	if (!active && !read_inactive)
 		goto unlock;
 
 	if (!data->group) {
@@ -3379,7 +3415,8 @@ static void __perf_event_read(void *info)
 
 	list_for_each_entry(sub, &event->sibling_list, group_entry) {
 		update_event_times(sub);
-		if (sub->state == PERF_EVENT_STATE_ACTIVE) {
+		active = sub->state == PERF_EVENT_STATE_ACTIVE;
+		if (active || can_read_inactive(sub)) {
 			/*
 			 * Use sibling's PMU rather than @event's since
 			 * sibling could be on different (eg: software) PMU.
@@ -3457,19 +3494,17 @@ u64 perf_event_read_local(struct perf_event *event)
 
 static int perf_event_read(struct perf_event *event, bool group)
 {
-	int ret = 0;
+	int ret = 0, cpu_to_read;
 
-	/*
-	 * If event is enabled and currently active on a CPU, update the
-	 * value in the event structure:
-	 */
-	if (event->state == PERF_EVENT_STATE_ACTIVE) {
+	cpu_to_read = find_cpu_to_read(event);
+
+	if (cpu_to_read >= 0) {
 		struct perf_read_data data = {
 			.event = event,
 			.group = group,
 			.ret = 0,
 		};
-		ret = smp_call_function_single(event->oncpu,
+		ret = smp_call_function_single(cpu_to_read,
 					       __perf_event_read, &data, 1);
 		ret = ret ? : data.ret;
 	} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/3] perf/x86: use PMUEF_READ_CPU_PKG in uncore events

[David Carrillo-Cisneros]

Add flag to Intel's uncore and RAPL.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Reviewed-by: Stephane Eranian <eranian@google.com>
---
 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(+)

diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index d0c58b3..a423d7d 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -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)
 	 */
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index dc965d2..ba3ebba 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -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;
diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index 96531d2..c70bfc6 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -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;
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 0/3] remove unnecessary IPI reading uncore events

[Andi Kleen]

Patches look good to me.
-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> +++ b/include/linux/perf_event.h
> @@ -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

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> 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..

>   - 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

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> +/*
> + * 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.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[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..
>
>>   - 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.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[David Carrillo-Cisneros]

>>       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?

Sounds good, I will do that.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Thu, Aug 04, 2016 at 10:23:43AM -0700, David Carrillo-Cisneros wrote:
> >> 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..
> >
> >>   - 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

[David Carrillo-Cisneros]

On Thu, Aug 4, 2016 at 10:27 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Aug 04, 2016 at 10:23:43AM -0700, David Carrillo-Cisneros wrote:
>> >> 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..
>> >
>> >>   - 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.
>
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.