[PATCH v2 0/4] remove unnecessary IPI reading uncore events

[PATCH v2 0/4] 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 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

[PATCH v2 1/4] 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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9345028..96523fd 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 = data.ret;
+		ret = smp_call_function_single(event->oncpu,
+					       __perf_event_read, &data, 1);
+		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 v2 2/4] perf/core: generalize event->group_flags

[David Carrillo-Cisneros]

Currently, PERF_GROUP_SOFTWARE is used in the group_flags field of a
group's leader to indicate that is_software_event(event) is true for all
events in a group. This is the only usage of event->group_flags.

This pattern of setting a group level flags when all events in the group
share a property is useful for the flag introduced in the next patch and
for future CQM/CMT flags. So this patches generalizes group_flags to work
as an aggregate of event level flags.

PERF_GROUP_SOFTWARE denotes an inmutable event's property. All other flags
that I intend to add are also determinable at event initialization.
To better convey the above, this patch renames event's group_flags to
group_caps and PERF_GROUP_SOFTWARE to PERF_EV_CAP_SOFTWARE.

Individual event flags are stored in the new event->event_caps. Since the
cap flags do not change after event initialization, there is no need to
serialize event_caps. This new field is used when events are added to a
context, similarly to how PERF_GROUP_SOFTWARE and is_software_event()
worked.

Lastly, for consistency, updates is_software_event() to rely in event_cap
instead of the context index.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 include/linux/perf_event.h | 18 +++++++++++++-----
 kernel/events/core.c       | 16 ++++++++--------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 7921f4f..fa5617f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -497,9 +497,12 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
 					struct perf_sample_data *,
 					struct pt_regs *regs);
 
-enum perf_group_flag {
-	PERF_GROUP_SOFTWARE		= 0x1,
-};
+/*
+ * Event capabilities. For event_caps and groups caps.
+ *
+ * PERF_EV_CAP_SOFTWARE: Is a software event.
+ */
+#define PERF_EV_CAP_SOFTWARE		BIT(0)
 
 #define SWEVENT_HLIST_BITS		8
 #define SWEVENT_HLIST_SIZE		(1 << SWEVENT_HLIST_BITS)
