[PATCH 0/3] context_tracking: streamline code, avoid IRQ save/restore

[PATCH 0/3] context_tracking: streamline code, avoid IRQ save/restore

[Paolo Bonzini]

The first two of these patches were posted last February, the last one
is new.  Rik's old measurements were that it shaved around .3 microseconds
on each iteration of his KVM benchmark.

I guess three days before the start of the merge window is not
the best time to post patches.  However, I brought this series up at
kernel summit yesterday, and Andy's cleanups actually makes it trivial
to apply this to syscall entry.  So here it is, perhaps it's worth it.

Assuming it works, of course, because this is compile-tested only. :)

Paolo

Paolo Bonzini (3):
  context_tracking: remove duplicate enabled check
  context_tracking: avoid irq_save/irq_restore on guest entry and exit
  x86: context_tracking: avoid irq_save/irq_restore on kernel entry and exit

 arch/x86/entry/common.c          |  4 +-
 include/linux/context_tracking.h | 50 +++++++++++++++++--------
 kernel/context_tracking.c        | 80 ++++++++++++++++++++--------------------
 3 files changed, 76 insertions(+), 58 deletions(-)

-- 
2.5.0

[PATCH 1/3] context_tracking: remove duplicate enabled check

[Paolo Bonzini]

All calls to context_tracking_enter and context_tracking_exit
are already checking context_tracking_is_enabled, except the
context_tracking_user_enter and context_tracking_user_exit
functions left in for the benefit of assembly calls.

