measure SMI cost (kernel)

From: Kan Liang <[email protected]>

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

From: Kan Liang <[email protected]>

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.

---
 arch/x86/include/asm/msr.h | 25 +++++++++++++++
 arch/x86/lib/msr-smp.c     | 76 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

index 898dba2..bfd83ab 100644
@@ -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);

2.7.4

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.

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

index d1dee753b949..35763927adaa 100644
@@ -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

.

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

Yes.

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

Thanks,

tglx

From: Kan Liang <[email protected]>

Currently, the SMIs are visible to all performance counters. Because many users want to measure everything including SMIs. But in some cases, the SMI cycles should not be count. For example, to calculate the cost of SMI itself. So a knob is needed.

When setting FREEZE_WHILE_SMM bit in IA32_DEBUGCTL, all performance counters will be effected. There is no way to do per-counter freeze on SMI. So it should not use the per-event interface (e.g. ioctl or event attribute) to set FREEZE_WHILE_SMM bit.

Adds sysfs entry /sys/device/cpu/freeze_on_smi to set FREEZE_WHILE_SMM bit in IA32_DEBUGCTL. When set, freezes perfmon and trace messages while in SMM.
Value has to be 0 or 1. It will be applied to all possible cpus.

---
 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 ++
 4 files changed, 63 insertions(+)

index 349d4d1..c16fb50 100644
@@ -1750,6 +1750,8 @@ ssize_t x86_event_sysfs_show(char *page, u64 config, u64 event)
 	return ret;
 }
 
+static struct attribute_group x86_pmu_attr_group;
+
 static int __init init_hw_perf_events(void)
 {
 	struct x86_pmu_quirk *quirk;

@@ -1813,6 +1815,14 @@ static int __init init_hw_perf_events(void)
 			x86_pmu_events_group.attrs = tmp;
 	}
 
+	if (x86_pmu.attrs) {
+		struct attribute **tmp;
+
+		tmp = merge_attr(x86_pmu_attr_group.attrs, x86_pmu.attrs);
+		if (!WARN_ON(!tmp))
+			x86_pmu_attr_group.attrs = tmp;
+	}
+
 	pr_info("... version:                %d\n",     x86_pmu.version);
 	pr_info("... bit width:              %d\n",     x86_pmu.cntval_bits);
 	pr_info("... generic registers:      %d\n",     x86_pmu.num_counters);

index bcbb1d2..110cb9b0 100644
@@ -561,6 +561,9 @@ struct x86_pmu {
 	ssize_t		(*events_sysfs_show)(char *page, u64 config);
 	struct attribute **cpu_events;
 
+	int		attr_freeze_on_smi;
+	struct attribute **attrs;
+
 	/*
 	 * CPU Hotplug hooks
 	 */

index d8b5f8a..bdb00fa 100644
@@ -134,6 +134,8 @@
 #define DEBUGCTLMSR_BTS_OFF_OS		(1UL <<  9)
 #define DEBUGCTLMSR_BTS_OFF_USR		(1UL << 10)
 #define DEBUGCTLMSR_FREEZE_LBRS_ON_PMI	(1UL << 11)
+#define DEBUGCTLMSR_FREEZE_WHILE_SMM_BIT	14
+#define DEBUGCTLMSR_FREEZE_WHILE_SMM	(1UL << DEBUGCTLMSR_FREEZE_WHILE_SMM_BIT)
 
 #define MSR_PEBS_FRONTEND		0x000003f7
 

2.7.4

I've patched the intel_pmu_cpu_starting.
I think it guarantees that the new online CPU is set.

@@ -3174,6 +3174,11 @@ static void intel_pmu_cpu_starting(int cpu)

cpuc->lbr_sel = NULL;

+ if (x86_pmu.attr_freeze_on_smi)
+ msr_set_bit_on_cpu(cpu, MSR_IA32_DEBUGCTLMSR, DEBUGCTLMSR_FREEZE_WHILE_SMM_BIT);
+ else
+ msr_clear_bit_on_cpu(cpu, MSR_IA32_DEBUGCTLMSR, DEBUGCTLMSR_FREEZE_WHILE_SMM_BIT);
+
if (!cpuc->shared_regs)
return;

Thanks,
Kan

Only when the hotplug protection of the write is in place.....