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

Groups > linux.kernel > #1609964 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH V2 0/2] measure SMI cost (kernel) kan.liang@intel.com - 2017-03-27 18:10 +0200
    [PATCH V2 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions kan.liang@intel.com - 2017-03-27 18:10 +0200
      Re: [PATCH V2 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access  functions Borislav Petkov <bp@alien8.de> - 2017-03-27 19:00 +0200
        RE: [PATCH V2 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus  access functions "Liang, Kan" <kan.liang@intel.com> - 2017-03-27 19:20 +0200

#1609964 — [PATCH V2 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

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       | 29 +++++++++++++++
 arch/x86/lib/msr-smp.c           | 76 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 168 insertions(+)

-- 
2.7.4

[toc] | [next] | [standalone]

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

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

Having msr_set/clear_bit on many cpus or given CPU can avoid extra
unnecessory IPIs and simplify MSR content manipulation, when it only
needs to flip a bit.
There is already msr_set/clear_bit, but missing the _on_cpu and _on_cpus
version.

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

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 898dba2..9bc999b 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -20,6 +20,11 @@ struct msr {
 	};
 };
 
+struct msr_bit_info {
+	u32 msr_no;
+	u8 bit;
+};
+
 struct msr_info {
 	u32 msr_no;
 	struct msr reg;
@@ -314,6 +319,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 +336,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..100b3cb 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_bit_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_bit_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_bit_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_bit_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_bit_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_bit_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]

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

On Mon, Mar 27, 2017 at 08:47:37AM -0700, kan.liang@intel.com wrote:
> From: Kan Liang <Kan.liang@intel.com>
> 
> Having msr_set/clear_bit on many cpus or given CPU can avoid extra
> unnecessory IPIs

How does that happen?

You have smp_call_function_many() sending IPIs to each CPU in the mask.
Doesn't look like avoiding anything to me.

Now if you want to have interfaces set/clear_bit_on_cpu(s), that's a
different story.

And those actually double the amount of IPIs the moment you do a
read-modify-write operation on the MSR, i.e., you want to read *and*
write afterwards.

If you only want to do a single operation - set or clear - like you're
doing in your other patch, then I guess that's fine as it wraps the
smp_call_function* boilerplate code.

> and simplify MSR content manipulation, when it only
> needs to flip a bit.
> There is already msr_set/clear_bit, but missing the _on_cpu and _on_cpus
> version.
> 
> Signed-off-by: Kan Liang <Kan.liang@intel.com>
> ---
>  arch/x86/include/asm/msr.h | 29 ++++++++++++++++++
>  arch/x86/lib/msr-smp.c     | 76 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 898dba2..9bc999b 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -20,6 +20,11 @@ struct msr {
>  	};
>  };
>  
> +struct msr_bit_info {
> +	u32 msr_no;
> +	u8 bit;
> +};

No, not *another* struct msr*info. Please reuse msr_info.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

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

> 
> On Mon, Mar 27, 2017 at 08:47:37AM -0700, kan.liang@intel.com wrote:
> > From: Kan Liang <Kan.liang@intel.com>
> >
> > Having msr_set/clear_bit on many cpus or given CPU can avoid extra
> > unnecessory IPIs
> 
> How does that happen?
>

My previous patch did a read-modify-write operation. Compared with the
single operation set/clear, it will has extra IPIs.
Sorry for the confusing wording.
I will change the description. 

> You have smp_call_function_many() sending IPIs to each CPU in the mask.
> Doesn't look like avoiding anything to me.
> 
> Now if you want to have interfaces set/clear_bit_on_cpu(s), that's a
> different story.
> 
> And those actually double the amount of IPIs the moment you do a read-
> modify-write operation on the MSR, i.e., you want to read *and* write
> afterwards.
> 
> If you only want to do a single operation - set or clear - like you're doing in
> your other patch, then I guess that's fine as it wraps the
> smp_call_function* boilerplate code.
> 
> > and simplify MSR content manipulation, when it only needs to flip a
> > bit.
> > There is already msr_set/clear_bit, but missing the _on_cpu and
> > _on_cpus version.
> >
> > Signed-off-by: Kan Liang <Kan.liang@intel.com>
> > ---
> >  arch/x86/include/asm/msr.h | 29 ++++++++++++++++++
> >  arch/x86/lib/msr-smp.c     | 76
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 105 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> > index 898dba2..9bc999b 100644
> > --- a/arch/x86/include/asm/msr.h
> > +++ b/arch/x86/include/asm/msr.h
> > @@ -20,6 +20,11 @@ struct msr {
> >  	};
> >  };
> >
> > +struct msr_bit_info {
> > +	u32 msr_no;
> > +	u8 bit;
> > +};
> 
> No, not *another* struct msr*info. Please reuse msr_info.
> 

OK.

Thanks,
Kan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web