Re: [RFC] Sharing PMU counters across compatible events

[Jiri Olsa <jolsa@redhat.com>]

On Fri, Dec 01, 2017 at 06:19:50AM -0800, Tejun Heo wrote:
> Hello,
> 
> There are cases where a single PMU event, let's say instructions, is
> interesting for different subsets of the system.  For example, it
> could be interesting to monitor instructions system-wide, at cgroup
> level and per each thread.
> 
> This could easily be me not knowing better but I can't think of a way
> to do the above without consuming multiple PMU counters to monitor
> instructions, which quickly gets out of hand with the product of
> multiple domains and counters.  Getting broken down numbers and adding
> up doesn't work at cgroup (the numbers aren't mutually exclusive) or
> thread level.
> 
> If there's a way to achieve this using the existing features, I'd be
> glad to learn about it.  If not, the patch at the bottom is a crude
> half-working proof-of-concept to share PMU counters across compatible
> events.
> 
> In a nutshell, while adding events, it looks whether there already is
> a compatible event.  If so, instead of enabling the new event, it gets
> linked to the already enabled one (the master event) and count of the
> dup event is updated by adding the delta of the master event.
> 
> That patch is just a proof of concept.  It's missing counter
> propagation somewhere and the dup counts end up somewhat lower than
> they should be.  The master selection and switching are half-broken.
> Event compatibility testing is barely implemented, so on and so forth.
> However, it does allow gathering events for different targets without
> consuming multiple PMU counters.
> 
> What do you think?  Would this be something worth pursuing?

I see this rather on the hw level, since it concerns HW counters

I think we could detect same (alias) events at the time counters
are added/removed on/from the cpu and share their HW part like
counter idx, regs and such (struct hw_perf_event_cpu in my changes)

this way it'd be completely transparent for generic code

I made some untested (just compile) changes and attaching the
top patch, that has the main logic, please check all changes
in here:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/alias_2

there's big change due to changing the perf_event::hw stuff
but the rest is quite simple (attached).. not completely sure
yet if I'm missing some functionality which would break due
to this change

thoughts?

thanks,
jirka


---
 arch/x86/events/core.c     | 58 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/perf_event.h |  4 ++++
 kernel/events/core.c       |  1 +
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 2bb5d1b2a622..737857f07881 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1189,6 +1189,54 @@ void x86_pmu_enable_event(struct perf_event *event)
 				       ARCH_PERFMON_EVENTSEL_ENABLE);
 }
 
+static bool is_alias(struct perf_event *alias, struct perf_event *event)
+{
+	if (is_sampling_event(alias))
+		return false;
+
+	return memcmp(&alias->attr, &event->attr, sizeof(alias->attr));
+}
+
+static int alias_add(struct cpu_hw_events *cpuc, struct perf_event *event)
+{
+	struct perf_event *master;
+	int n;
+
+	for (n = 0; n < cpuc->n_events; n++) {
+		if (is_alias(event, cpuc->event_list[n]))
+			break;
+	}
+
+	if (n == cpuc->n_events)
+		return 0;
+
+	master = cpuc->event_list[n];
+	event->hw.cpu = master->hw.cpu;
+	list_add_tail(&event->hw.alias, &master->hw.master);
+	return 1;
+}
+
+static int alias_del(struct cpu_hw_events *cpuc, struct perf_event *event)
+{
+	if (event->hw.cpu != &event->hw.cpu_local)
+		return 0;
+
+	event->hw.cpu = &event->hw.cpu_local;
+	list_del(&event->hw.alias);
+	return 1;
+}
+
+static void alias_update(struct perf_event *event)
+{
+	struct perf_event *alias;
+
+	list_for_each_entry(alias, &event->hw.master, hw.alias) {
+		alias->count          = event->count;
+		alias->hw.prev_count  = event->hw.prev_count;
+		alias->hw.period_left = event->hw.period_left;
+	}
+}
+
 /*
  * Add a single event to the PMU.
  *
@@ -1200,7 +1248,10 @@ static int x86_pmu_add(struct perf_event *event, int flags)
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct hw_perf_event *hwc;
 	int assign[X86_PMC_IDX_MAX];
-	int n, n0, ret;
+	int n, n0, ret = 0;
+
+	if (alias_add(cpuc, event))
+		goto out;
 
 	hwc = &event->hw;
 
@@ -1383,11 +1434,16 @@ static void x86_pmu_del(struct perf_event *event, int flags)
 	if (cpuc->txn_flags & PERF_PMU_TXN_ADD)
 		goto do_del;
 
+	if (alias_del(cpuc, event))
+		goto do_del;
+
 	/*
 	 * Not a TXN, therefore cleanup properly.
 	 */
 	x86_pmu_stop(event, PERF_EF_UPDATE);
 
+	alias_update(event);
+
 	for (i = 0; i < cpuc->n_events; i++) {
 		if (event == cpuc->event_list[i])
 			break;
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index b8bc5ddfbffd..6a09914a3555 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -136,6 +136,10 @@ struct hw_perf_event {
 		struct { /* hardware */
 			struct hw_perf_event_cpu *cpu;
 			struct hw_perf_event_cpu  cpu_local;
+			union {
+				struct list_head master;
+				struct list_head alias;
+			};
 		};
 		struct { /* software */
 			struct hrtimer	hrtimer;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 5879c67d90e4..9cf36b2b533f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9513,6 +9513,7 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	INIT_LIST_HEAD(&event->active_event_entry);
 	INIT_LIST_HEAD(&event->dup_list);
 	INIT_LIST_HEAD(&event->addr_filters.list);
+	INIT_LIST_HEAD(&event->hw.master);
 	INIT_HLIST_NODE(&event->hlist_entry);
 
 
-- 
2.13.6