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

Groups > linux.kernel > #1610071 > unrolled thread

[PATCH V3 0/2] measure SMI cost (kernel)

Back to article view | Back to linux.kernel

Contents

  [PATCH V3 0/2] measure SMI cost (kernel) kan.liang@intel.com - 2017-03-27 20:50 +0200
    [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions kan.liang@intel.com - 2017-03-27 20:50 +0200
      Re: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access  functions Thomas Gleixner <tglx@linutronix.de> - 2017-03-28 10:40 +0200
        Re: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access  functions Thomas Gleixner <tglx@linutronix.de> - 2017-03-28 19:30 +0200
          RE: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus  access functions "Liang, Kan" <kan.liang@intel.com> - 2017-03-28 19:50 +0200
            RE: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access  functions Thomas Gleixner <tglx@linutronix.de> - 2017-03-28 20:40 +0200

#1610071 — [PATCH V3 0/2] measure SMI cost (kernel)

From: Kan Liang <Kan.liang@intel.com>

Currently, there is no way to measure the time cost in System management
mode (SMM) by perf.

Intel perfmon supports FREEZE_WHILE_SMM bit in IA32_DEBUGCTL. Once it sets,
the PMU core counters will freeze on SMI handler. But it will not have an
effect on free running counters. E.g. APERF counter.
The cost of SMI can be measured by (aperf - cycles).

A new sysfs entry /sys/device/cpu/freeze_on_smi is introduced to set
FREEZE_WHILE_SMM bit in IA32_DEBUGCTL.

A new --smi-cost mode in perf stat is implemented to measure the SMI cost
by calculating cycles and aperf results. In practice, the percentages of
SMI cycles should be more useful than absolute value. So the output will be
the percentage of SMI cycles and SMI#.
If user wants to get the actual cycles, they can apply --no-metric-only.

Here is an example output.

 Performance counter stats for 'sudo echo ':

SMI cycles%          SMI#
    0.1%              1

       0.010858678 seconds time elapsed

Changes since V1:
 - Only include kernel patch
 - New functions to set msr bit on cpu and cpus.
   Using the new functions to replace rdmsrl_on_cpu and wrmsrl_on_cpu.
   That avoids the extra IPIs and atomic issue.
 - Support hotplug

Changes since V2:
 - reuse msr_info

Kan Liang (2):
  x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions
  perf/x86: add sysfs entry to freeze counter on SMI

 arch/x86/events/core.c           | 10 ++++++
 arch/x86/events/intel/core.c     | 48 +++++++++++++++++++++++++
 arch/x86/events/perf_event.h     |  3 ++
 arch/x86/include/asm/msr-index.h |  2 ++
 arch/x86/include/asm/msr.h       | 25 +++++++++++++
 arch/x86/lib/msr-smp.c           | 76 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 164 insertions(+)

-- 
2.7.4

[toc] | [next] | [standalone]

#1610073 — [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions

From: Kan Liang <Kan.liang@intel.com>

To flip a MSR bit on many CPUs or specific CPU, currently it has to do
read-modify-write operation on the MSR through rd/wrmsr_on_cpu(s).
It actually sends two IPIs to the given CPU.

It is necessory to extend the single operation - msr_set/clear_bit - on
many CPUs or given CPU. It only sends one IPI to the given CPU, and
simplify MSR content manipulation.
The new functions wrap the smp_call_function* boilerplate code.

Signed-off-by: Kan Liang <Kan.liang@intel.com>
---
 arch/x86/include/asm/msr.h | 25 +++++++++++++++
 arch/x86/lib/msr-smp.c     | 76 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 898dba2..bfd83ab 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -25,6 +25,7 @@ struct msr_info {
 	struct msr reg;
 	struct msr *msrs;
 	int err;
+	u8 bit;
 };
 
 struct msr_regs_info {
@@ -314,6 +315,10 @@ int msr_set_bit(u32 msr, u8 bit);
 int msr_clear_bit(u32 msr, u8 bit);
 
 #ifdef CONFIG_SMP
+int msr_set_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit);
+int msr_clear_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit);
+void msr_set_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit);
+void msr_clear_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit);
 int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
 int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
 int rdmsrl_on_cpu(unsigned int cpu, u32 msr_no, u64 *q);
@@ -327,6 +332,26 @@ int wrmsrl_safe_on_cpu(unsigned int cpu, u32 msr_no, u64 q);
 int rdmsr_safe_regs_on_cpu(unsigned int cpu, u32 regs[8]);
 int wrmsr_safe_regs_on_cpu(unsigned int cpu, u32 regs[8]);
 #else  /*  CONFIG_SMP  */
