Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1396198 > unrolled thread

[PATCH 2/3] perf/x86/mbm: Fix mbm counting for RMID reuse

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/3] perf/x86/mbm: Fix mbm counting for RMID reuse Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2016-05-07 01:50 +0200
    Re: [PATCH 2/3] perf/x86/mbm: Fix mbm counting for RMID reuse Peter Zijlstra <peterz@infradead.org> - 2016-05-10 14:20 +0200
      RE: [PATCH 2/3] perf/x86/mbm: Fix mbm counting for RMID reuse "Luck, Tony" <tony.luck@intel.com> - 2016-05-10 18:40 +0200
        Re: [PATCH 2/3] perf/x86/mbm: Fix mbm counting for RMID reuse Peter Zijlstra <peterz@infradead.org> - 2016-05-11 09:30 +0200

#1396198 — [PATCH 2/3] perf/x86/mbm: Fix mbm counting for RMID reuse

This patch tries to fix the issue where multiple perf instances try to
monitor the same PID.
MBM cannot count directly in the usual perf way of continuously adding
 the diff of current h/w counter and the prev count to the event->count
because of some h/w dependencies: (1) the mbm h/w counters overflow. (2)
There are limited h/w RMIDs and hence we recycle the RMIDs due to
which an event may count from different RMIDs. (3) Also we may not want to
count at every sched_in and sched_out because the MSR reads involve
quite a bit of overhead.

However we try to do something similar to usual perf way in this patch
and mainly handle (1) and (3).
update_sample takes care of the overflow in the hardware counters and
provides abstraction by returning total bytes counted as if there was no
overflow. We use this abstraction to count as below:

init:
  event->prev_count = update_sample(rmid) //returns current total_bytes

count: // MBM right now uses count instead of read
  cur_count = update_sample(rmid)
  event->count += cur_count - event->prev_count
  event->prev_count = cur_count

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/events/intel/cqm.c | 66 ++++++++++++++++++++++++++++++++++++++++++---
 include/linux/perf_event.h  |  1 +
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 5f2104a..a98d841 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -479,6 +479,14 @@ static void cqm_mask_call(struct rmid_read *rr)
 		on_each_cpu_mask(&cqm_cpumask, __intel_cqm_event_count, rr, 1);
 }
 
+static void update_mbm_count(u64 val, struct perf_event *event)
+{
+	u64 diff = val - local64_read(&event->hw.cqm_prev_count);
+
+	local64_add(diff, &event->count);
+	local64_set(&event->hw.cqm_prev_count, val);
+}
+
 /*
  * Exchange the RMID of a group of events.
  */
@@ -1005,6 +1013,52 @@ static void init_mbm_sample(u32 rmid, u32 evt_type)
 	on_each_cpu_mask(&cqm_cpumask, __intel_mbm_event_init, &rr, 1);
 }
 
