[PATCH v7 0/2] x86/apic: x2apic write eoi msr notrace

[PATCH v7 0/2] x86/apic: x2apic write eoi msr notrace

[Wanpeng Li]

| RCU used illegally from idle CPU!
| rcu_scheduler_active = 1, debug_locks = 0
| RCU used illegally from extended quiescent state!
| no locks held by swapper/1/0.
| 
|  [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
|  [<ffffffff9d06f860>] native_write_msr+0x20/0x30
|  [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
|  [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
|  [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0

Reschedule interrupt may be called in cpu idle state. This causes lockdep
check warning above.

As Peterz pointed out:

| The thing is, many many smp_reschedule_interrupt() invocations don't
| actually execute anything much at all and are only send to tickle the
| return to user path (which does the actual preemption).
| 
| Having to do the whole irq_enter/irq_exit dance just for this unlikely
| debug case totally blows.

This patchset adds x2apic write eoi msr notrace to avoid the debug codes 
splash and reverts irq_enter/irq_exit dance to avoid to make a very frequent 
interrupt slower because of debug code.

v6 -> v7:
 * cleanup storage class should be at the beginning of the declaration

v5 -> v6:
 * split the patch 
 * don't duplicate the inline asm

v4 -> v5:
 * add notrace mark 

v3 -> v4: 
 * add notrace mark 

v2 -> v3:
 * revert irq_enter/irq_exit() since it is merged

v1 -> v2:
 * add write msr notrace to avoid debug codes splash instead of slowdown 
   a very frequent interrupt


Wanpeng Li (2):
  x86/msr: Add write msr notrace
  x86/apic: x2apic write eoi msr notrace

 arch/x86/include/asm/apic.h |  3 ++-
 arch/x86/include/asm/msr.h  | 14 +++++++++++++-
 arch/x86/kernel/apic/apic.c |  1 +
 arch/x86/kernel/kvm.c       |  4 ++--
 arch/x86/kernel/smp.c       |  2 --
 5 files changed, 18 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH v7 1/2] x86/msr: Add write msr notrace

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Add write msr notrace, it will be used by later patch.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/include/asm/msr.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index b5fee97..eec29a7 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -115,17 +115,29 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 }
 
 /* Can be uninlined because referenced by paravirt */
-notrace static inline void native_write_msr(unsigned int msr,
+static notrace inline void __native_write_msr_notrace(unsigned int msr,
 					    unsigned low, unsigned high)
 {
 	asm volatile("1: wrmsr\n"
 		     "2:\n"
 		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
 		     : : "c" (msr), "a"(low), "d" (high) : "memory");
+}
+
+/* Can be uninlined because referenced by paravirt */
+static notrace inline void native_write_msr(unsigned int msr,
+					    unsigned low, unsigned high)
+{
+	__native_write_msr_notrace(msr, low, high);
 	if (msr_tracepoint_active(__tracepoint_write_msr))
 		do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
 }
 
+static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned high)
+{
+	__native_write_msr_notrace(msr, low, high);
+}
+
 /* Can be uninlined because referenced by paravirt */
 notrace static inline int native_write_msr_safe(unsigned int msr,
 					unsigned low, unsigned high)
-- 
1.9.1

[PATCH v7 2/2] x86/apic: x2apic write eoi msr notrace

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

| RCU used illegally from idle CPU!
| rcu_scheduler_active = 1, debug_locks = 0
| RCU used illegally from extended quiescent state!
| no locks held by swapper/1/0.
| 
|  [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
|  [<ffffffff9d06f860>] native_write_msr+0x20/0x30
|  [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
|  [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
|  [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0

Reschedule interrupt may be called in cpu idle state. This causes lockdep
check warning above.

As Peterz pointed out:

| So now we're making a very frequent interrupt slower because of debug 
| code.
|
| The thing is, many many smp_reschedule_interrupt() invocations don't
| actually execute anything much at all and are only send to tickle the
| return to user path (which does the actual preemption).
| 
| Having to do the whole irq_enter/irq_exit dance just for this unlikely
| debug case totally blows.

This patch converts x2apic write eoi msr to notrace to avoid the debug 
codes splash and reverts irq_enter/irq_exit dance to avoid to make a very 
frequent interrupt slower because of debug code.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/include/asm/apic.h | 3 ++-
 arch/x86/kernel/apic/apic.c | 1 +
 arch/x86/kernel/kvm.c       | 4 ++--
 arch/x86/kernel/smp.c       | 2 --
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index f5aaf6c..a5a0bcf 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -196,7 +196,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v)
 
 static inline void native_apic_msr_eoi_write(u32 reg, u32 v)
 {
-	wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
+	wrmsr_notrace(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
 }
 
 static inline u32 native_apic_msr_read(u32 reg)
@@ -332,6 +332,7 @@ struct apic {
 	 * on write for EOI.
 	 */
 	void (*eoi_write)(u32 reg, u32 v);
+	void (*native_eoi_write)(u32 reg, u32 v);
 	u64 (*icr_read)(void);
 	void (*icr_write)(u32 low, u32 high);
 	void (*wait_icr_idle)(void);
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 88c657b..2686894 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2263,6 +2263,7 @@ void __init apic_set_eoi_write(void (*eoi_write)(u32 reg, u32 v))
 	for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
 		/* Should happen once for each apic */
 		WARN_ON((*drv)->eoi_write == eoi_write);
+		(*drv)->native_eoi_write = (*drv)->eoi_write;
 		(*drv)->eoi_write = eoi_write;
 	}
 }
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index edbbfc8..d230513 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -308,7 +308,7 @@ static void kvm_register_steal_time(void)
 
 static DEFINE_PER_CPU(unsigned long, kvm_apic_eoi) = KVM_PV_EOI_DISABLED;
 
-static void kvm_guest_apic_eoi_write(u32 reg, u32 val)
+static notrace void kvm_guest_apic_eoi_write(u32 reg, u32 val)
 {
 	/**
 	 * This relies on __test_and_clear_bit to modify the memory
@@ -319,7 +319,7 @@ static void kvm_guest_apic_eoi_write(u32 reg, u32 val)
 	 */
 	if (__test_and_clear_bit(KVM_PV_EOI_BIT, this_cpu_ptr(&kvm_apic_eoi)))
 		return;
-	apic_write(APIC_EOI, APIC_EOI_ACK);
+	apic->native_eoi_write(APIC_EOI, APIC_EOI_ACK);
 }
 
 static void kvm_guest_cpu_init(void)
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index c00cb64..68f8cc2 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -261,10 +261,8 @@ static inline void __smp_reschedule_interrupt(void)
 
 __visible void smp_reschedule_interrupt(struct pt_regs *regs)
 {
-	irq_enter();
 	ack_APIC_irq();
 	__smp_reschedule_interrupt();
-	irq_exit();
 	/*
 	 * KVM uses this interrupt to force a cpu out of guest mode
 	 */
-- 
1.9.1

Re: [PATCH v7 1/2] x86/msr: Add write msr notrace

[Borislav Petkov]

On Mon, Oct 31, 2016 at 09:37:01AM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Add write msr notrace, it will be used by later patch.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  arch/x86/include/asm/msr.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH v7 2/2] x86/apic: x2apic write eoi msr notrace

[Borislav Petkov]

On Mon, Oct 31, 2016 at 09:37:02AM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> | RCU used illegally from idle CPU!
> | rcu_scheduler_active = 1, debug_locks = 0
> | RCU used illegally from extended quiescent state!
> | no locks held by swapper/1/0.
> | 
> |  [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
> |  [<ffffffff9d06f860>] native_write_msr+0x20/0x30
> |  [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
> |  [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
> |  [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0

Please remove the text between [] and the offsets after "+..." - those
are useless in a commit message.

> Reschedule interrupt may be called in cpu idle state. This causes lockdep

s/cpu/CPU/

> check warning above.
> 
> As Peterz pointed out:
> 
> | So now we're making a very frequent interrupt slower because of debug 
> | code.
> |
> | The thing is, many many smp_reschedule_interrupt() invocations don't
> | actually execute anything much at all and are only send to tickle the

s/send/sent/

> | return to user path (which does the actual preemption).
> | 
> | Having to do the whole irq_enter/irq_exit dance just for this unlikely
> | debug case totally blows.
> 
> This patch converts x2apic write eoi msr to notrace to avoid the debug 
> codes splash and reverts irq_enter/irq_exit dance to avoid to make a very 
> frequent interrupt slower because of debug code.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH v7 0/2] x86/apic: x2apic write eoi msr notrace

[Borislav Petkov]

On Mon, Oct 31, 2016 at 09:37:00AM +0800, Wanpeng Li wrote:
> | RCU used illegally from idle CPU!
> | rcu_scheduler_active = 1, debug_locks = 0
> | RCU used illegally from extended quiescent state!
> | no locks held by swapper/1/0.
> | 
> |  [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
> |  [<ffffffff9d06f860>] native_write_msr+0x20/0x30
> |  [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
> |  [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
> |  [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0

Ontop of the two:

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 2 Nov 2016 19:35:22 +0100
Subject: [PATCH] x86/MSR: Cleanup/streamline MSR helpers

Make the MSR argument an unsigned int, both low and high u32, put
"notrace" last in the function signature. Reflow function signatures for
better readability.

Cleanup white space.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/msr.h | 40 +++++++++++++++++++++-------------------
 arch/x86/lib/msr.c         |  4 ++--
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 9b0a23236836..db0b90c3b03e 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -70,14 +70,14 @@ extern struct tracepoint __tracepoint_read_msr;
 extern struct tracepoint __tracepoint_write_msr;
 extern struct tracepoint __tracepoint_rdpmc;
 #define msr_tracepoint_active(t) static_key_false(&(t).key)
-extern void do_trace_write_msr(unsigned msr, u64 val, int failed);
-extern void do_trace_read_msr(unsigned msr, u64 val, int failed);
-extern void do_trace_rdpmc(unsigned msr, u64 val, int failed);
+extern void do_trace_write_msr(unsigned int msr, u64 val, int failed);
+extern void do_trace_read_msr(unsigned int msr, u64 val, int failed);
+extern void do_trace_rdpmc(unsigned int msr, u64 val, int failed);
 #else
 #define msr_tracepoint_active(t) false
-static inline void do_trace_write_msr(unsigned msr, u64 val, int failed) {}
-static inline void do_trace_read_msr(unsigned msr, u64 val, int failed) {}
-static inline void do_trace_rdpmc(unsigned msr, u64 val, int failed) {}
+static inline void do_trace_write_msr(unsigned int msr, u64 val, int failed) {}
+static inline void do_trace_read_msr(unsigned int msr, u64 val, int failed) {}
+static inline void do_trace_rdpmc(unsigned int msr, u64 val, int failed) {}
 #endif
 
 static inline unsigned long long native_read_msr(unsigned int msr)
@@ -115,8 +115,8 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 }
 
 /* Can be uninlined because referenced by paravirt */
-static notrace inline void __native_write_msr_notrace(unsigned int msr,
-					    unsigned low, unsigned high)
+static inline void notrace
+__native_write_msr_notrace(unsigned int msr, u32 low, u32 high)
 {
 	asm volatile("1: wrmsr\n"
 		     "2:\n"
@@ -125,24 +125,26 @@ static notrace inline void __native_write_msr_notrace(unsigned int msr,
 }
 
 /* Can be uninlined because referenced by paravirt */
-static notrace inline void native_write_msr(unsigned int msr,
-					    unsigned low, unsigned high)
+static inline void notrace
+native_write_msr(unsigned int msr, u32 low, u32 high)
 {
 	__native_write_msr_notrace(msr, low, high);
 	if (msr_tracepoint_active(__tracepoint_write_msr))
 		do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
 }
 
-static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned high)
+static inline void
+wrmsr_notrace(unsigned int msr, u32 low, u32 high)
 {
 	__native_write_msr_notrace(msr, low, high);
 }
 
 /* Can be uninlined because referenced by paravirt */
-notrace static inline int native_write_msr_safe(unsigned int msr,
-					unsigned low, unsigned high)
+static inline int notrace
+native_write_msr_safe(unsigned int msr, u32 low, u32 high)
 {
 	int err;
+
 	asm volatile("2: wrmsr ; xor %[err],%[err]\n"
 		     "1:\n\t"
 		     ".section .fixup,\"ax\"\n\t"
@@ -235,7 +237,7 @@ do {								\
 	(void)((high) = (u32)(__val >> 32));			\
 } while (0)
 
-static inline void wrmsr(unsigned msr, unsigned low, unsigned high)
+static inline void wrmsr(unsigned int msr, u32 low, u32 high)
 {
 	native_write_msr(msr, low, high);
 }
@@ -243,13 +245,13 @@ static inline void wrmsr(unsigned msr, unsigned low, unsigned high)
 #define rdmsrl(msr, val)			\
 	((val) = native_read_msr((msr)))
 
-static inline void wrmsrl(unsigned msr, u64 val)
+static inline void wrmsrl(unsigned int msr, u64 val)
 {
 	native_write_msr(msr, (u32)(val & 0xffffffffULL), (u32)(val >> 32));
 }
 
 /* wrmsr with exception handling */
-static inline int wrmsr_safe(unsigned msr, unsigned low, unsigned high)
+static inline int wrmsr_safe(unsigned int msr, u32 low, u32 high)
 {
 	return native_write_msr_safe(msr, low, high);
 }
@@ -264,7 +266,7 @@ static inline int wrmsr_safe(unsigned msr, unsigned low, unsigned high)
 	__err;							\
 })
 
-static inline int rdmsrl_safe(unsigned msr, unsigned long long *p)
+static inline int rdmsrl_safe(unsigned int msr, unsigned long long *p)
 {
 	int err;
 
@@ -337,12 +339,12 @@ static inline int wrmsrl_on_cpu(unsigned int cpu, u32 msr_no, u64 q)
 static inline void rdmsr_on_cpus(const struct cpumask *m, u32 msr_no,
 				struct msr *msrs)
 {
-       rdmsr_on_cpu(0, msr_no, &(msrs[0].l), &(msrs[0].h));
+	rdmsr_on_cpu(0, msr_no, &(msrs[0].l), &(msrs[0].h));
 }
 static inline void wrmsr_on_cpus(const struct cpumask *m, u32 msr_no,
 				struct msr *msrs)
 {
-       wrmsr_on_cpu(0, msr_no, msrs[0].l, msrs[0].h);
+	wrmsr_on_cpu(0, msr_no, msrs[0].l, msrs[0].h);
 }
 static inline int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no,
 				    u32 *l, u32 *h)
diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index d1dee753b949..07764255b611 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -113,14 +113,14 @@ int msr_clear_bit(u32 msr, u8 bit)
 }
 
 #ifdef CONFIG_TRACEPOINTS
-void do_trace_write_msr(unsigned msr, u64 val, int failed)
+void do_trace_write_msr(unsigned int msr, u64 val, int failed)
 {
 	trace_write_msr(msr, val, failed);
 }
 EXPORT_SYMBOL(do_trace_write_msr);
 EXPORT_TRACEPOINT_SYMBOL(write_msr);
 
-void do_trace_read_msr(unsigned msr, u64 val, int failed)
+void do_trace_read_msr(unsigned int msr, u64 val, int failed)
 {
 	trace_read_msr(msr, val, failed);
 }
-- 
2.10.0

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH v7 2/2] x86/apic: x2apic write eoi msr notrace

[Wanpeng Li]

2016-11-03 5:10 GMT+08:00 Borislav Petkov <bp@alien8.de>:
> On Mon, Oct 31, 2016 at 09:37:02AM +0800, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> | RCU used illegally from idle CPU!
>> | rcu_scheduler_active = 1, debug_locks = 0
>> | RCU used illegally from extended quiescent state!
>> | no locks held by swapper/1/0.
>> |
>> |  [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
>> |  [<ffffffff9d06f860>] native_write_msr+0x20/0x30
>> |  [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
>> |  [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
>> |  [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
>
> Please remove the text between [] and the offsets after "+..." - those
> are useless in a commit message.
>
>> Reschedule interrupt may be called in cpu idle state. This causes lockdep
>
> s/cpu/CPU/
>
>> check warning above.
>>
>> As Peterz pointed out:
>>
>> | So now we're making a very frequent interrupt slower because of debug
>> | code.
>> |
>> | The thing is, many many smp_reschedule_interrupt() invocations don't
>> | actually execute anything much at all and are only send to tickle the
>
> s/send/sent/

I will send a new version to clean it up.

Regards,
Wanpeng Li