Pull the check up to those functions, by making them simple
wrappers around the user_enter and user_exit inline functions.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/linux/context_tracking.h |  4 ++--
 kernel/context_tracking.c        | 16 ++--------------
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index 008fc67d0d96..6ef136ff0897 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -18,13 +18,13 @@ extern void context_tracking_user_exit(void);
 static inline void user_enter(void)
 {
 	if (context_tracking_is_enabled())
-		context_tracking_user_enter();
+		context_tracking_enter(CONTEXT_USER);
 
 }
 static inline void user_exit(void)
 {
 	if (context_tracking_is_enabled())
-		context_tracking_user_exit();
+		context_tracking_exit(CONTEXT_USER);
 }
 
 static inline enum ctx_state exception_enter(void)
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 0a495ab35bc7..6d4c6ce21275 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -63,15 +63,6 @@ void context_tracking_enter(enum ctx_state state)
 	unsigned long flags;
 
 	/*
-	 * Repeat the user_enter() check here because some archs may be calling
-	 * this from asm and if no CPU needs context tracking, they shouldn't
-	 * go further. Repeat the check here until they support the inline static
-	 * key check.
-	 */
-	if (!context_tracking_is_enabled())
-		return;
-
-	/*
 	 * Some contexts may involve an exception occuring in an irq,
 	 * leading to that nesting:
 	 * rcu_irq_enter() rcu_user_exit() rcu_user_exit() rcu_irq_exit()
@@ -128,7 +119,7 @@ EXPORT_SYMBOL_GPL(context_tracking_enter);
 
 void context_tracking_user_enter(void)
 {
-	context_tracking_enter(CONTEXT_USER);
+	user_enter();
 }
 NOKPROBE_SYMBOL(context_tracking_user_enter);
 
@@ -148,9 +139,6 @@ void context_tracking_exit(enum ctx_state state)
 {
 	unsigned long flags;
 
-	if (!context_tracking_is_enabled())
-		return;
-
 	if (in_interrupt())
 		return;
 
@@ -181,7 +169,7 @@ EXPORT_SYMBOL_GPL(context_tracking_exit);
 
 void context_tracking_user_exit(void)
 {
-	context_tracking_exit(CONTEXT_USER);
+	user_exit();
 }
 NOKPROBE_SYMBOL(context_tracking_user_exit);
 
-- 
2.5.0

Re: [PATCH 1/3] context_tracking: remove duplicate enabled check

[Andy Lutomirski]

On Tue, Oct 27, 2015 at 6:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> All calls to context_tracking_enter and context_tracking_exit
> are already checking context_tracking_is_enabled, except the
> context_tracking_user_enter and context_tracking_user_exit
> functions left in for the benefit of assembly calls.
>
> Pull the check up to those functions, by making them simple
> wrappers around the user_enter and user_exit inline functions.

This makes my brain hurt.  Assuming that this survives a boot with
CONFIG_CONTEXT_TRACKING_FORCE=y and CONFIG_PROVE_LOCKING=y (with the
implied CONFIG_PROVE_RCU), then:

Acked-by: Andy Lutomirski <luto@kernel.org>

Re: [PATCH 1/3] context_tracking: remove duplicate enabled check

[Paolo Bonzini]

On 28/10/2015 06:19, Andy Lutomirski wrote:
> On Tue, Oct 27, 2015 at 6:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> All calls to context_tracking_enter and context_tracking_exit
>> are already checking context_tracking_is_enabled, except the
>> context_tracking_user_enter and context_tracking_user_exit
>> functions left in for the benefit of assembly calls.
>>
>> Pull the check up to those functions, by making them simple
>> wrappers around the user_enter and user_exit inline functions.
> 
> This makes my brain hurt.  Assuming that this survives a boot with
> CONFIG_CONTEXT_TRACKING_FORCE=y and CONFIG_PROVE_LOCKING=y (with the
> implied CONFIG_PROVE_RCU), then:
> 
> Acked-by: Andy Lutomirski <luto@kernel.org>

Tested now.

Paolo

Re: [PATCH 1/3] context_tracking: remove duplicate enabled check

[Rik van Riel]

On 10/27/2015 09:39 PM, Paolo Bonzini wrote:
> All calls to context_tracking_enter and context_tracking_exit
> are already checking context_tracking_is_enabled, except the
> context_tracking_user_enter and context_tracking_user_exit
> functions left in for the benefit of assembly calls.
> 
> Pull the check up to those functions, by making them simple
> wrappers around the user_enter and user_exit inline functions.
> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Rik van Riel <riel@redhat.com>
Tested-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

[PATCH 2/3] context_tracking: avoid irq_save/irq_restore on guest entry and exit

[Paolo Bonzini]

guest_enter and guest_exit must be called with interrupts disabled,
since they take the vtime_seqlock with write_seq{lock,unlock}.
Therefore, it is not necessary to check for exceptions, nor to
save/restore the IRQ state, when context tracking functions are
called by guest_enter and guest_exit.

Split the body of context_tracking_entry and context_tracking_exit
out to __-prefixed functions, and use them from KVM.

Rik van Riel has measured this to speed up a tight vmentry/vmexit
loop by about 2%.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/linux/context_tracking.h |  8 +++--
 kernel/context_tracking.c        | 64 ++++++++++++++++++++++++----------------
 2 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index 6ef136ff0897..68b575afe5f5 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -10,6 +10,10 @@
 #ifdef CONFIG_CONTEXT_TRACKING
 extern void context_tracking_cpu_set(int cpu);
 
+/* Called with interrupts disabled.  */
+extern void __context_tracking_enter(enum ctx_state state);
+extern void __context_tracking_exit(enum ctx_state state);
+
 extern void context_tracking_enter(enum ctx_state state);
 extern void context_tracking_exit(enum ctx_state state);
 extern void context_tracking_user_enter(void);
@@ -88,13 +92,13 @@ static inline void guest_enter(void)
 		current->flags |= PF_VCPU;
 
 	if (context_tracking_is_enabled())