@@ -555,7 +558,12 @@ struct perf_event {
 	struct hlist_node		hlist_entry;
 	struct list_head		active_entry;
 	int				nr_siblings;
-	int				group_flags;
+
+	/* Not serialized. Only written during event initialization. */
+	int				event_caps;
+	/* The cumulative AND of all event_caps for events in this group. */
+	int				group_caps;
+
 	struct perf_event		*group_leader;
 	struct pmu			*pmu;
 	void				*pmu_private;
@@ -968,7 +976,7 @@ static inline bool is_sampling_event(struct perf_event *event)
  */
 static inline int is_software_event(struct perf_event *event)
 {
-	return event->pmu->task_ctx_nr == perf_sw_context;
+	return event->event_caps & PERF_EV_CAP_SOFTWARE;
 }
 
 extern struct static_key perf_swevent_enabled[PERF_COUNT_SW_MAX];
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 96523fd..34049cc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1405,8 +1405,7 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
 	if (event->group_leader == event) {
 		struct list_head *list;
 
-		if (is_software_event(event))
-			event->group_flags |= PERF_GROUP_SOFTWARE;
+		event->group_caps = event->event_caps;
 
 		list = ctx_group_list(event, ctx);
 		list_add_tail(&event->group_entry, list);
@@ -1561,9 +1560,7 @@ static void perf_group_attach(struct perf_event *event)
 
 	WARN_ON_ONCE(group_leader->ctx != event->ctx);
 
-	if (group_leader->group_flags & PERF_GROUP_SOFTWARE &&
-			!is_software_event(event))
-		group_leader->group_flags &= ~PERF_GROUP_SOFTWARE;
+	group_leader->group_caps &= event->event_caps;
 
 	list_add_tail(&event->group_entry, &group_leader->sibling_list);
 	group_leader->nr_siblings++;
@@ -1669,7 +1666,7 @@ static void perf_group_detach(struct perf_event *event)
 		sibling->group_leader = sibling;
 
 		/* Inherit group flags from the previous leader */
-		sibling->group_flags = event->group_flags;
+		sibling->group_caps = event->group_caps;
 
 		WARN_ON_ONCE(sibling->ctx != event->ctx);
 	}
@@ -2070,7 +2067,7 @@ static int group_can_go_on(struct perf_event *event,
 	/*
 	 * Groups consisting entirely of software events can always go on.
 	 */
-	if (event->group_flags & PERF_GROUP_SOFTWARE)
+	if (event->group_caps & PERF_EV_CAP_SOFTWARE)
 		return 1;
 	/*
 	 * If an exclusive group is already on, no other hardware
@@ -9359,6 +9356,9 @@ SYSCALL_DEFINE5(perf_event_open,
 			goto err_alloc;
 	}
 
+	if (pmu->task_ctx_nr == perf_sw_context)
+		event->event_caps |= PERF_EV_CAP_SOFTWARE;
+
 	if (group_leader &&
 	    (is_software_event(event) != is_software_event(group_leader))) {
 		if (is_software_event(event)) {
@@ -9372,7 +9372,7 @@ SYSCALL_DEFINE5(perf_event_open,
 			 */
 			pmu = group_leader->pmu;
 		} else if (is_software_event(group_leader) &&
-			   (group_leader->group_flags & PERF_GROUP_SOFTWARE)) {
+			   (group_leader->group_caps & PERF_EV_CAP_SOFTWARE)) {
 			/*
 			 * In case the group is a pure software group, and we
 			 * try to add a hardware event, move the whole group to
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[David Carrillo-Cisneros]

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 <davidcc@google.com>
---
 include/linux/perf_event.h |  3 +++
 kernel/events/core.c       | 32 +++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index fa5617f..c8bb1b3 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -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)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 34049cc..77f1bd3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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) {
 		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 v2 4/4] 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..9922526 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->event_caps |= PERF_EV_CAP_READ_ACTIVE_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..dfb62ea 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->event_caps |= PERF_EV_CAP_READ_ACTIVE_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..a966c00 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->event_caps |= PERF_EV_CAP_READ_ACTIVE_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 v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[Nilay Vaish]

On 08/06/16 22:12, David Carrillo-Cisneros wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 34049cc..77f1bd3 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -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) {
>  		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) {
>

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.


Diff below:

+static int find_cpu_to_read(struct perf_event *event)
+{
+    int event_cpu = event->oncpu, local_cpu = smp_processor_id();
+    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
   */
@@ -3477,17 +3493,15 @@ static int perf_event_read(struct perf_event 
*event, bool group)
  {
         int ret = 0;

-       /*
-        * If event is enabled and currently active on a CPU, update the
-        * value in the event structure:
-        */
-       if (event->state == PERF_EVENT_STATE_ACTIVE) {
+    	if (event->state == PERF_EVENT_STATE_ACTIVE) {
+        	int cpu_to_read = find_cpu_to_read(event);
+
                 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) {

Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[David Carrillo-Cisneros]

Hi Nilay,

>>  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) {
>>
>
> 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.

[PATCH v3 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[David Carrillo-Cisneros]

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

Changes in v3:
  - Fix "BUG: using smp_processor_id() in preemptible" reported by
  Xiaolong Ye.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 include/linux/perf_event.h |  3 +++
 kernel/events/core.c       | 44 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index fa5617f..c8bb1b3 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -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)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 34049cc..2c22b30 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3333,6 +3333,26 @@ struct perf_read_data {
 	int ret;
 };
 
+static int find_cpu_to_read(struct perf_event *event, int local_cpu)
+{
+	bool active = event->state == PERF_EVENT_STATE_ACTIVE;
+	int event_cpu = event->oncpu;
+	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,22 +3474,26 @@ 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
-	 * value in the event structure:
-	 */
-	if (event->state == PERF_EVENT_STATE_ACTIVE) {
+	local_cpu = get_cpu();
+	cpu_to_read = find_cpu_to_read(event, local_cpu);
+
+	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) {
+		put_cpu();
+
+		return ret ? : data.ret;
+	}
+	put_cpu();
+
+	if (event->state == PERF_EVENT_STATE_INACTIVE) {
 		struct perf_event_context *ctx = event->ctx;
 		unsigned long flags;
 
@@ -3490,7 +3514,7 @@ static int perf_event_read(struct perf_event *event, bool group)
 		raw_spin_unlock_irqrestore(&ctx->lock, flags);
 	}
 
-	return ret;
+	return 0;
 }
 
 /*
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[Nilay Vaish]

On 7 August 2016 at 15:10, David Carrillo-Cisneros <davidcc@google.com> wrote:
> Hi Nilay,
>
>>>  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) {
>>>
>>
>> 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.


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

Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[David Carrillo-Cisneros]

Hi Nilay,

Sounds good, I will post an updated version.

Thanks,
David

On Mon, Aug 8, 2016 at 9:12 AM, Nilay Vaish <nilayvaish@gmail.com> wrote:
> On 7 August 2016 at 15:10, David Carrillo-Cisneros <davidcc@google.com> wrote:
>> Hi Nilay,
>>
>>>>  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) {
>>>>
>>>
>>> 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.
>
>
> 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

[PATCH 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[David Carrillo-Cisneros]

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 <davidcc@google.com>
---
 include/linux/perf_event.h |  3 +++
 kernel/events/core.c       | 26 ++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index fa5617f..c8bb1b3 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -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)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 34049cc..38ec68d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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

Re: [PATCH v2 1/4] perf/core: check return value of perf_event_read IPI

[Ingo Molnar]

* David Carrillo-Cisneros <davidcc@google.com> wrote:

> 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 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9345028..96523fd 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 = data.ret;
> +		ret = smp_call_function_single(event->oncpu,
> +					       __perf_event_read, &data, 1);
> +		ret = ret ? : data.ret;
>  	} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
>  		struct perf_event_context *ctx = event->ctx;
>  		unsigned long flags;

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

Thanks,

        Ingo

Re: [PATCH 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[Nilay Vaish]

On 9 August 2016 at 17:28, David Carrillo-Cisneros <davidcc@google.com> wrote:
> 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 <davidcc@google.com>
> ---
>  include/linux/perf_event.h |  3 +++
>  kernel/events/core.c       | 26 ++++++++++++++++++++++++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index fa5617f..c8bb1b3 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -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)
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 34049cc..38ec68d 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -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
>

David, thanks for making the changes.  This patch looks good to me.

--
Nilay

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

[Nilay Vaish]

On 6 August 2016 at 22:12, David Carrillo-Cisneros <davidcc@google.com> wrote:
> 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
>

Reviewed-by: Nilay Vaish <nilayvaish@gmail.com>

Re: [PATCH v2 1/4] 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.
> >
> Please also describe the symptoms of the bug - under what circumstances may the
> smp-call fail and how does the user notice?
>
> Thanks,
>
>         Ingo
generic_exec_single only fails if cpu is invalid or not online, which
currently is not supposed to happen. Since perf_event_read's error is
not checked, I guess a WARN_ON_ONCE is needed. I'll post a rebased
version with this change.

Thanks,
David