Urgent fixes for Intel CQM/MBM counting

During RMID recycling, when an event loses the RMID we saved the counter for group leader but it was not being saved for all the events in an event group. This would lead to a situation where if 2 perf instances are counting the same PID one of them would not see the updated count which other perf instance is seeing. This patch tries to fix the issue by saving the count for all the events in the same event group.

arch/x86/events/intel/cqm.c +1 -0

@@ -486,14 +495,21 @@ static u32 intel_cqm_xchg_rmid(struct perf_event *group, u32 rmid)
 	 * If our RMID is being deallocated, perform a read now.
 	 */
 	if (__rmid_valid(old_rmid) && !__rmid_valid(rmid)) {
 
+		rr = __init_rr(old_rmid, group->attr.config, 0);
 		cqm_mask_call(&rr);

arch/x86/events/intel/cqm.c +2 -0

+			}
+		}
 	}

Quoted 13 lines · Vikas Shivappa

During RMID recycling, when an event loses the RMID we saved the counter for group leader but it was not being saved for all the events in an event group. This would lead to a situation where if 2 perf instances are counting the same PID one of them would not see the updated count which other perf instance is seeing. This patch tries to fix the issue by saving the count for all the events in the same event group.

arch/x86/events/intel/cqm.c +0 -0

@@ -486,14 +495,21 @@ static u32 intel_cqm_xchg_rmid(struct perf_event *group, u32 rmid)
 	 * If our RMID is being deallocated, perform a read now.
 	 */
 	if (__rmid_valid(old_rmid) && !__rmid_valid(rmid)) {
 

arch/x86/events/intel/cqm.c static u32 intel_cqm_xchg_rmid(struct perf_event *group, u32 rmid) · line 510

Quoted 12 lines

arch/x86/events/intel/cqm.c +10 -0

+		rr = __init_rr(old_rmid, group->attr.config, 0);
 		cqm_mask_call(&rr);
 		local64_set(&group->count, atomic64_read(&rr.value));
+		list_for_each_entry(event, head, hw.cqm_group_entry) {
+			if (event->hw.is_group_event) {
+
+				evttype = event->attr.config;
+				rr = __init_rr(old_rmid, evttype, 0);
+
+				cqm_mask_call(&rr);
+					local64_set(&event->count,
+						    atomic64_read(&rr.value));

Randomly indent much?

Quoted 4 lines

arch/x86/events/intel/cqm.c +2 -0

+			}
+		}
 	}

arch/x86/events/intel/cqm.c +3 -2

@@ -1244,8 +1270,16 @@ 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)) {
+			tmpval = atomic64_read(&rr.value) +

arch/x86/events/intel/cqm.c static void cqm_mask_call(struct rmid_read *rr) · line 483

Quoted 2 lines · Vikas Shivappa

arch/x86/events/intel/cqm.c +2 -0

+static inline void mbm_set_rccount(
+			  struct perf_event *event, struct rmid_read *rr)

That's horrible style, the 'normal' style is something like:

static inline
void mbm_set_rccount(struct perf_event *event, struct rmid_read *rr) {
}

Quoted 10 lines

arch/x86/events/intel/cqm.c +3 -2

@@ -1244,8 +1270,16 @@ 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)) {
+			tmpval = atomic64_read(&rr.value) +

arch/x86/events/intel/cqm.c static u64 intel_cqm_event_count(struct perf_event *event) · line 1285

Quoted 10 lines

arch/x86/events/intel/cqm.c +7 -0

+				local64_read(&event->hw.rc_count);
+
+			local64_set(&event->count, tmpval);
+		} else {
+			local64_set(&event->count, atomic64_read(&rr.value));
+		}
+	}
 	raw_spin_unlock_irqrestore(&cache_lock, flags);
 out:
 	return __perf_event_count(event);

This is a 'creative' solution; why don't you do the normal thing, which is:

start:

	prev_count = read_hw_counter();

read:

	do {
		prev = prev_count;
		cur_val = read_hw_counter();
		delta = cur_val - prev;
	} while (local_cmpxchg(&prev_count, prev, cur_val) != prev);
	count += delta;

Quoted 6 lines

This is a 'creative' solution; why don't you do the normal thing, which is:

start:

	prev_count = read_hw_counter();

Quoted 7 lines

read:

	do {
		prev = prev_count;
		cur_val = read_hw_counter();
		delta = cur_val - prev;
	} while (local_cmpxchg(&prev_count, prev, cur_val) != prev);
	count += delta;

I may need to update the comment.

rc_count stores the total bytes for RMIDs that were used for this event except for the count of current RMID.

Say an event used RMID(1) .. RMID(k) from init to read and it had RMID(k) when read was called, the rc_count stores the values read from RMID1 .. RMID(k-1).

For MBM the patch is trying to do:
count
= total_bytes of RMID(1) + ... +total_bytes of RMID(k-1) + total_bytes of RMID(k))
= rc_count + total_bytes of RMID(k).

Quoted 15 lines · Vikas Shivappa

Quoted 6 lines

This is a 'creative' solution; why don't you do the normal thing, which is:

start:

	prev_count = read_hw_counter();

Quoted 7 lines

