x86, perf, cqm: Cleanups and preparation for RDT/CAT
Subsystems (2)
cover x86, perf, cqm: Cleanups and preparation for RDT/CAT
While reviewing the RDT/CAT patches I had to look into the perf CQM code. As usual when my review mood reaches the grumpiness level, I start to poke around in the code some more and find stuff which really sucks.
Here is a step by step patch series, which cleans up and clarifies the code and prepares it for reuasage of the PQR cache for the RDT/CAT stuff.
Compile tested only.
Shameless hint @Intel: I have no access to a box with CQM/RDT :)
Thanks,
tglx
PATCH 1/6 x86, perf, cqm: Document PQR MSR abuse
The cqm code acts like it owns the PQR MSR completely. That's not true because only the lower 10 bits are used for CQM. The upper 32bits are used for CLass Of Service ID (closid). Document the abuse. Will be fixed in a later patch.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +13 -2
@@ -978,7 +978,12 @@ static void intel_cqm_event_start(structWARN_ON_ONCE (state->rmid); state->rmid = rmid; -wrmsrl (MSR_IA32_PQR_ASSOC , state->rmid); +/* +* This is actually wrong, as the upper 32 bit MSR contain the +* closid which is used for configuring the Cache Allocation +* Technology component. +*/ +wrmsr (MSR_IA32_PQR_ASSOC , rmid,0 );raw_spin_unlock_irqrestore (&state->lock, flags); }
@@ -998,7 +1003,13 @@ static void intel_cqm_event_stop(structif (!--state->cnt) { state->rmid =0 ; -wrmsrl (MSR_IA32_PQR_ASSOC ,0 ); +/* +* This is actually wrong, as the upper 32 bit of the +* MSR contain the closid which is used for +* configuring the Cache Allocation Technology +* component. +*/ +wrmsr (MSR_IA32_PQR_ASSOC ,0 ,0 ); }else {WARN_ON_ONCE (!state->rmid); }
Re: [patch 1/6] x86, perf, cqm: Document PQR MSR abuse
Quoted 9 lines · Thomas Gleixner
The cqm code acts like it owns the PQR MSR completely. That's not true because only the lower 10 bits are used for CQM. The upper 32bits are used for CLass Of Service ID (closid). Document the abuse. Will be fixed in a later patch.
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
tip-bot for Thomas Gleixner
merge bot[tip:perf/core] perf/x86/intel/cqm: Document PQR MSR abuse
In reply to Thomas Gleixnerperf/x86/intel/cqm: Document PQR MSR abuse
Commit-ID: f4d9757ca6f5a2db6919a5b1ab86b8afa16773d0 Gitweb: http://git.kernel.org/tip/f4d9757ca6f5a2db6919a5b1ab86b8afa16773d0 Author: Thomas Gleixner <[email protected]> AuthorDate: Tue, 19 May 2015 00:00:50 +0000 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 27 May 2015 09:17:38 +0200
perf/x86/intel/cqm: Document PQR MSR abuse
The CQM code acts like it owns the PQR MSR completely. That's not true because only the lower 10 bits are used for CQM. The upper 32 bits are used for the 'CLass Of Service ID' (CLOSID). Document the abuse. Will be fixed in a later patch.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +13 -2
@@ -978,7 +978,12 @@ static void intel_cqm_event_start(struct perf_event *event, int mode)WARN_ON_ONCE (state->rmid); state->rmid = rmid; -wrmsrl (MSR_IA32_PQR_ASSOC , state->rmid); +/* +* This is actually wrong, as the upper 32 bit MSR contain the +* closid which is used for configuring the Cache Allocation +* Technology component. +*/ +wrmsr (MSR_IA32_PQR_ASSOC , rmid,0 );raw_spin_unlock_irqrestore (&state->lock, flags); }
@@ -998,7 +1003,13 @@ static void intel_cqm_event_stop(struct perf_event *event, int mode)if (!--state->cnt) { state->rmid =0 ; -wrmsrl (MSR_IA32_PQR_ASSOC ,0 ); +/* +* This is actually wrong, as the upper 32 bit of the +* MSR contain the closid which is used for +* configuring the Cache Allocation Technology +* component. +*/ +wrmsr (MSR_IA32_PQR_ASSOC ,0 ,0 ); }else {WARN_ON_ONCE (!state->rmid); }
PATCH 2/6 x86, perf, cqm: Use proper data type
In reply to Thomas Gleixnerint is really not a proper data type for a MSR. Use u32 to make it clear that we are dealing with a 32bit unsigned hardware value.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 4 ++-- include/linux/perf_event.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +2 -2
@@ -18,7 +18,7 @@ static unsigned int cqm_l3_scale; /* supstruct intel_cqm_state {raw_spinlock_t lock; -int rmid; +u32 rmid;int cnt; };
@@ -962,7 +962,7 @@ out:static void intel_cqm_event_start (struct perf_event *event,int mode) {struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); -unsigned int rmid = event->hw.cqm_rmid; +u32 rmid = event->hw.cqm_rmid;unsigned long flags;if (!(event->hw.cqm_state &PERF_HES_STOPPED ))
include/linux/perf_event.h +1 -1
@@ -124,7 +124,7 @@ struct hw_perf_event { };struct {/* intel_cqm */ int cqm_state; -int cqm_rmid; +u32 cqm_rmid;struct list_head cqm_events_entry;struct list_head cqm_groups_entry;struct list_head cqm_group_entry;
Re: [patch 2/6] x86, perf, cqm: Use proper data type
Quoted 8 lines · Thomas Gleixner
int is really not a proper data type for a MSR. Use u32 to make it clear that we are dealing with a 32bit unsigned hardware value.
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 4 ++-- include/linux/perf_event.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
Yeah, makes sense, but this is missing a bunch of changes to other functions that pass rmids around.
Lemme take a swing at that.
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
Re: [patch 2/6] x86, perf, cqm: Use proper data type
Quoted 4 lines · Matt Fleming
Quoted 3 lines · Thomas Gleixner
int is really not a proper data type for a MSR. Use u32 to make it clear that we are dealing with a 32bit unsigned hardware value.
Quoted 8 lines
Quoted 5 lines
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 4 ++-- include/linux/perf_event.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)Yeah, makes sense, but this is missing a bunch of changes to other functions that pass rmids around.
Right. I cared about the stuff which handles the cached state.
Quoted 1 line
Lemme take a swing at that.
Yes, please.
Thanks,
tglx
tip-bot for Thomas Gleixner
merge bot[tip:perf/core] perf/x86/intel/cqm: Use proper data types
In reply to Thomas Gleixnerperf/x86/intel/cqm: Use proper data types
Commit-ID: b3df4ec4424f27e55d754cfe586195fecca1c4e4 Gitweb: http://git.kernel.org/tip/b3df4ec4424f27e55d754cfe586195fecca1c4e4 Author: Thomas Gleixner <[email protected]> AuthorDate: Tue, 19 May 2015 00:00:51 +0000 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 27 May 2015 09:17:39 +0200
perf/x86/intel/cqm: Use proper data types
'int' is really not a proper data type for an MSR. Use u32 to make it clear that we are dealing with a 32-bit unsigned hardware value.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 4 ++-- include/linux/perf_event.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +2 -2
@@ -18,7 +18,7 @@ static unsigned int cqm_l3_scale; /* supposedly cacheline size */struct intel_cqm_state {raw_spinlock_t lock; -int rmid; +u32 rmid;int cnt; };
@@ -962,7 +962,7 @@ out:static void intel_cqm_event_start (struct perf_event *event,int mode) {struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); -unsigned int rmid = event->hw.cqm_rmid; +u32 rmid = event->hw.cqm_rmid;unsigned long flags;if (!(event->hw.cqm_state &PERF_HES_STOPPED ))
include/linux/perf_event.h +1 -1
@@ -120,7 +120,7 @@ struct hw_perf_event { };struct {/* intel_cqm */ int cqm_state; -int cqm_rmid; +u32 cqm_rmid;struct list_head cqm_events_entry;struct list_head cqm_groups_entry;struct list_head cqm_group_entry;
PATCH 3/6 x86, perf, cqm: Remove pointless spinlock from state cache
In reply to Thomas Gleixnerstruct intel_cqm_state is a strict per cpu cache of the rmid and the usage counter. It can never be modified from a remote cpu.
The 3 functions which modify the content: start, stop and del (del maps to stop) are called from the perf core with interrupts disabled which is enough protection for the per cpu state values.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +6 -11
@@ -17,11 +17,16 @@ static unsigned int cqm_max_rmid = -1;static unsigned int cqm_l3_scale;/* supposedly cacheline size */ struct intel_cqm_state { -raw_spinlock_t lock;u32 rmid;int cnt; }; +/* +* The cached intel_cqm_state is strictly per cpu and can never be +* updated from a remote cpu. Both functions which modify the state +* (intel_cqm_event_start and intel_cqm_event_stop) are called with +* interrupts disabled, which is sufficient for the protection. +*/ static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state);/*
@@ -963,15 +968,12 @@ static void intel_cqm_event_start(struct {struct intel_cqm_state *state =this_cpu_ptr (&cqm_state);u32 rmid = event->hw.cqm_rmid; -unsigned long flags;if (!(event->hw.cqm_state &PERF_HES_STOPPED ))return ; event->hw.cqm_state &= ~PERF_HES_STOPPED ; -raw_spin_lock_irqsave (&state->lock, flags); -if (state->cnt++)WARN_ON_ONCE (state->rmid != rmid);else
@@ -984,21 +986,17 @@ static void intel_cqm_event_start(struct* Technology component. */ wrmsr (MSR_IA32_PQR_ASSOC , rmid,0 ); - -raw_spin_unlock_irqrestore (&state->lock, flags); }static void intel_cqm_event_stop (struct perf_event *event,int mode) {struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); -unsigned long flags;if (event->hw.cqm_state &PERF_HES_STOPPED )return ; event->hw.cqm_state |=PERF_HES_STOPPED ; -raw_spin_lock_irqsave (&state->lock, flags);intel_cqm_event_read (event);if (!--state->cnt) {
@@ -1013,8 +1011,6 @@ static void intel_cqm_event_stop(struct }else {WARN_ON_ONCE (!state->rmid); } - -raw_spin_unlock_irqrestore (&state->lock, flags); }static int intel_cqm_event_add (struct perf_event *event,int mode)
@@ -1257,7 +1253,6 @@ static void intel_cqm_cpu_prepare(unsignstruct intel_cqm_state *state = &per_cpu (cqm_state, cpu);struct cpuinfo_x86 *c = &cpu_data (cpu); -raw_spin_lock_init (&state->lock); state->rmid =0 ; state->cnt =0 ;
Re: [patch 3/6] x86, perf, cqm: Remove pointless spinlock from state cache
Quoted 11 lines · Thomas Gleixner
struct
intel_cqm_stateis a strict per cpu cache of the rmid and the usage counter. It can never be modified from a remote cpu.The 3 functions which modify the content: start, stop and del (del maps to stop) are called from the perf core with interrupts disabled which is enough protection for the per cpu state values.
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
The state locking code was taken from Peter's original patch last year, so it would be good for him to chime in that this is safe. It's probably just that it was necessary in Peter's patches but after I refactored bits I forgot to rip it out.
But yeah, from reading the code again the lock does look entirely superfluous.
So unless Peter complains,
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
Peter Zijlstra
maintainerRe: [patch 3/6] x86, perf, cqm: Remove pointless spinlock from state cache
Quoted 13 lines · Matt Fleming
Quoted 11 lines · Thomas Gleixner
struct
intel_cqm_stateis a strict per cpu cache of the rmid and the usage counter. It can never be modified from a remote cpu.The 3 functions which modify the content: start, stop and del (del maps to stop) are called from the perf core with interrupts disabled which is enough protection for the per cpu state values.
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
Quoted 7 lines
The state locking code was taken from Peter's original patch last year, so it would be good for him to chime in that this is safe. It's probably just that it was necessary in Peter's patches but after I refactored bits I forgot to rip it out.
But yeah, from reading the code again the lock does look entirely superfluous.
I think that all stems from a point in time when it wasn't at all clear to me what the hardware looked like, but what do I know, I can't even remember last week.
All the patches looked good to me, so I already queued them.
I'll add your Ack on them.
tip-bot for Thomas Gleixner
merge bot[tip:perf/core] perf/x86/intel/cqm: Remove pointless spinlock from state cache
In reply to Thomas Gleixnerperf/x86/intel/cqm: Remove pointless spinlock from state cache
Commit-ID: 9e7eaac95af6c1aecaf558b8c7a1757d5f2d2ad7 Gitweb: http://git.kernel.org/tip/9e7eaac95af6c1aecaf558b8c7a1757d5f2d2ad7 Author: Thomas Gleixner <[email protected]> AuthorDate: Tue, 19 May 2015 00:00:53 +0000 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 27 May 2015 09:17:39 +0200
perf/x86/intel/cqm: Remove pointless spinlock from state cache
'struct intel_cqm_state' is a strict per CPU cache of the rmid and the usage counter. It can never be modified from a remote CPU.
The three functions which modify the content: intel_cqm_event[start|stop|del] (del maps to stop) are called from the perf core with interrupts disabled which is enough protection for the per CPU state values.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +6 -11
@@ -17,11 +17,16 @@ static unsigned int cqm_max_rmid = -1;static unsigned int cqm_l3_scale;/* supposedly cacheline size */ struct intel_cqm_state { -raw_spinlock_t lock;u32 rmid;int cnt; }; +/* +* The cached intel_cqm_state is strictly per CPU and can never be +* updated from a remote CPU. Both functions which modify the state +* (intel_cqm_event_start and intel_cqm_event_stop) are called with +* interrupts disabled, which is sufficient for the protection. +*/ static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state);/*
@@ -963,15 +968,12 @@ static void intel_cqm_event_start(struct perf_event *event, int mode) {struct intel_cqm_state *state =this_cpu_ptr (&cqm_state);u32 rmid = event->hw.cqm_rmid; -unsigned long flags;if (!(event->hw.cqm_state &PERF_HES_STOPPED ))return ; event->hw.cqm_state &= ~PERF_HES_STOPPED ; -raw_spin_lock_irqsave (&state->lock, flags); -if (state->cnt++)WARN_ON_ONCE (state->rmid != rmid);else
@@ -984,21 +986,17 @@ static void intel_cqm_event_start(struct perf_event *event, int mode)* Technology component. */ wrmsr (MSR_IA32_PQR_ASSOC , rmid,0 ); - -raw_spin_unlock_irqrestore (&state->lock, flags); }static void intel_cqm_event_stop (struct perf_event *event,int mode) {struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); -unsigned long flags;if (event->hw.cqm_state &PERF_HES_STOPPED )return ; event->hw.cqm_state |=PERF_HES_STOPPED ; -raw_spin_lock_irqsave (&state->lock, flags);intel_cqm_event_read (event);if (!--state->cnt) {
@@ -1013,8 +1011,6 @@ static void intel_cqm_event_stop(struct perf_event *event, int mode) }else {WARN_ON_ONCE (!state->rmid); } - -raw_spin_unlock_irqrestore (&state->lock, flags); }static int intel_cqm_event_add (struct perf_event *event,int mode)
@@ -1257,7 +1253,6 @@ static void intel_cqm_cpu_prepare(unsigned int cpu)struct intel_cqm_state *state = &per_cpu (cqm_state, cpu);struct cpuinfo_x86 *c = &cpu_data (cpu); -raw_spin_lock_init (&state->lock); state->rmid =0 ; state->cnt =0 ;
RE: [patch 3/6] x86, perf, cqm: Remove pointless spinlock from state cache
In reply to Thomas GleixnerQuoted 97 lines
-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Monday, May 18, 2015 5:01 PM
To: LKML
Cc: Peter Zijlstra; Vikas Shivappa; [email protected]; Fleming, Matt; Auld, Will; Juvva, Kanaka D
Subject: [patch 3/6] x86, perf, cqm: Remove pointless spinlock from state cachestruct
intel_cqm_stateis a strict per cpu cache of the rmid and the usage counter. It can never be modified from a remote cpu.The 3 functions which modify the content: start, stop and del (del maps to stop) are called from the perf core with interrupts disabled which is enough protection for the per cpu state values.
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)Index:
linux/arch/x86/kernel/cpu/perf_event_intel_cqm.c
=================================================================
==arch/x86/kernel/cpu/perf_event_intel_cqm.c +0
-0@@ -17,11 +17,16 @@ static unsigned int cqm_max_rmid = -1; static unsignedint
cqm_l3_scale; /* supposedly cacheline size */struct
intel_cqm_state{arch/x86/kernel/cpu/perf_event_intel_cqm.c +6
-10-raw_spinlock_t lock;u32 rmid;int cnt; }; +/* +* The cached intel_cqm_state is strictly per cpu and can never be +* updated from a remote cpu. Both functions which modify the state +* (intel_cqm_event_start and intel_cqm_event_stop) are called with +* interrupts disabled, which is sufficient for the protection. +*/ static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state);/* @@ -963,15 +968,12 @@ static void intel_cqm_event_start(struct {struct intel_cqm_state *state =this_cpu_ptr (&cqm_state);u32 rmid = event->hw.cqm_rmid; -unsigned long flags;if (!(event->hw.cqm_state &PERF_HES_STOPPED ))return ; event->hw.cqm_state &= ~PERF_HES_STOPPED ; -raw_spin_lock_irqsave (&state->lock, flags); -if (state->cnt++)WARN_ON_ONCE (state->rmid != rmid);else @@ -984,21 +986,17 @@ static void intel_cqm_event_start(struct* Technology component. */ wrmsr (MSR_IA32_PQR_ASSOC , rmid,0 ); - -raw_spin_unlock_irqrestore (&state->lock, flags); }static void intel_cqm_event_stop (struct perf_event *event,int mode) {struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); -unsigned long flags;if (event->hw.cqm_state &PERF_HES_STOPPED )return ; event->hw.cqm_state |=PERF_HES_STOPPED ; -raw_spin_lock_irqsave (&state->lock, flags);intel_cqm_event_read (event);if (!--state->cnt) {@@ -1013,8 +1011,6 @@ static void intel_cqm_event_stop(struct }else {WARN_ON_ONCE (!state->rmid); } - -raw_spin_unlock_irqrestore (&state->lock, flags); } static int intel_cqm_event_add(struct perf_event *event, int mode) @@ -1257,7 +1253,6 @@ static void intel_cqm_cpu_prepare(unsign
arch/x86/kernel/cpu/perf_event_intel_cqm.c +0
-1struct intel_cqm_state *state = &per_cpu (cqm_state, cpu);struct cpuinfo_x86 *c = &cpu_data (cpu); -raw_spin_lock_init (&state->lock); state->rmid =0 ; state->cnt =0 ;
PATCH 4/6 x86, perf, cqm: Avoid pointless msr write
In reply to Thomas GleixnerIf the usage counter is non-zero there is no point to update the rmid in the PQR MSR.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +5 -3
@@ -974,10 +974,12 @@ static void intel_cqm_event_start(struct event->hw.cqm_state &= ~PERF_HES_STOPPED ; -if (state->cnt++) -WARN_ON_ONCE (state->rmid != rmid); -else +if (state->cnt++) { +if (!WARN_ON_ONCE (state->rmid != rmid)) +return ; + }else {WARN_ON_ONCE (state->rmid); + } state->rmid = rmid;/*
Re: [patch 4/6] x86, perf, cqm: Avoid pointless msr write
Quoted 7 lines · Thomas Gleixner
If the usage counter is non-zero there is no point to update the rmid in the PQR MSR.
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Good catch.
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
tip-bot for Thomas Gleixner
merge bot[tip:perf/core] perf/x86/intel/cqm: Avoid pointless MSR write
In reply to Thomas Gleixnerperf/x86/intel/cqm: Avoid pointless MSR write
Commit-ID: 0bac237845e203dd1439cfc571b1baf1b2274b3b Gitweb: http://git.kernel.org/tip/0bac237845e203dd1439cfc571b1baf1b2274b3b Author: Thomas Gleixner <[email protected]> AuthorDate: Tue, 19 May 2015 00:00:55 +0000 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 27 May 2015 09:17:40 +0200
perf/x86/intel/cqm: Avoid pointless MSR write
If the usage counter is non-zero there is no point to update the rmid in the PQR MSR.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +5 -3
@@ -974,10 +974,12 @@ static void intel_cqm_event_start(struct perf_event *event, int mode) event->hw.cqm_state &= ~PERF_HES_STOPPED ; -if (state->cnt++) -WARN_ON_ONCE (state->rmid != rmid); -else +if (state->cnt++) { +if (!WARN_ON_ONCE (state->rmid != rmid)) +return ; + }else {WARN_ON_ONCE (state->rmid); + } state->rmid = rmid;/*
PATCH 5/6 x86, perf, cqm: Remove useless wrapper function
In reply to Thomas Gleixnerintel_cqm_event_del is a 1:1 wrapper for intel_cqm_event_stop. Remove the useless gunk.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +1 -6
@@ -1033,11 +1033,6 @@ static int intel_cqm_event_add(struct pereturn 0 ; } -static void intel_cqm_event_del (struct perf_event *event,int mode) -{ -intel_cqm_event_stop (event, mode); -} -static void intel_cqm_event_destroy (struct perf_event *event) {struct perf_event *group_other =NULL ;
@@ -1230,7 +1225,7 @@ static struct pmu intel_cqm_pmu = { .task_ctx_nr = perf_sw_context, .event_init = intel_cqm_event_init, .add = intel_cqm_event_add, - .del = intel_cqm_event_del, + .del = intel_cqm_event_stop, .start = intel_cqm_event_start, .stop = intel_cqm_event_stop, .read = intel_cqm_event_read,
Re: [patch 5/6] x86, perf, cqm: Remove useless wrapper function
Quoted 7 lines · Thomas Gleixner
intel_cqm_event_delis a 1:1 wrapper forintel_cqm_event_stop. Remove the useless gunk.Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
tip-bot for Thomas Gleixner
merge bot[tip:perf/core] perf/x86/intel/cqm: Remove useless wrapper function
In reply to Thomas Gleixnerperf/x86/intel/cqm: Remove useless wrapper function
Commit-ID: 43d0c2f6dcd07ffc0de658a7fbeeb63c806e9caa Gitweb: http://git.kernel.org/tip/43d0c2f6dcd07ffc0de658a7fbeeb63c806e9caa Author: Thomas Gleixner <[email protected]> AuthorDate: Tue, 19 May 2015 00:00:56 +0000 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 27 May 2015 09:17:40 +0200
perf/x86/intel/cqm: Remove useless wrapper function
intel_cqm_event_del() is a 1:1 wrapper for intel_cqm_event_stop(). Remove the useless indirection.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +1 -6
@@ -1033,11 +1033,6 @@ static int intel_cqm_event_add(struct perf_event *event, int mode)return 0 ; } -static void intel_cqm_event_del (struct perf_event *event,int mode) -{ -intel_cqm_event_stop (event, mode); -} -static void intel_cqm_event_destroy (struct perf_event *event) {struct perf_event *group_other =NULL ;
@@ -1230,7 +1225,7 @@ static struct pmu intel_cqm_pmu = { .task_ctx_nr = perf_sw_context, .event_init = intel_cqm_event_init, .add = intel_cqm_event_add, - .del = intel_cqm_event_del, + .del = intel_cqm_event_stop, .start = intel_cqm_event_start, .stop = intel_cqm_event_stop, .read = intel_cqm_event_read,
PATCH 6/6 x86, perf, cqm: Add storage for closid and cleanup struct intel_pqr_state
In reply to Thomas Gleixnerclosid (CLass Of Service ID) is used for the Class based Cache Allocation Technology (CAT). Add explicit storage to the per cpu cache for it, so it can be used later with the CAT support (requires to move the per cpu data).
While at it:
- Rename the structure to intel_pqr_state which reflects the actual purpose of the struct: Cache values which go into the PQR MSR
- Rename 'cnt' to rmid_usecnt which reflects the actual purpose of the counter.
- Document the structure and the struct members.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 50 +++++++++++++++-------------- 1 file changed, 27 insertions(+), 23 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +27 -23
@@ -16,18 +16,32 @@static unsigned int cqm_max_rmid =-1 ;static unsigned int cqm_l3_scale;/* supposedly cacheline size */ -struct intel_cqm_state { +/** +* struct intel_pqr_state - State cache for the PQR MSR +* @rmid: The cached Resource Monitoring ID +* @closid: The cached Class Of Service ID +* @usecnt: The usage counter for rmid +* +* The upper 32 bits of MSR_IA32_PQR_ASSOC contain closid and the +* lower 10 bits rmid. The update to MSR_IA32_PQR_ASSOC always +* contains both parts, so we need to cache them. +* +* The cache also helps to avoid pointless updates if the value does +* not change. +*/ +struct intel_pqr_state {u32 rmid; -int cnt; +u32 closid; +int rmid_usecnt; };/* -* The cached intel_cqm_state is strictly per cpu and can never be +* The cached intel_pqr_state is strictly per cpu and can never be * updated from a remote cpu. Both functions which modify the state * (intel_cqm_event_start and intel_cqm_event_stop) are called with * interrupts disabled, which is sufficient for the protection. */ -static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state); +static DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);/* * Protects cache_cgroups and cqm_rmid_free_lru and cqm_rmid_limbo_lru.
@@ -966,7 +980,7 @@ out:static void intel_cqm_event_start (struct perf_event *event,int mode) { -struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); +struct intel_pqr_state *state =this_cpu_ptr (&pqr_state);u32 rmid = event->hw.cqm_rmid;if (!(event->hw.cqm_state &PERF_HES_STOPPED ))
@@ -974,7 +988,7 @@ static void intel_cqm_event_start(struct event->hw.cqm_state &= ~PERF_HES_STOPPED ; -if (state->cnt++) { +if (state->rmid_usecnt++) {if (!WARN_ON_ONCE (state->rmid != rmid))return ; }else {
@@ -982,17 +996,12 @@ static void intel_cqm_event_start(struct } state->rmid = rmid; -/* -* This is actually wrong, as the upper 32 bit MSR contain the -* closid which is used for configuring the Cache Allocation -* Technology component. -*/ -wrmsr (MSR_IA32_PQR_ASSOC , rmid,0 ); +wrmsr (MSR_IA32_PQR_ASSOC , rmid, state->closid); }static void intel_cqm_event_stop (struct perf_event *event,int mode) { -struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); +struct intel_pqr_state *state =this_cpu_ptr (&pqr_state);if (event->hw.cqm_state &PERF_HES_STOPPED )return ;
@@ -1001,15 +1010,9 @@ static void intel_cqm_event_stop(structintel_cqm_event_read (event); -if (!--state->cnt) { +if (!--state->rmid_usecnt) { state->rmid =0 ; -/* -* This is actually wrong, as the upper 32 bit of the -* MSR contain the closid which is used for -* configuring the Cache Allocation Technology -* component. -*/ -wrmsr (MSR_IA32_PQR_ASSOC ,0 ,0 ); +wrmsr (MSR_IA32_PQR_ASSOC ,0 , state->closid); }else {WARN_ON_ONCE (!state->rmid); }
@@ -1247,11 +1250,12 @@ static inline void cqm_pick_event_readerstatic void intel_cqm_cpu_prepare (unsigned int cpu) { -struct intel_cqm_state *state = &per_cpu (cqm_state, cpu); +struct intel_pqr_state *state = &per_cpu (pqr_state, cpu);struct cpuinfo_x86 *c = &cpu_data (cpu); state->rmid =0 ; - state->cnt =0 ; + state->closid =0 ; + state->rmid_usecnt =0 ;WARN_ON (c->x86_cache_max_rmid != cqm_max_rmid);WARN_ON (c->x86_cache_occ_scale != cqm_l3_scale);
Re: [patch 6/6] x86, perf, cqm: Add storage for closid and cleanup struct intel_pqr_state
Quoted 25 lines · Thomas Gleixner
closid (CLass Of Service ID) is used for the Class based Cache Allocation Technology (CAT). Add explicit storage to the per cpu cache for it, so it can be used later with the CAT support (requires to move the per cpu data).
While at it:
- Rename the structure to
intel_pqr_statewhich reflects the actual purpose of the struct: Cache values which go into the PQR MSR- Rename 'cnt' to
rmid_usecntwhich reflects the actual purpose of the counter.- Document the structure and the struct members.
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 50 +++++++++++++++-------------- 1 file changed, 27 insertions(+), 23 deletions(-)Index:
linux/arch/x86/kernel/cpu/perf_event_intel_cqm.c
===================================================================
arch/x86/kernel/cpu/perf_event_intel_cqm.c line 24
Quoted 10 lines
arch/x86/kernel/cpu/perf_event_intel_cqm.c +6
-1@@ -16,18 +16,32 @@static unsigned int cqm_max_rmid =-1 ;static unsigned int cqm_l3_scale;/* supposedly cacheline size */ -struct intel_cqm_state { +/** +* struct intel_pqr_state - State cache for the PQR MSR +* @rmid: The cached Resource Monitoring ID +* @closid: The cached Class Of Service ID +* @usecnt: The usage counter for rmid +*
Typo? Should be @rmid_usecnt.
Otherwise,
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
Re: [patch 6/6] x86, perf, cqm: Add storage for closid and cleanup struct intel_pqr_state
Quoted 29 lines · Matt Fleming
Quoted 28 lines · Thomas Gleixner
closid (CLass Of Service ID) is used for the Class based Cache Allocation Technology (CAT). Add explicit storage to the per cpu cache for it, so it can be used later with the CAT support (requires to move the per cpu data).
While at it:
- Rename the structure to
intel_pqr_statewhich reflects the actual purpose of the struct: Cache values which go into the PQR MSR- Rename 'cnt' to
rmid_usecntwhich reflects the actual purpose of the counter.- Document the structure and the struct members.
Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 50 +++++++++++++++-------------- 1 file changed, 27 insertions(+), 23 deletions(-)Index:
linux/arch/x86/kernel/cpu/perf_event_intel_cqm.c
===================================================================arch/x86/kernel/cpu/perf_event_intel_cqm.c +0
-0@@ -16,18 +16,32 @@static unsigned int cqm_max_rmid =-1 ;static unsigned int cqm_l3_scale;/* supposedly cacheline size */
Quoted 9 lines
arch/x86/kernel/cpu/perf_event_intel_cqm.c line 24
Quoted 7 lines
arch/x86/kernel/cpu/perf_event_intel_cqm.c +6
-1-struct intel_cqm_state { +/** +* struct intel_pqr_state - State cache for the PQR MSR +* @rmid: The cached Resource Monitoring ID +* @closid: The cached Class Of Service ID +* @usecnt: The usage counter for rmid +* Typo? Should be @rmid_usecnt.
Indeed.
tip-bot for Thomas Gleixner
merge bot[tip:perf/core] perf/x86/intel/cqm: Add storage for 'closid' and clean up 'struct intel_pqr_state'
In reply to Thomas Gleixnerperf/x86/intel/cqm: Add storage for 'closid' and clean up 'struct intel_pqr_state'
Commit-ID: bf926731e1585ccad029ca2fad1444fee082b78d Gitweb: http://git.kernel.org/tip/bf926731e1585ccad029ca2fad1444fee082b78d Author: Thomas Gleixner <[email protected]> AuthorDate: Tue, 19 May 2015 00:00:58 +0000 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 27 May 2015 09:17:41 +0200
perf/x86/intel/cqm: Add storage for 'closid' and clean up 'struct intel_pqr_state'
'closid' (CLass Of Service ID) is used for the Class based Cache Allocation Technology (CAT). Add explicit storage to the per cpu cache for it, so it can be used later with the CAT support (requires to move the per cpu data).
While at it:
- Rename the structure to intel_pqr_state which reflects the actual purpose of the struct: cache values which go into the PQR MSR
- Rename 'cnt' to rmid_usecnt which reflects the actual purpose of the counter.
- Document the structure and the struct members.
--- arch/x86/kernel/cpu/perf_event_intel_cqm.c | 50 ++++++++++++++++-------------- 1 file changed, 27 insertions(+), 23 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_cqm.c +27 -23
@@ -16,18 +16,32 @@static unsigned int cqm_max_rmid =-1 ;static unsigned int cqm_l3_scale;/* supposedly cacheline size */ -struct intel_cqm_state { +/** +* struct intel_pqr_state - State cache for the PQR MSR +* @rmid: The cached Resource Monitoring ID +* @closid: The cached Class Of Service ID +* @rmid_usecnt: The usage counter for rmid +* +* The upper 32 bits of MSR_IA32_PQR_ASSOC contain closid and the +* lower 10 bits rmid. The update to MSR_IA32_PQR_ASSOC always +* contains both parts, so we need to cache them. +* +* The cache also helps to avoid pointless updates if the value does +* not change. +*/ +struct intel_pqr_state {u32 rmid; -int cnt; +u32 closid; +int rmid_usecnt; };/* -* The cached intel_cqm_state is strictly per CPU and can never be +* The cached intel_pqr_state is strictly per CPU and can never be * updated from a remote CPU. Both functions which modify the state * (intel_cqm_event_start and intel_cqm_event_stop) are called with * interrupts disabled, which is sufficient for the protection. */ -static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state); +static DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);/* * Protects cache_cgroups and cqm_rmid_free_lru and cqm_rmid_limbo_lru.
@@ -966,7 +980,7 @@ out:static void intel_cqm_event_start (struct perf_event *event,int mode) { -struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); +struct intel_pqr_state *state =this_cpu_ptr (&pqr_state);u32 rmid = event->hw.cqm_rmid;if (!(event->hw.cqm_state &PERF_HES_STOPPED ))
@@ -974,7 +988,7 @@ static void intel_cqm_event_start(struct perf_event *event, int mode) event->hw.cqm_state &= ~PERF_HES_STOPPED ; -if (state->cnt++) { +if (state->rmid_usecnt++) {if (!WARN_ON_ONCE (state->rmid != rmid))return ; }else {
@@ -982,17 +996,12 @@ static void intel_cqm_event_start(struct perf_event *event, int mode) } state->rmid = rmid; -/* -* This is actually wrong, as the upper 32 bit MSR contain the -* closid which is used for configuring the Cache Allocation -* Technology component. -*/ -wrmsr (MSR_IA32_PQR_ASSOC , rmid,0 ); +wrmsr (MSR_IA32_PQR_ASSOC , rmid, state->closid); }static void intel_cqm_event_stop (struct perf_event *event,int mode) { -struct intel_cqm_state *state =this_cpu_ptr (&cqm_state); +struct intel_pqr_state *state =this_cpu_ptr (&pqr_state);if (event->hw.cqm_state &PERF_HES_STOPPED )return ;
@@ -1001,15 +1010,9 @@ static void intel_cqm_event_stop(struct perf_event *event, int mode)intel_cqm_event_read (event); -if (!--state->cnt) { +if (!--state->rmid_usecnt) { state->rmid =0 ; -/* -* This is actually wrong, as the upper 32 bit of the -* MSR contain the closid which is used for -* configuring the Cache Allocation Technology -* component. -*/ -wrmsr (MSR_IA32_PQR_ASSOC ,0 ,0 ); +wrmsr (MSR_IA32_PQR_ASSOC ,0 , state->closid); }else {WARN_ON_ONCE (!state->rmid); }
@@ -1247,11 +1250,12 @@ static inline void cqm_pick_event_reader(int cpu)static void intel_cqm_cpu_prepare (unsigned int cpu) { -struct intel_cqm_state *state = &per_cpu (cqm_state, cpu); +struct intel_pqr_state *state = &per_cpu (pqr_state, cpu);struct cpuinfo_x86 *c = &cpu_data (cpu); state->rmid =0 ; - state->cnt =0 ; + state->closid =0 ; + state->rmid_usecnt =0 ;WARN_ON (c->x86_cache_max_rmid != cqm_max_rmid);WARN_ON (c->x86_cache_occ_scale != cqm_l3_scale);
Peter Zijlstra
maintainerQuoted 5 lines · Thomas Gleixner
While reviewing the RDT/CAT patches I had to look into the perf CQM code. As usual when my review mood reaches the grumpiness level, I start to poke around in the code some more and find stuff which really sucks.
Quoted 7 lines
Here is a step by step patch series, which cleans up and clarifies the code and prepares it for reuasage of the PQR cache for the RDT/CAT stuff.
Compile tested only.
Shameless hint @Intel: I have no access to a box with CQM/RDT :)
Thanks!