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

Groups > linux.kernel > #1361655 > unrolled thread

[tip:perf/urgent] perf/x86/mbm: Implement RMID recycling

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

  [tip:perf/urgent] perf/x86/mbm: Implement RMID recycling tip-bot for Vikas Shivappa <tipbot@zytor.com> - 2016-03-21 11:00 +0100
    Re: [tip:perf/urgent] perf/x86/mbm: Implement RMID recycling Matt Fleming <matt@codeblueprint.co.uk> - 2016-03-21 16:10 +0100
      Re: [tip:perf/urgent] perf/x86/mbm: Implement RMID recycling Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2016-03-21 20:50 +0100
        Re: [tip:perf/urgent] perf/x86/mbm: Implement RMID recycling Matt Fleming <matt@codeblueprint.co.uk> - 2016-03-23 22:00 +0100

#1361655 — [tip:perf/urgent] perf/x86/mbm: Implement RMID recycling

Commit-ID:  2d4de8376ff1d94a5070cfa9092c59bfdc4e693e
Gitweb:     http://git.kernel.org/tip/2d4de8376ff1d94a5070cfa9092c59bfdc4e693e
Author:     Vikas Shivappa <vikas.shivappa@linux.intel.com>
AuthorDate: Thu, 10 Mar 2016 15:32:11 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 21 Mar 2016 09:08:20 +0100

perf/x86/mbm: Implement RMID recycling

RMID could be allocated or deallocated as part of RMID recycling.

When an RMID is allocated for MBM event, the MBM counter needs to be
initialized because next time we read the counter we need the previous
value to account for total bytes that went to the memory controller.

Similarly, when RMID is deallocated we need to update the ->count
variable.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: fenghua.yu@intel.com
Cc: h.peter.anvin@intel.com
Cc: ravi.v.shankar@intel.com
Cc: vikas.shivappa@intel.com
Link: http://lkml.kernel.org/r/1457652732-4499-6-git-send-email-vikas.shivappa@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/events/intel/cqm.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 610bd8a..a98f472 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -450,6 +450,7 @@ struct rmid_read {
 
 static void __intel_cqm_event_count(void *info);
 static void init_mbm_sample(u32 rmid, u32 evt_type);
+static void __intel_mbm_event_count(void *info);
 
 static bool is_mbm_event(int e)
 {
@@ -476,8 +477,14 @@ static u32 intel_cqm_xchg_rmid(struct perf_event *group, u32 rmid)
 			.rmid = old_rmid,
 		};
 
-		on_each_cpu_mask(&cqm_cpumask, __intel_cqm_event_count,
-				 &rr, 1);
+		if (is_mbm_event(group->attr.config)) {
+			rr.evt_type = group->attr.config;
+			on_each_cpu_mask(&cqm_cpumask, __intel_mbm_event_count,
+					 &rr, 1);
+		} else {
+			on_each_cpu_mask(&cqm_cpumask, __intel_cqm_event_count,
+					 &rr, 1);
+		}
 		local64_set(&group->count, atomic64_read(&rr.value));
 	}
 
@@ -489,6 +496,22 @@ static u32 intel_cqm_xchg_rmid(struct perf_event *group, u32 rmid)
 
 	raw_spin_unlock_irq(&cache_lock);
 
+	/*
+	 * If the allocation is for mbm, init the mbm stats.
+	 * Need to check if each event in the group is mbm event
+	 * because there could be multiple type of events in the same group.
+	 */
+	if (__rmid_valid(rmid)) {
+		event = group;
+		if (is_mbm_event(event->attr.config))
+			init_mbm_sample(rmid, event->attr.config);
+
+		list_for_each_entry(event, head, hw.cqm_group_entry) {
+			if (is_mbm_event(event->attr.config))
+				init_mbm_sample(rmid, event->attr.config);
+		}
+	}
+
 	return old_rmid;
 }
 