+static inline bool first_event_ingroup(struct perf_event *group,
+				    struct perf_event *event)
+{
+	struct list_head *head = &group->hw.cqm_group_entry;
+	u32 evt_type = event->attr.config;
+
+	if (evt_type == group->attr.config)
+		return false;
+	list_for_each_entry(event, head, hw.cqm_group_entry) {
+		if (evt_type == event->attr.config)
+			return false;
+	}
+
+	return true;
+}
+
+/*
+ * mbm_setup_event - Does mbm specific count initialization
+ * when multiple events share RMID.
+ *
+ * If this is the first mbm event then the event prev_count is 0 bytes,
+ * else the current bytes of the RMID is the prev_count.
+*/
+static inline void mbm_setup_event(u32 rmid, struct perf_event *group,
+					  struct perf_event *event)
+{
+	u32 evt_type = event->attr.config;
+	struct rmid_read rr;
+	u64 val;
+
+	if (first_event_ingroup(group, event)) {
+		init_mbm_sample(rmid, evt_type);
+	} else {
+		rr = __init_rr(rmid, evt_type, 0);
+		cqm_mask_call(&rr);
+		val = atomic64_read(&rr.value);
+		local64_set(&event->hw.cqm_prev_count, val);
+	}
+}
+
+static inline void mbm_setup_event_init(struct perf_event *event)
+{
+	event->hw.is_group_event = false;
+	local64_set(&event->hw.cqm_prev_count, 0UL);
+}
+
 /*
  * Find a group and setup RMID.
  *
@@ -1017,7 +1071,7 @@ static void intel_cqm_setup_event(struct perf_event *event,
 	bool conflict = false;
 	u32 rmid;
 
-	event->hw.is_group_event = false;
+	mbm_setup_event_init(event);
 	list_for_each_entry(iter, &cache_groups, hw.cqm_groups_entry) {
 		rmid = iter->hw.cqm_rmid;
 
@@ -1026,7 +1080,7 @@ static void intel_cqm_setup_event(struct perf_event *event,
 			event->hw.cqm_rmid = rmid;
 			*group = iter;
 			if (is_mbm_event(event->attr.config) && __rmid_valid(rmid))
-				init_mbm_sample(rmid, event->attr.config);
+				mbm_setup_event(rmid, iter, event);
 			return;
 		}
 
@@ -1244,8 +1298,12 @@ static u64 intel_cqm_event_count(struct perf_event *event)
 	cqm_mask_call(&rr);
 
 	raw_spin_lock_irqsave(&cache_lock, flags);
-	if (event->hw.cqm_rmid == rr.rmid)
-		local64_set(&event->count, atomic64_read(&rr.value));
+	if (event->hw.cqm_rmid == rr.rmid) {
+		if (is_mbm_event(event->attr.config))
+			update_mbm_count(atomic64_read(&rr.value), event);
+		else
+			local64_set(&event->count, atomic64_read(&rr.value));
+	}
 	raw_spin_unlock_irqrestore(&cache_lock, flags);
 out:
 	return __perf_event_count(event);
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index f291275..9298a89 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -122,6 +122,7 @@ struct hw_perf_event {
 			int			cqm_state;
 			u32			cqm_rmid;
 			int			is_group_event;
+			local64_t		cqm_prev_count;
 			struct list_head	cqm_events_entry;
 			struct list_head	cqm_groups_entry;
 			struct list_head	cqm_group_entry;
-- 
1.9.1

[toc] | [next] | [standalone]

#1398041

On Fri, May 06, 2016 at 04:44:14PM -0700, Vikas Shivappa wrote:
> This patch tries to fix the issue where multiple perf instances try to
> monitor the same PID.

> MBM cannot count directly in the usual perf way of continuously adding
> the diff of current h/w counter and the prev count to the event->count
> because of some h/w dependencies:

And yet the patch appears to do exactly that; *confused*.

>  (1) the mbm h/w counters overflow.

As do most other counters.. so your point is? You also have the software
timer < overflow period..

>  (2) There are limited h/w RMIDs and hence we recycle the RMIDs due to
>      which an event may count from different RMIDs.

This fails to explain why this is a problem.

>  (3) Also we may not want to count at every sched_in and sched_out
>      because the MSR reads involve quite a bit of overhead.

Every single other PMU driver just does this; why are you special?

You list 3 issues of why you think you cannot do the regular thing, but
completely fail to explain how these issues are a problem.

> However we try to do something similar to usual perf way in this patch
> and mainly handle (1) and (3).

> update_sample takes care of the overflow in the hardware counters and
> provides abstraction by returning total bytes counted as if there was no
> overflow. We use this abstraction to count as below:
> 
> init:
>   event->prev_count = update_sample(rmid) //returns current total_bytes
> 
> count: // MBM right now uses count instead of read
>   cur_count = update_sample(rmid)
>   event->count += cur_count - event->prev_count
>   event->prev_count = cur_count

So where does cqm_prev_count come from and why do you need it? What's
wrong with event->hw.prev_count ?

In fact, I cannot seem to find any event->hw.prev_count usage in this or
the next patch, so can we simply use that and not add pointless new
members?

[toc] | [prev] | [next] | [standalone]

#1398305

>>  (3) Also we may not want to count at every sched_in and sched_out
>>      because the MSR reads involve quite a bit of overhead.
>
> Every single other PMU driver just does this; why are you special?

They just have to read a register.  We have to write the IA32_EM_EVT_SEL MSR
and then read from the IA32_QM_CTR MSR ... if we are tracking both local
and total bandwidth, we have to do repeat and wrmr/rdmsr again to get the
other counter.  That seems like it will noticeably affect the system if we do it
on every sched_in and sched_out.

But the more we make this complicated, the more I think that we should not
go through the pain of stealing/recycling RMIDs and just limit the number of
things that can be simultaneously monitored.  If someone tries to monitor one
more thing when all the RMIDs are in use, we should just error out with
-ERUNOUTOFRMIDSTRYAGAINLATER (maybe -EAGAIN???)

-Tony

[toc] | [prev] | [next] | [standalone]

#1398731

On Tue, May 10, 2016 at 04:39:39PM +0000, Luck, Tony wrote:
> >>  (3) Also we may not want to count at every sched_in and sched_out
> >>      because the MSR reads involve quite a bit of overhead.
> >
> > Every single other PMU driver just does this; why are you special?
> 
> They just have to read a register.  We have to write the IA32_EM_EVT_SEL MSR
> and then read from the IA32_QM_CTR MSR ... if we are tracking both local
> and total bandwidth, we have to do repeat and wrmr/rdmsr again to get the
> other counter.  That seems like it will noticeably affect the system if we do it
> on every sched_in and sched_out.

Right; but Vikas didn't say that did he ;-), he just mentioned msr-read.

Also; I don't think you actually have to do it on every sched event,
only when the event<->rmid association changes. As long as the
event<->rmid association doesn't change, you can forgo updates.

> But the more we make this complicated, the more I think that we should not
> go through the pain of stealing/recycling RMIDs and just limit the number of
> things that can be simultaneously monitored.  If someone tries to monitor one
> more thing when all the RMIDs are in use, we should just error out with
> -ERUNOUTOFRMIDSTRYAGAINLATER (maybe -EAGAIN???)

Possibly; but I would like to minimize churn at this point to let the
Google guys get their patches in shape. They seem to have definite ideas
about that as well :-)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web