+static inline int msr_set_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit)
+{
+	return msr_set_bit(msr, bit);
+}
+
+static inline int msr_clear_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit)
+{
+	return msr_clear_bit(msr, bit);
+}
+
+static inline void msr_set_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit)
+{
+	msr_set_bit(msr, bit);
+}
+
+static inline void msr_clear_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit)
+{
+	msr_clear_bit(msr, bit);
+}
+
 static inline int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h)
 {
 	rdmsr(msr_no, *l, *h);
diff --git a/arch/x86/lib/msr-smp.c b/arch/x86/lib/msr-smp.c
index ce68b6a..8e704d9 100644
--- a/arch/x86/lib/msr-smp.c
+++ b/arch/x86/lib/msr-smp.c
@@ -3,6 +3,82 @@
 #include <linux/smp.h>
 #include <asm/msr.h>
 
+static void __msr_set_bit_on_cpu(void *info)
+{
+	struct msr_info *bit_info = info;
+
+	msr_set_bit(bit_info->msr_no, bit_info->bit);
+}
+
+static void __msr_clear_bit_on_cpu(void *info)
+{
+	struct msr_info *bit_info = info;
+
+	msr_clear_bit(bit_info->msr_no, bit_info->bit);
+}
+
+int msr_set_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit)
+{
+	struct msr_info info;
+	int err;
+
+	info.msr_no = msr;
+	info.bit = bit;
+
+	err = smp_call_function_single(cpu, __msr_set_bit_on_cpu, &info, 1);
+
+	return err;
+}
+EXPORT_SYMBOL(msr_set_bit_on_cpu);
+
+int msr_clear_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit)
+{
+	struct msr_info info;
+	int err;
+
+	info.msr_no = msr;
+	info.bit = bit;
+
+	err = smp_call_function_single(cpu, __msr_clear_bit_on_cpu, &info, 1);
+
+	return err;
+}
+EXPORT_SYMBOL(msr_clear_bit_on_cpu);
+
+void msr_set_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit)
+{
+	struct msr_info info;
+	int this_cpu;
+
+	info.msr_no = msr;
+	info.bit = bit;
+
+	this_cpu = get_cpu();
+	if (cpumask_test_cpu(this_cpu, mask))
+		__msr_set_bit_on_cpu(&info);
+
+	smp_call_function_many(mask, __msr_set_bit_on_cpu, &info, 1);
+	put_cpu();
+}
+EXPORT_SYMBOL(msr_set_bit_on_cpus);
+
+void msr_clear_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit)
+{
+	struct msr_info info;
+	int this_cpu;
+
+	info.msr_no = msr;
+	info.bit = bit;
+
+	this_cpu = get_cpu();
+	if (cpumask_test_cpu(this_cpu, mask))
+		__msr_clear_bit_on_cpu(&info);
+
+	smp_call_function_many(mask, __msr_clear_bit_on_cpu, &info, 1);
+	put_cpu();
+}
+EXPORT_SYMBOL(msr_clear_bit_on_cpus);
+
 static void __rdmsr_on_cpu(void *info)
 {
 	struct msr_info *rv = info;
-- 
2.7.4

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

#1610411 — Re: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions

On Mon, 27 Mar 2017, kan.liang@intel.com wrote:

> From: Kan Liang <Kan.liang@intel.com>
> 
> To flip a MSR bit on many CPUs or specific CPU, currently it has to do
> read-modify-write operation on the MSR through rd/wrmsr_on_cpu(s).
> It actually sends two IPIs to the given CPU.

The IPIs are the least of the problems, really. The real problem is that

       rdmsr_on_cpu()
       wrmsr_on_cpu()

is not atomic. That's what wants to be solved. The reduction of IPIs just a
side effect.

>  #else  /*  CONFIG_SMP  */
> +static inline int msr_set_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit)
> +{
> +	return msr_set_bit(msr, bit);
> +}
> +
> +static inline int msr_clear_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit)
> +{
> +	return msr_clear_bit(msr, bit);
> +}
> +
> +static inline void msr_set_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit)
> +{
> +	msr_set_bit(msr, bit);
> +}
> +
> +static inline void msr_clear_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit)
> +{
> +	msr_clear_bit(msr, bit);
> +}

This is utter crap because it's fundamentaly different from the SMP
version.

msr_set/clear_bit() are not protected by anyhting. And in your call site
this is invoked from fully preemptible context. What protects against
context switch and interrupts fiddling with DEBUGMSR?

Thanks,

	tglx

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

#1611239 — Re: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions

On Tue, 28 Mar 2017, Thomas Gleixner wrote:
> On Mon, 27 Mar 2017, kan.liang@intel.com wrote:
> 
> > From: Kan Liang <Kan.liang@intel.com>
> > 
> > To flip a MSR bit on many CPUs or specific CPU, currently it has to do
> > read-modify-write operation on the MSR through rd/wrmsr_on_cpu(s).
> > It actually sends two IPIs to the given CPU.
> 
> The IPIs are the least of the problems, really. The real problem is that
> 
>        rdmsr_on_cpu()
>        wrmsr_on_cpu()
> 
> is not atomic. That's what wants to be solved. The reduction of IPIs just a
> side effect.
> 
> >  #else  /*  CONFIG_SMP  */
> > +static inline int msr_set_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit)
> > +{
> > +	return msr_set_bit(msr, bit);
> > +}
> > +
> > +static inline int msr_clear_bit_on_cpu(unsigned int cpu, u32 msr, u8 bit)
> > +{
> > +	return msr_clear_bit(msr, bit);
> > +}
> > +
> > +static inline void msr_set_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit)
> > +{
> > +	msr_set_bit(msr, bit);
> > +}
> > +
> > +static inline void msr_clear_bit_on_cpus(const struct cpumask *mask, u32 msr, u8 bit)
> > +{
> > +	msr_clear_bit(msr, bit);
> > +}
> 
> This is utter crap because it's fundamentaly different from the SMP
> version.
> 
> msr_set/clear_bit() are not protected by anyhting. And in your call site
> this is invoked from fully preemptible context. What protects against
> context switch and interrupts fiddling with DEBUGMSR?

