remove unnecessary IPI reading uncore events

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 <[email protected]>

Reviewed-by Stephane Eranian <[email protected]>

---
 kernel/events/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/events/core.c +0 -0

index 9345028..96523fd 100644
@@ -3469,9 +3469,9 @@ static int perf_event_read(struct perf_event *event, bool group)
 			.group = group,
 			.ret = 0,

Quoted 3 lines

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.

Please also describe the symptoms of the bug - under what circumstances may the smp-call fail and how does the user notice?

Thanks,

Ingo

kernel/events/core.c +24 -6

index 34049cc..77f1bd3 100644
@@ -3333,6 +3333,26 @@ struct perf_read_data {
 	int ret;
 };
 
+static int find_cpu_to_read(struct perf_event *event)
+{
+	bool active = event->state == PERF_EVENT_STATE_ACTIVE;
+	int event_cpu = event->oncpu, local_cpu = smp_processor_id();
+	u16 local_pkg, event_pkg;
+
+	if (!active)
+		return -1;
+
+	if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
+		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
  */

@@ -3454,19 +3474,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) {

Quoted 21 lines

static int perf_event_read(struct perf_event *event, bool group) {

kernel/events/core.c +5 -7

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

Quoted 4 lines

1);
                ret = ret ? : data.ret;
        } else if (event->state == PERF_EVENT_STATE_INACTIVE) {

I would like to suggest a small change to this patch. I think the check on PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read() function. The new function should assume that the event is active. I find this more readable since the next check in function perf_event_read() is on PERF_EVENT_STATE_INACTIVE.

Hi Nilay,

Quoted 26 lines

Quoted 21 lines

static int perf_event_read(struct perf_event *event, bool group) {

kernel/events/core.c +5 -7

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

Quoted 4 lines

1);
                ret = ret ? : data.ret;
        } else if (event->state == PERF_EVENT_STATE_INACTIVE) {

Quoted 5 lines

I would like to suggest a small change to this patch. I think the check on PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read() function. The new function should assume that the event is active. I find this more readable since the next check in function perf_event_read() is on PERF_EVENT_STATE_INACTIVE.

Two oncoming flags that Intel CQM/CMT will use are meant to allow read even if event is inactive. This makes sense in CQM/CMT because the hw RMID is always reserved. I am ok with keeping the check for STATE_ACTIVE until those flags are actually introduced, tough.

Quoted 34 lines · David Carrillo-Cisneros

Hi Nilay,

Quoted 26 lines

Quoted 21 lines

static int perf_event_read(struct perf_event *event, bool group) {

kernel/events/core.c +5 -7

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

Quoted 4 lines

1);
                ret = ret ? : data.ret;
        } else if (event->state == PERF_EVENT_STATE_INACTIVE) {

Quoted 5 lines

I would like to suggest a small change to this patch. I think the check on PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read() function. The new function should assume that the event is active. I find this more readable since the next check in function perf_event_read() is on PERF_EVENT_STATE_INACTIVE.

Quoted 4 lines

Two oncoming flags that Intel CQM/CMT will use are meant to allow read even if event is inactive. This makes sense in CQM/CMT because the hw RMID is always reserved. I am ok with keeping the check for STATE_ACTIVE until those flags are actually introduced, tough.

Hello David

Lets go with checking PERF_EVENT_STATE_ACTIVE in perf_event_read() for the time being. With the new version of the patch that you posted, I find that checking PERF_EVENT_STATE_ACTIVE in find_cpu_to_read() makes you introduce another if statement for checking STATE_INACTIVE.

If your CQM/CMT patches later need the code structure you have now, I would also support it. But as of now, I think, it is better to check STATE_ACTIVE in perf_event_read().

Thanks
Nilay

Introduce the flag PMU_EV_CAP_READ_ACTIVE_PKG, useful for uncore events, that allows a PMU to signal the generic perf code that an event is readable in the current CPU if the event is active in a CPU 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.

For the optimization to work, all events in the group must have it (similarly to PERF_EV_CAP_SOFTWARE).

Signed-off-by David Carrillo-Cisneros <[email protected]>

---
 include/linux/perf_event.h |  3 +++
 kernel/events/core.c       | 26 ++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

include/linux/perf_event.h +3 -0

index fa5617f..c8bb1b3 100644
@@ -501,8 +501,11 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
  * Event capabilities. For event_caps and groups caps.
  *
  * PERF_EV_CAP_SOFTWARE: Is a software event.
+ * PERF_EV_CAP_READ_ACTIVE_PKG: A CPU event (or cgroup event) that can be read
+ * from any CPU in the package where it is active.
  */
 #define PERF_EV_CAP_SOFTWARE           BIT(0)
+#define PERF_EV_CAP_READ_ACTIVE_PKG    BIT(1)
 
 #define SWEVENT_HLIST_BITS             8
 #define SWEVENT_HLIST_SIZE             (1 << SWEVENT_HLIST_BITS)

kernel/events/core.c +24 -3

index 34049cc..38ec68d 100644
@@ -3333,6 +3333,22 @@ struct perf_read_data {
        int ret;
 };
 
+static int find_cpu_to_read(struct perf_event *event, int local_cpu)
+{
+       int event_cpu = event->oncpu;
+       u16 local_pkg, event_pkg;
+
+       if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
+               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
  */

@@ -3454,7 +3470,7 @@ 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, local_cpu;
 
        /*
         * If event is enabled and currently active on a CPU, update the

@@ -3466,8 +3482,14 @@ static int perf_event_read(struct perf_event *event, bool group)
                        .group = group,
                        .ret = 0,
                };
-               ret = smp_call_function_single(event->oncpu,
+
+               local_cpu = get_cpu();
+               cpu_to_read = find_cpu_to_read(event, local_cpu);
+               put_cpu();
+
+               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) {
                struct perf_event_context *ctx = event->ctx;
--

2.8.0.rc3.226.g39d4020

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 where the is 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 3 new flags that use the pmu_event_flags field (added in patch 03 in this series).

Patch 02 generalizes event->group_flags so that new flags can use it in a similar way that PERF_GROUP_SOFTWARE was used.

Patches rebased at peterz/queue/perf/core

Changes in v2:
- Change logic to use event->group_flags instead of individually testing sibling in perf_event_read.
- Remove erroneous read of inactive events.

David Carrillo-Cisneros (4):
perf/core: check return value of perf_event_read IPI perf/core: generalize event->group_flags perf/core: introduce PMU_EV_CAP_READ_ACTIVE_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         | 21 +++++++++++----
 kernel/events/core.c               | 52 +++++++++++++++++++++++++-------------
 5 files changed, 57 insertions(+), 22 deletions(-)

--
2.8.0.rc3.226.g39d4020