@@ -978,7 +1001,7 @@ static void intel_cqm_setup_event(struct perf_event *event,
 			/* All tasks in a group share an RMID */
 			event->hw.cqm_rmid = rmid;
 			*group = iter;
-			if (is_mbm_event(event->attr.config))
+			if (is_mbm_event(event->attr.config) && __rmid_valid(rmid))
 				init_mbm_sample(rmid, event->attr.config);
 			return;
 		}
@@ -996,7 +1019,7 @@ static void intel_cqm_setup_event(struct perf_event *event,
 	else
 		rmid = __get_rmid();
 
-	if (is_mbm_event(event->attr.config))
+	if (is_mbm_event(event->attr.config) && __rmid_valid(rmid))
 		init_mbm_sample(rmid, event->attr.config);
 
 	event->hw.cqm_rmid = rmid;

[toc] | [next] | [standalone]

#1361919

On Mon, 21 Mar, at 02:53:04AM, tip-bot for Vikas Shivappa wrote:
> @@ -489,6 +496,22 @@ static u32 intel_cqm_xchg_rmid(struct perf_event *group, u32 rmid)
>  
>  	raw_spin_unlock_irq(&cache_lock);
>  
> +	/*
> +	 * If the allocation is for mbm, init the mbm stats.
> +	 * Need to check if each event in the group is mbm event
> +	 * because there could be multiple type of events in the same group.
> +	 */
> +	if (__rmid_valid(rmid)) {
> +		event = group;
> +		if (is_mbm_event(event->attr.config))
> +			init_mbm_sample(rmid, event->attr.config);
> +
> +		list_for_each_entry(event, head, hw.cqm_group_entry) {
> +			if (is_mbm_event(event->attr.config))
> +				init_mbm_sample(rmid, event->attr.config);
> +		}
> +	}
> +
>  	return old_rmid;
>  }
>  

You're calling init_mbm_sample() without holding cache_lock. Won't
this potentially trash the existing value in MSR_IA32_QM_EVTSEL, if
say, we're reading the counter at the same time as the recycling
worker is running?

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

#1362119

On Mon, 21 Mar 2016, Matt Fleming wrote:

> On Mon, 21 Mar, at 02:53:04AM, tip-bot for Vikas Shivappa wrote:
>> @@ -489,6 +496,22 @@ static u32 intel_cqm_xchg_rmid(struct perf_event *group, u32 rmid)
>>
>>  	raw_spin_unlock_irq(&cache_lock);
>>
>> +	/*
>> +	 * If the allocation is for mbm, init the mbm stats.
>> +	 * Need to check if each event in the group is mbm event
>> +	 * because there could be multiple type of events in the same group.
>> +	 */
>> +	if (__rmid_valid(rmid)) {
>> +		event = group;
>> +		if (is_mbm_event(event->attr.config))
>> +			init_mbm_sample(rmid, event->attr.config);
>> +
>> +		list_for_each_entry(event, head, hw.cqm_group_entry) {
>> +			if (is_mbm_event(event->attr.config))
>> +				init_mbm_sample(rmid, event->attr.config);
>> +		}
>> +	}
>> +
>>  	return old_rmid;
>>  }
>>
>
> You're calling init_mbm_sample() without holding cache_lock. Won't
> this potentially trash the existing value in MSR_IA32_QM_EVTSEL, if
> say, we're reading the counter at the same time as the recycling
> worker is running?

The init_mbm_sample calls the update_sample to read the MSR in IPI .. Since the 
count is also in IPI , they should not trash each other ?

Basically all the MSR read/writes are in high irql , except for the mbm overflow 
timer and read calls which holds an irqsave spinlock.

Thanks,
Vikas


>

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

#1363688

On Mon, 21 Mar, at 11:27:55AM, Vikas Shivappa wrote:
> 
> The init_mbm_sample calls the update_sample to read the MSR in IPI .. Since
> the count is also in IPI , they should not trash each other ?
> 
> Basically all the MSR read/writes are in high irql , except for the mbm
> overflow timer and read calls which holds an irqsave spinlock.

Good point! This should be fine.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web