And thinking more about that whole interface. It's just overkill.

diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index d1dee753b949..35763927adaa 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -58,7 +58,7 @@ int msr_write(u32 msr, struct msr *m)
 	return wrmsrl_safe(msr, m->q);
 }
 
-static inline int __flip_bit(u32 msr, u8 bit, bool set)
+int msr_flip_bit(u32 msr, u8 bit, bool set)
 {
 	struct msr m, m1;
 	int err = -EINVAL;
@@ -85,6 +85,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(msr_flip_bit);
 
 /**
  * Set @bit in a MSR @msr.
@@ -96,7 +97,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
  */
 int msr_set_bit(u32 msr, u8 bit)
 {
-	return __flip_bit(msr, bit, true);
+	return msr_flip_bit(msr, bit, true);
 }
 
 /**
@@ -109,7 +110,7 @@ int msr_set_bit(u32 msr, u8 bit)
  */
 int msr_clear_bit(u32 msr, u8 bit)
 {
-	return __flip_bit(msr, bit, false);
+	return msr_flip_bit(msr, bit, false);
 }
 
 #ifdef CONFIG_TRACEPOINTS

And in the driver:

static void flip_smm_bit(void *data)
{
	int val = *(int *)data;
	
	msr_flip_bit(DEBUGMSR, SMMBIT, val);
}

And in the write function:

       smp_call_function(flip_smm_bit, &val, 1);

That avoids all the extra interfaces and requires less code and less
text foot print when unused .....

Thanks,

	tglx

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

#1611252 — RE: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions

.
> > msr_set/clear_bit() are not protected by anyhting. And in your call
> > site this is invoked from fully preemptible context. What protects
> > against context switch and interrupts fiddling with DEBUGMSR?
> 
> And thinking more about that whole interface. It's just overkill.
> 
> diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c index
> d1dee753b949..35763927adaa 100644
> --- a/arch/x86/lib/msr.c
> +++ b/arch/x86/lib/msr.c
> @@ -58,7 +58,7 @@ int msr_write(u32 msr, struct msr *m)
>  	return wrmsrl_safe(msr, m->q);
>  }
> 
> -static inline int __flip_bit(u32 msr, u8 bit, bool set)
> +int msr_flip_bit(u32 msr, u8 bit, bool set)
>  {
>  	struct msr m, m1;
>  	int err = -EINVAL;
> @@ -85,6 +85,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
> 
>  	return 1;
>  }
> +EXPORT_SYMBOL_GPL(msr_flip_bit);
> 
>  /**
>   * Set @bit in a MSR @msr.
> @@ -96,7 +97,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
>   */
>  int msr_set_bit(u32 msr, u8 bit)
>  {
> -	return __flip_bit(msr, bit, true);
> +	return msr_flip_bit(msr, bit, true);
>  }
> 
>  /**
> @@ -109,7 +110,7 @@ int msr_set_bit(u32 msr, u8 bit)
>   */
>  int msr_clear_bit(u32 msr, u8 bit)
>  {
> -	return __flip_bit(msr, bit, false);
> +	return msr_flip_bit(msr, bit, false);
>  }
> 
>  #ifdef CONFIG_TRACEPOINTS
> 
> And in the driver:
> 
> static void flip_smm_bit(void *data)
> {
> 	int val = *(int *)data;
> 
> 	msr_flip_bit(DEBUGMSR, SMMBIT, val);
> }
> 
> And in the write function:
> 
>        smp_call_function(flip_smm_bit, &val, 1);
> 
> That avoids all the extra interfaces and requires less code and less text foot
> print when unused .....
> 

Thanks. It simplify the code very much.
I think we still need to protect the smp_call_function in the driver, right?
Would be the following code enough?

get_online_cpus();
preempt_disable();
smp_call_function(flip_smm_bit, &val, 1);
preempt_enable();
put_online_cpus();

Thanks,
Kan

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

#1611273 — RE: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions

On Tue, 28 Mar 2017, Liang, Kan wrote:
> Thanks. It simplify the code very much.
> I think we still need to protect the smp_call_function in the driver, right?

Yes.

> Would be the following code enough?
> 
> get_online_cpus();
> preempt_disable();

Only get_online_cpus(). smp_call_function() disables preemption internaly.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web