read:

	do {
		prev = prev_count;
		cur_val = read_hw_counter();
		delta = cur_val - prev;
	} while (local_cmpxchg(&prev_count, prev, cur_val) != prev);
	count += delta;

Quoted 4 lines

I may need to update the comment.

rc_count stores the total bytes for RMIDs that were used for this event except for the count of current RMID.

Yeah, I got that, eventually.

Quoted 4 lines

Say an event used RMID(1) .. RMID(k) from init to read and it had RMID(k) when read was called, the rc_count stores the values read from RMID1 .. RMID(k-1).

Quoted 5 lines

For MBM the patch is trying to do:
count
= total_bytes of RMID(1) + ... +total_bytes of RMID(k-1) + total_bytes of RMID(k))
= rc_count + total_bytes of RMID(k).

How is the regular counting scheme as outlined above not dealing with this properly?

start:

      prev_count = read_hw_counter();

read:

      do {
              prev = prev_count;
              cur_val = read_hw_counter();
              delta = cur_val - prev;
      } while (local_cmpxchg(&prev_count, prev, cur_val) != prev);
      count += delta;

Quoted 2 lines

start:

     prev_count = read_hw_counter();

I am assuming this means we keep the prev_count when event is initialized. This is done in the mbm_init which calls update_sample with first parameter set to true..

Quoted 7 lines

read:

     do {
             prev = prev_count;
             cur_val = read_hw_counter();
             delta = cur_val - prev;
     } while (local_cmpxchg(&prev_count, prev, cur_val) != prev);
     count += delta;

the update_sample does the work to compute the delta and add the delta to total_bytes.. it has all the code except for the while loop.

Quoted 6 lines · Vikas Shivappa

Quoted 2 lines

start:

     prev_count = read_hw_counter();

I am assuming this means we keep the prev_count when event is initialized. This is done in the mbm_init which calls update_sample with first parameter set to true..

No, when pmu::start() is called.

Quoted 7 lines

Quoted 7 lines

read:

     do {
             prev = prev_count;
             cur_val = read_hw_counter();
             delta = cur_val - prev;
     } while (local_cmpxchg(&prev_count, prev, cur_val) != prev);
     count += delta;

And this you do on pmu::{stop,read}()

When multiple instances of perf reuse RMID, then we need to start counting for each instance rather than reporting the current RMID count. This patch adds a st_count(start count) per event to track the same.

Quoted 3 lines · Vikas Shivappa

When multiple instances of perf reuse RMID, then we need to start counting for each instance rather than reporting the current RMID count. This patch adds a st_count(start count) per event to track the same.

what?

Quoted 7 lines · Peter Zijlstra

Quoted 3 lines · Vikas Shivappa

When multiple instances of perf reuse RMID, then we need to start counting for each instance rather than reporting the current RMID count. This patch adds a st_count(start count) per event to track the same.

what?

Will fix the comit log :

When multiple instances of perf reuse RMID for the same PID, then we need to start counting from zero for each new event, rather than reporting the current RMID. This patch adds a st_count(start count) per event to track the same.

For ex:
1.RMID1's total_bytes is 100MB for event1(PID1)
2.another perf instance starts measuring the same PID1 with event2. We reuse RMID1 as the PID1 is already counted.
3.event2 stores st_count as 100MB.
4.After some time, when user wants to count event2 and say RMID1's current total_bytes 110MB, we report 110MB - 100MB = 10MB

Quoted 19 lines · Vikas Shivappa

Quoted 7 lines · Peter Zijlstra

Quoted 3 lines · Vikas Shivappa

When multiple instances of perf reuse RMID, then we need to start counting for each instance rather than reporting the current RMID count. This patch adds a st_count(start count) per event to track the same.

what?

Will fix the comit log :

When multiple instances of perf reuse RMID for the same PID, then we need to start counting from zero for each new event, rather than reporting the current RMID. This patch adds a st_count(start count) per event to track the same.

Quoted 7 lines

For ex:
1.RMID1's total_bytes is 100MB for event1(PID1)
2.another perf instance starts measuring the same PID1 with event2. We reuse RMID1 as the PID1 is already counted.
3.event2 stores st_count as 100MB.
4.After some time, when user wants to count event2 and say RMID1's current total_bytes 110MB, we report 110MB - 100MB = 10MB

This is naturally handled by the scheme I outlined in the other patch.

'per event' rc_count = read_sample(event->rmid) - per event start count;

The cgroup support for cqm is broken. Instead of mapping RMID to a cgroup currently its mapped to the task and then hence when task moves cgroup we get incorrect count.

Also the conflict handling code which is meant to handle the case of co-existing cgroup and task events, is broken. It reports very confusing numbers of intermittent zero and some occupancy when perf is run with cgroup and task events.

Hence removing support for the parts which are broken rather than pretending to support it and giving incorrect data.

Quoted 2 lines

Hence removing support for the parts which are broken rather than pretending to support it and giving incorrect data.

Uh what, how about attempt to fix it?

Quoted 4 lines

Quoted 2 lines

Hence removing support for the parts which are broken rather than pretending to support it and giving incorrect data.

Uh what, how about attempt to fix it?

No hope to do that by 4.6 release ... so I suggested to Vikas that it would be better to disable the feature now so users wouldn't be confused by the random numbers that they'll see if they try to do this.