-		context_tracking_enter(CONTEXT_GUEST);
+		__context_tracking_enter(CONTEXT_GUEST);
 }
 
 static inline void guest_exit(void)
 {
 	if (context_tracking_is_enabled())
-		context_tracking_exit(CONTEXT_GUEST);
+		__context_tracking_exit(CONTEXT_GUEST);
 
 	if (vtime_accounting_enabled())
 		vtime_guest_exit(current);
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 6d4c6ce21275..d8560ee3bab7 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -58,27 +58,13 @@ static void context_tracking_recursion_exit(void)
  * instructions to execute won't use any RCU read side critical section
  * because this function sets RCU in extended quiescent state.
  */
-void context_tracking_enter(enum ctx_state state)
+void __context_tracking_enter(enum ctx_state state)
 {
-	unsigned long flags;
-
-	/*
-	 * Some contexts may involve an exception occuring in an irq,
-	 * leading to that nesting:
-	 * rcu_irq_enter() rcu_user_exit() rcu_user_exit() rcu_irq_exit()
-	 * This would mess up the dyntick_nesting count though. And rcu_irq_*()
-	 * helpers are enough to protect RCU uses inside the exception. So
-	 * just return immediately if we detect we are in an IRQ.
-	 */
-	if (in_interrupt())
-		return;
-
 	/* Kernel threads aren't supposed to go to userspace */
 	WARN_ON_ONCE(!current->mm);
 
-	local_irq_save(flags);
 	if (!context_tracking_recursion_enter())
-		goto out_irq_restore;
+		return;
 
 	if ( __this_cpu_read(context_tracking.state) != state) {
 		if (__this_cpu_read(context_tracking.active)) {
@@ -111,7 +97,27 @@ void context_tracking_enter(enum ctx_state state)
 		__this_cpu_write(context_tracking.state, state);
 	}
 	context_tracking_recursion_exit();
-out_irq_restore:
+}
+NOKPROBE_SYMBOL(__context_tracking_enter);
+EXPORT_SYMBOL_GPL(__context_tracking_enter);
+
+void context_tracking_enter(enum ctx_state state)
+{
+	unsigned long flags;
+
+	/*
+	 * Some contexts may involve an exception occuring in an irq,
+	 * leading to that nesting:
+	 * rcu_irq_enter() rcu_user_exit() rcu_user_exit() rcu_irq_exit()
+	 * This would mess up the dyntick_nesting count though. And rcu_irq_*()
+	 * helpers are enough to protect RCU uses inside the exception. So
+	 * just return immediately if we detect we are in an IRQ.
+	 */
+	if (in_interrupt())
+		return;
+
+	local_irq_save(flags);
+	__context_tracking_enter(state);
 	local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(context_tracking_enter);
@@ -135,16 +141,10 @@ NOKPROBE_SYMBOL(context_tracking_user_enter);
  * This call supports re-entrancy. This way it can be called from any exception
  * handler without needing to know if we came from userspace or not.
  */
-void context_tracking_exit(enum ctx_state state)
+void __context_tracking_exit(enum ctx_state state)
 {
-	unsigned long flags;
-
-	if (in_interrupt())
-		return;
-
-	local_irq_save(flags);
 	if (!context_tracking_recursion_enter())
-		goto out_irq_restore;
+		return;
 
 	if (__this_cpu_read(context_tracking.state) == state) {
 		if (__this_cpu_read(context_tracking.active)) {
@@ -161,7 +161,19 @@ void context_tracking_exit(enum ctx_state state)
 		__this_cpu_write(context_tracking.state, CONTEXT_KERNEL);
 	}
 	context_tracking_recursion_exit();
-out_irq_restore:
+}
+NOKPROBE_SYMBOL(__context_tracking_exit);
+EXPORT_SYMBOL_GPL(__context_tracking_exit);
+
+void context_tracking_exit(enum ctx_state state)
+{
+	unsigned long flags;
+
+	if (in_interrupt())
+		return;
+
+	local_irq_save(flags);
+	__context_tracking_exit(state);
 	local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(context_tracking_exit);
-- 
2.5.0

Re: [PATCH 2/3] context_tracking: avoid irq_save/irq_restore on guest entry and exit

[Andy Lutomirski]

On Tue, Oct 27, 2015 at 6:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> guest_enter and guest_exit must be called with interrupts disabled,
> since they take the vtime_seqlock with write_seq{lock,unlock}.
> Therefore, it is not necessary to check for exceptions, nor to
> save/restore the IRQ state, when context tracking functions are
> called by guest_enter and guest_exit.
>
> Split the body of context_tracking_entry and context_tracking_exit
> out to __-prefixed functions, and use them from KVM.
>
> Rik van Riel has measured this to speed up a tight vmentry/vmexit
> loop by about 2%.

Looks generally sensible.  I'm not familiar enough with the code to
call it reviewed-by while sitting on the airport shuttle.

--Andy

Re: [PATCH 2/3] context_tracking: avoid irq_save/irq_restore on guest entry and exit

[Rik van Riel]

On 10/27/2015 09:39 PM, Paolo Bonzini wrote:
> guest_enter and guest_exit must be called with interrupts disabled,
> since they take the vtime_seqlock with write_seq{lock,unlock}.
> Therefore, it is not necessary to check for exceptions, nor to
> save/restore the IRQ state, when context tracking functions are
> called by guest_enter and guest_exit.
> 
> Split the body of context_tracking_entry and context_tracking_exit
> out to __-prefixed functions, and use them from KVM.
> 
> Rik van Riel has measured this to speed up a tight vmentry/vmexit
> loop by about 2%.
> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Rik van Riel <riel@redhat.com>
Tested-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

[PATCH 3/3] x86: context_tracking: avoid irq_save/irq_restore on kernel entry and exit

[Paolo Bonzini]

x86 always calls user_enter and user_exit with interrupt disabled.
Therefore, it is not necessary to check for exceptions, nor to
save/restore the IRQ state, when context tracking functions are
called by guest_enter and guest_exit.

Use the previously introduced __context_tracking_entry and
__context_tracking_exit.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/entry/common.c          |  4 ++--
 include/linux/context_tracking.h | 42 ++++++++++++++++++++++++++--------------
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 80dcc9261ca3..ba707c6da818 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -33,7 +33,7 @@
 __visible void enter_from_user_mode(void)
 {
 	CT_WARN_ON(ct_state() != CONTEXT_USER);
-	user_exit();
+	__user_exit();
 }
 #endif
 
@@ -262,7 +262,7 @@ __visible void prepare_exit_to_usermode(struct pt_regs *regs)
 		local_irq_disable();
 	}
 
-	user_enter();
+	__user_enter();
 }
 
 /*
diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index 68b575afe5f5..d36a5c8085b7 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -19,18 +19,6 @@ extern void context_tracking_exit(enum ctx_state state);
 extern void context_tracking_user_enter(void);
 extern void context_tracking_user_exit(void);
 
-static inline void user_enter(void)
-{
-	if (context_tracking_is_enabled())
-		context_tracking_enter(CONTEXT_USER);
-
-}
-static inline void user_exit(void)
-{
-	if (context_tracking_is_enabled())
-		context_tracking_exit(CONTEXT_USER);
-}
-
 static inline enum ctx_state exception_enter(void)
 {
 	enum ctx_state prev_ctx;
@@ -67,13 +55,39 @@ static inline enum ctx_state ct_state(void)
 		this_cpu_read(context_tracking.state) : CONTEXT_DISABLED;
 }
 #else
-static inline void user_enter(void) { }
-static inline void user_exit(void) { }
+static inline void __context_tracking_enter(enum ctx_state state) { }
+static inline void __context_tracking_exit(enum ctx_state state) { }
+static inline void context_tracking_enter(enum ctx_state state) { }
+static inline void context_tracking_exit(enum ctx_state state) { }
 static inline enum ctx_state exception_enter(void) { return 0; }
 static inline void exception_exit(enum ctx_state prev_ctx) { }
 static inline enum ctx_state ct_state(void) { return CONTEXT_DISABLED; }
 #endif /* !CONFIG_CONTEXT_TRACKING */
 
+static inline void __user_enter(void)
+{
+	if (context_tracking_is_enabled())
+		__context_tracking_enter(CONTEXT_USER);
+
+}
+static inline void __user_exit(void)
+{
+	if (context_tracking_is_enabled())
+		__context_tracking_exit(CONTEXT_USER);
+}
+
+static inline void user_enter(void)
+{
+	if (context_tracking_is_enabled())
+		context_tracking_enter(CONTEXT_USER);
+
+}
+static inline void user_exit(void)
+{
+	if (context_tracking_is_enabled())
+		context_tracking_exit(CONTEXT_USER);
+}
+
 #define CT_WARN_ON(cond) WARN_ON(context_tracking_is_enabled() && (cond))
 
 #ifdef CONFIG_CONTEXT_TRACKING_FORCE
-- 
2.5.0

Re: [PATCH 3/3] x86: context_tracking: avoid irq_save/irq_restore on kernel entry and exit

[Andy Lutomirski]

On Tue, Oct 27, 2015 at 6:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> x86 always calls user_enter and user_exit with interrupt disabled.
> Therefore, it is not necessary to check for exceptions, nor to
> save/restore the IRQ state, when context tracking functions are
> called by guest_enter and guest_exit.
>
> Use the previously introduced __context_tracking_entry and
> __context_tracking_exit.

x86 isn't ready for this yet.  We could do a quick-and-dirty fix with
explicit IRQs-on-and-off much protected by the static key, or we could
just wait until I finish the syscall cleanup.  I favor the latter, but
you're all welcome to do the former and I'll review it.

BTW, Frederic, I have a static key patch coming that I think you'll
like.  I'll send it tomorrow once I'm in front of a real computer.

--Andy

Re: [PATCH 3/3] x86: context_tracking: avoid irq_save/irq_restore on kernel entry and exit

[Paolo Bonzini]

On 28/10/2015 06:22, Andy Lutomirski wrote:
>> > called by guest_enter and guest_exit.
>> >
>> > Use the previously introduced __context_tracking_entry and
>> > __context_tracking_exit.
> x86 isn't ready for this yet.  We could do a quick-and-dirty fix with
> explicit IRQs-on-and-off much protected by the static key, or we could
> just wait until I finish the syscall cleanup.  I favor the latter, but
> you're all welcome to do the former and I'll review it.

Or we could just do save/restore for the only call that doesn't ensure
that interrupts are disabled (syscall_trace_phase1 or whatever it's called).

But two days from the merge window, I also favor waiting until 4.5.

Paolo

Re: [PATCH 3/3] x86: context_tracking: avoid irq_save/irq_restore on kernel entry and exit

[Andy Lutomirski]

On Tue, Oct 27, 2015 at 10:22 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Oct 27, 2015 at 6:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> x86 always calls user_enter and user_exit with interrupt disabled.
>> Therefore, it is not necessary to check for exceptions, nor to
>> save/restore the IRQ state, when context tracking functions are
>> called by guest_enter and guest_exit.
>>
>> Use the previously introduced __context_tracking_entry and
>> __context_tracking_exit.
>
> x86 isn't ready for this yet.  We could do a quick-and-dirty fix with
> explicit IRQs-on-and-off much protected by the static key, or we could
> just wait until I finish the syscall cleanup.  I favor the latter, but
> you're all welcome to do the former and I'll review it.
>

Once Linus pulls tip:x86/asm, x86 should be ready for this.  Want to try again?

--Andy

Re: [PATCH 3/3] x86: context_tracking: avoid irq_save/irq_restore on kernel entry and exit

[Paolo Bonzini]

On 14/03/2016 20:23, Andy Lutomirski wrote:
> On Tue, Oct 27, 2015 at 10:22 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Tue, Oct 27, 2015 at 6:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> x86 always calls user_enter and user_exit with interrupt disabled.
>>> Therefore, it is not necessary to check for exceptions, nor to
>>> save/restore the IRQ state, when context tracking functions are
>>> called by guest_enter and guest_exit.
>>>
>>> Use the previously introduced __context_tracking_entry and
>>> __context_tracking_exit.
>>
>> x86 isn't ready for this yet.  We could do a quick-and-dirty fix with
>> explicit IRQs-on-and-off much protected by the static key, or we could
>> just wait until I finish the syscall cleanup.  I favor the latter, but
>> you're all welcome to do the former and I'll review it.
>>
> 
> Once Linus pulls tip:x86/asm, x86 should be ready for this.  Want to try again?

Yup, it's on my todo list. :)

Paolo

Re: [PATCH 0/3] context_tracking: streamline code, avoid IRQ save/restore

[Paolo Bonzini]

On 28/10/2015 02:39, Paolo Bonzini wrote:
> The first two of these patches were posted last February, the last one
> is new.  Rik's old measurements were that it shaved around .3 microseconds
> on each iteration of his KVM benchmark.
> 
> I guess three days before the start of the merge window is not
> the best time to post patches.  However, I brought this series up at
> kernel summit yesterday, and Andy's cleanups actually makes it trivial
> to apply this to syscall entry.  So here it is, perhaps it's worth it.
> 
> Assuming it works, of course, because this is compile-tested only. :)

Heh, Andy said it doesn't. :)

However, we could still merge the first two patches for 4.4 since they
have been tested.  I can even take them myself if I get Acked-by.  The
third one will have to wait for 4.5.

Thanks,

Paolo