[PATCH 0/2] x86/entry: speed up context-tracking system calls by 150 clock cycles

[PATCH 0/2] x86/entry: speed up context-tracking system calls by 150 clock cycles

[Paolo Bonzini]

Two simple optimizations to the system call entry/exit code.
The first matches what commit d0e536d8939 ("context_tracking:
avoid irq_save/irq_restore on guest entry and exit", 2015-10-28)
did for guest entry and exit.  The second is just adding an
inline annotation.

Thanks,

Paolo

Paolo Bonzini (2):
  x86/entry: Avoid interrupt flag save and restore
  x86/entry: Inline enter_from_user_mode

 arch/x86/entry/common.c          |  6 +++---
 include/linux/context_tracking.h | 15 +++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

-- 
2.4.3

[PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Paolo Bonzini]

Thanks to all the work that was done by Andy Lutomirski and others,
enter_from_user_mode and prepare_exit_to_usermode are now called only with
interrupts disabled.  Let's provide them a version of user_enter/user_exit
that skips saving and restoring the interrupt flag.

On an AMD-based machine I tested this patch on, with force-enabled
context tracking, the speed-up in system calls was 90 clock cycles or 6%,
measured with the following simple benchmark:

    #include <sys/signal.h>
    #include <time.h>
    #include <unistd.h>
    #include <stdio.h>

    unsigned long rdtsc()
    {
        unsigned long result;
        asm volatile("rdtsc; shl $32, %%rdx; mov %%eax, %%eax\n"
                     "or %%rdx, %%rax" : "=a" (result) : : "rdx");
        return result;
    }

    int main()
    {
        unsigned long tsc1, tsc2;
        int pid = getpid();
        int i;

        tsc1 = rdtsc();
        for (i = 0; i < 100000000; i++)
            kill(pid, SIGWINCH);
        tsc2 = rdtsc();

        printf("%ld\n", tsc2 - tsc1);
    }

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/entry/common.c          |  4 ++--
 include/linux/context_tracking.h | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index e79d93d..946bc1a 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -43,7 +43,7 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
 __visible void enter_from_user_mode(void)
 {
 	CT_WARN_ON(ct_state() != CONTEXT_USER);
-	user_exit();
+	__user_exit();
 }
 #else
 static inline void enter_from_user_mode(void) {}
@@ -274,7 +274,7 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
 	ti->status &= ~TS_COMPAT;
 #endif
 
-	user_enter();
+	__user_enter();
 }
 
 #define SYSCALL_EXIT_WORK_FLAGS				\
diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index d259274..81a1fbc 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -31,6 +31,19 @@ static inline void user_exit(void)
 		context_tracking_exit(CONTEXT_USER);
 }
 
+/* Called with interrupts disabled.  */
+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;
@@ -69,6 +82,8 @@ static inline enum ctx_state ct_state(void)
 #else
 static inline void user_enter(void) { }
 static inline void user_exit(void) { }
+static inline void __user_enter(void) { }
+static inline void __user_exit(void) { }
 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; }
-- 
2.4.3

[PATCH 2/2] x86/entry: Inline enter_from_user_mode

[Paolo Bonzini]

This matches what is already done for prepare_exit_to_usermode,
and saves about 60 clock cycles (4% speedup) with the benchmark
in the previous commit message.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/entry/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 946bc1a..582bbc8 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -40,7 +40,7 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
 
 #ifdef CONFIG_CONTEXT_TRACKING
 /* Called on entry from user mode with IRQs off. */
-__visible void enter_from_user_mode(void)
+__visible inline void enter_from_user_mode(void)
 {
 	CT_WARN_ON(ct_state() != CONTEXT_USER);
 	__user_exit();
-- 
2.4.3

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

On Mon, 2016-05-30 at 14:30 +0200, Paolo Bonzini wrote:
> Thanks to all the work that was done by Andy Lutomirski and others,
> enter_from_user_mode and prepare_exit_to_usermode are now called only
> with
> interrupts disabled.  Let's provide them a version of
> user_enter/user_exit
> that skips saving and restoring the interrupt flag.
> 
> On an AMD-based machine I tested this patch on, with force-enabled
> context tracking, the speed-up in system calls was 90 clock cycles or
> 6%,
> measured with the following simple benchmark:
> 

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

-- 
All Rights Reversed.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] x86/entry: Inline enter_from_user_mode

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Mon, 2016-05-30 at 14:30 +0200, Paolo Bonzini wrote:
> This matches what is already done for prepare_exit_to_usermode,
> and saves about 60 clock cycles (4% speedup) with the benchmark
> in the previous commit message.
> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@kernel.org.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All Rights Reversed.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Andy Lutomirski]

On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>
> Thanks to all the work that was done by Andy Lutomirski and others,
> enter_from_user_mode and prepare_exit_to_usermode are now called only with
> interrupts disabled.  Let's provide them a version of user_enter/user_exit
> that skips saving and restoring the interrupt flag.

> +/* Called with interrupts disabled.  */
> +static inline void __user_enter(void)
> +{
> +       if (context_tracking_is_enabled())
> +               __context_tracking_enter(CONTEXT_USER);
> +
> +}

Would user_enter_irqs_off be a better name?

--Andy

Re: [PATCH 2/2] x86/entry: Inline enter_from_user_mode

[Andy Lutomirski]

On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>
> This matches what is already done for prepare_exit_to_usermode,
> and saves about 60 clock cycles (4% speedup) with the benchmark
> in the previous commit message.
>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@kernel.org.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/entry/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 946bc1a..582bbc8 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -40,7 +40,7 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
>
>  #ifdef CONFIG_CONTEXT_TRACKING
>  /* Called on entry from user mode with IRQs off. */
> -__visible void enter_from_user_mode(void)
> +__visible inline void enter_from_user_mode(void)
>  {
>         CT_WARN_ON(ct_state() != CONTEXT_USER);
>         __user_exit();

I wonder if an extern inline *declaration* is needed as well in this C
file.  At least C99 suggests it is.  Maybe __visible is sufficient to
force an external definition to be emitted.

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Paolo Bonzini]

On 04/06/2016 07:07, Andy Lutomirski wrote:
> On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>>
>> Thanks to all the work that was done by Andy Lutomirski and others,
>> enter_from_user_mode and prepare_exit_to_usermode are now called only with
>> interrupts disabled.  Let's provide them a version of user_enter/user_exit
>> that skips saving and restoring the interrupt flag.
> 
>> +/* Called with interrupts disabled.  */
>> +static inline void __user_enter(void)
>> +{
>> +       if (context_tracking_is_enabled())
>> +               __context_tracking_enter(CONTEXT_USER);
>> +
>> +}
> 
> Would user_enter_irqs_off be a better name?

I'm just mimicking __context_tracking_enter and vs.
context_tracking_enter.  So it is at least consistent with those functions.

The guest ones are not quite as consistent.  I can fix that later,
there's no reason also to have guest context tracking split between
include/linux/context_tracking.h and include/linux/kvm_host.h.

Thanks,

Paolo

Re: [PATCH 2/2] x86/entry: Inline enter_from_user_mode

[Paolo Bonzini]

On 04/06/2016 07:08, Andy Lutomirski wrote:
> On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>>
>> This matches what is already done for prepare_exit_to_usermode,
>> and saves about 60 clock cycles (4% speedup) with the benchmark
>> in the previous commit message.
>>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Rik van Riel <riel@redhat.com>
>> Cc: H. Peter Anvin <hpa@zytor.com>
>> Cc: Ingo Molnar <mingo@kernel.org.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  arch/x86/entry/common.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>> index 946bc1a..582bbc8 100644
>> --- a/arch/x86/entry/common.c
>> +++ b/arch/x86/entry/common.c
>> @@ -40,7 +40,7 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
>>
>>  #ifdef CONFIG_CONTEXT_TRACKING
>>  /* Called on entry from user mode with IRQs off. */
>> -__visible void enter_from_user_mode(void)
>> +__visible inline void enter_from_user_mode(void)
>>  {
>>         CT_WARN_ON(ct_state() != CONTEXT_USER);
>>         __user_exit();
> 
> I wonder if an extern inline *declaration* is needed as well in this C
> file.  At least C99 suggests it is.  Maybe __visible is sufficient to
> force an external definition to be emitted.

An extern inline declaration is not needed because the kernel uses
-std=gnu89 (or, if you prefer, because prepare_exit_to_usermode didn't
have one :)).

It's awesomely perverted:

  __attribute__((externally_visible)) inline void f(void) {}

  inline void g(void) {}

  extern inline void h(void);
  extern inline void h(void) {}

  inline void i(void);
  inline void i(void) {}

  extern inline void j(void);
  inline void j(void) {}

This patch (and the preexisting prepare_exit_to_usermode code) are
equivalent to "f".

Compile the above file with "--std=gnu89" or "--std=gnu99
-fgnu89-inline" and f/g/i/j are emitted.

Compile it with "--std=gnu99 -fno-gnu89-inline" and h/j is emitted.

Yes, the standard is _almost exactly_ the opposite of the preexisting
GCC implementation.  The only case which achieves the same effect is
when declarations are "extern inline" and definitions must always be
"inline".  Or of course just use "static inline".  At least it's
decently documented in the GCC info documentation.

Thanks,

Paolo

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 04/06/2016 07:07, Andy Lutomirski wrote:
> > On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
> >>
> >> Thanks to all the work that was done by Andy Lutomirski and others,
> >> enter_from_user_mode and prepare_exit_to_usermode are now called only with
> >> interrupts disabled.  Let's provide them a version of user_enter/user_exit
> >> that skips saving and restoring the interrupt flag.
> > 
> >> +/* Called with interrupts disabled.  */
> >> +static inline void __user_enter(void)
> >> +{
> >> +       if (context_tracking_is_enabled())
> >> +               __context_tracking_enter(CONTEXT_USER);
> >> +
> >> +}
> > 
> > Would user_enter_irqs_off be a better name?
> 
> I'm just mimicking __context_tracking_enter and vs.
> context_tracking_enter.  So it is at least consistent with those functions.
> 
> The guest ones are not quite as consistent.  I can fix that later,
> there's no reason also to have guest context tracking split between
> include/linux/context_tracking.h and include/linux/kvm_host.h.

Could we please first do the cleanups before complicating the code and applying 
more substantial changes?

Doing cleanups first makes it easier to review the substantial patches as well, so 
it's a win-win.

Thanks,

	Ingo

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Paolo Bonzini]

On 08/06/2016 14:16, Ingo Molnar wrote:
> > The guest ones are not quite as consistent.  I can fix that later,
> > there's no reason also to have guest context tracking split between
> > include/linux/context_tracking.h and include/linux/kvm_host.h.
>
> Could we please first do the cleanups before complicating the code and applying 
> more substantial changes?

The further cleanups wouldn't complicate the code.  It's just that
guest_enter/guest_exit require IRQs off but don't have __.

I'm thinking of something like this (untested):

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index d259274238db..c2dc581ddb0e 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -84,7 +84,7 @@ static inline void context_tracking_init(void) { }
 
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
-static inline void guest_enter(void)
+static inline void __guest_enter(void)
 {
 	if (vtime_accounting_cpu_enabled())
 		vtime_guest_enter(current);
@@ -93,9 +93,19 @@ static inline void guest_enter(void)
 
 	if (context_tracking_is_enabled())
 		__context_tracking_enter(CONTEXT_GUEST);
+
+	/* KVM does not hold any references to rcu protected data when it
+	 * switches CPU into a guest mode. In fact switching to a guest mode
+	 * is very similar to exiting to userspace from rcu point of view. In
+	 * addition CPU may stay in a guest mode for quite a long time (up to
+	 * one time slice). Lets treat guest mode as quiescent state, just like
+	 * we do with user-mode execution.
+	 */
+	if (!context_tracking_cpu_is_enabled())
+		rcu_virt_note_context_switch(smp_processor_id());
 }
 
-static inline void guest_exit(void)
+static inline void __guest_exit(void)
 {
 	if (context_tracking_is_enabled())
 		__context_tracking_exit(CONTEXT_GUEST);
@@ -107,7 +117,7 @@ static inline void guest_exit(void)
 }
 
 #else
-static inline void guest_enter(void)
+static inline void __guest_enter(void)
 {
 	/*
 	 * This is running in ioctl context so its safe
@@ -118,7 +128,7 @@ static inline void guest_enter(void)
 	current->flags |= PF_VCPU;
 }
 
-static inline void guest_exit(void)
+static inline void __guest_exit(void)
 {
 	/* Flush the guest cputime we spent on the guest */
 	vtime_account_system(current);
@@ -126,4 +136,23 @@ static inline void guest_exit(void)
 }
 #endif /* CONFIG_VIRT_CPU_ACCOUNTING_GEN */
 
+static inline void guest_enter(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	__guest_enter();
+	local_irq_restore(flags);
+}
+
+/* must be called with irqs disabled */
+static inline void guest_exit(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	__guest_exit();
+	local_irq_restore(flags);
+}
+
 #endif
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 5276fe0916fc..d00fdaa8da15 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -861,40 +861,23 @@ static inline void kvm_iommu_unmap_pages(struct kvm *kvm,
 /* must be called with irqs disabled */
 static inline void __kvm_guest_enter(void)
 {
-	guest_enter();
-	/* KVM does not hold any references to rcu protected data when it
-	 * switches CPU into a guest mode. In fact switching to a guest mode
-	 * is very similar to exiting to userspace from rcu point of view. In
-	 * addition CPU may stay in a guest mode for quite a long time (up to
-	 * one time slice). Lets treat guest mode as quiescent state, just like
-	 * we do with user-mode execution.
-	 */
-	if (!context_tracking_cpu_is_enabled())
-		rcu_virt_note_context_switch(smp_processor_id());
+	__guest_enter();
 }
 
 /* must be called with irqs disabled */
 static inline void __kvm_guest_exit(void)
 {
-	guest_exit();
+	__guest_exit();
 }
 
 static inline void kvm_guest_enter(void)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__kvm_guest_enter();
-	local_irq_restore(flags);
+	guest_enter();
 }
 
 static inline void kvm_guest_exit(void)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__kvm_guest_exit();
-	local_irq_restore(flags);
+	guest_exit();
 }
 
 /*


and then removing the kvm_-prefixed functions.  It's little
more than code movement.

Paolo

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 08/06/2016 14:16, Ingo Molnar wrote:
> > > The guest ones are not quite as consistent.  I can fix that later,
> > > there's no reason also to have guest context tracking split between
> > > include/linux/context_tracking.h and include/linux/kvm_host.h.
> >
> > Could we please first do the cleanups before complicating the code and applying 
> > more substantial changes?
> 
> The further cleanups wouldn't complicate the code.  It's just that
> guest_enter/guest_exit require IRQs off but don't have __.
> 
> I'm thinking of something like this (untested):
> 
> diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
> index d259274238db..c2dc581ddb0e 100644
> --- a/include/linux/context_tracking.h
> +++ b/include/linux/context_tracking.h
> @@ -84,7 +84,7 @@ static inline void context_tracking_init(void) { }
>  
>  
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
> -static inline void guest_enter(void)
> +static inline void __guest_enter(void)
>  {
>  	if (vtime_accounting_cpu_enabled())
>  		vtime_guest_enter(current);
> @@ -93,9 +93,19 @@ static inline void guest_enter(void)
>  
>  	if (context_tracking_is_enabled())
>  		__context_tracking_enter(CONTEXT_GUEST);
> +
> +	/* KVM does not hold any references to rcu protected data when it
> +	 * switches CPU into a guest mode. In fact switching to a guest mode

Nit, please use the customary (multi-line) comment style:

  /*
   * Comment .....
   * ...... goes here.
   */

> +	 * is very similar to exiting to userspace from rcu point of view. In

s/RCU

> +	 * addition CPU may stay in a guest mode for quite a long time (up to
> +	 * one time slice). Lets treat guest mode as quiescent state, just like
> +	 * we do with user-mode execution.
> +	 */
> +	if (!context_tracking_cpu_is_enabled())
> +		rcu_virt_note_context_switch(smp_processor_id());
>  }
>  
> -static inline void guest_exit(void)
> +static inline void __guest_exit(void)

> +static inline void guest_enter(void)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	__guest_enter();
> +	local_irq_restore(flags);

So I believe it would be cleaner to name the irqs-off code paths explicitly: 
__guest_enter_irqsoff(), and propagate that naming into other parts as well?

>  /* must be called with irqs disabled */
>  static inline void __kvm_guest_exit(void)

This way all these random comments about irqs-off requirements would become 
unnecessary - the code becomes self-documenting.

Thanks,

	Ingo

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Paolo Bonzini]

> So I believe it would be cleaner to name the irqs-off code paths explicitly:
> __guest_enter_irqsoff(), and propagate that naming into other parts as well?

Ok, I'll send v2 with both the KVM cleanups and the entry optimizations.  It should
be four patches putting all things together.  Thanks for the review!

Paolo

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> > So I believe it would be cleaner to name the irqs-off code paths explicitly:
> > __guest_enter_irqsoff(), and propagate that naming into other parts as well?
> 
> Ok, I'll send v2 with both the KVM cleanups and the entry optimizations.  It should
> be four patches putting all things together.  Thanks for the review!

Sounds good to me, thanks!

	Ingo

Re: [PATCH 2/2] x86/entry: Inline enter_from_user_mode

[Andy Lutomirski]

On Mon, Jun 6, 2016 at 9:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 04/06/2016 07:08, Andy Lutomirski wrote:
>> On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>>>
>>> This matches what is already done for prepare_exit_to_usermode,
>>> and saves about 60 clock cycles (4% speedup) with the benchmark
>>> in the previous commit message.
>>>
>>> Cc: Andy Lutomirski <luto@kernel.org>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Rik van Riel <riel@redhat.com>
>>> Cc: H. Peter Anvin <hpa@zytor.com>
>>> Cc: Ingo Molnar <mingo@kernel.org.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  arch/x86/entry/common.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>>> index 946bc1a..582bbc8 100644
>>> --- a/arch/x86/entry/common.c
>>> +++ b/arch/x86/entry/common.c
>>> @@ -40,7 +40,7 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
>>>
>>>  #ifdef CONFIG_CONTEXT_TRACKING
>>>  /* Called on entry from user mode with IRQs off. */
>>> -__visible void enter_from_user_mode(void)
>>> +__visible inline void enter_from_user_mode(void)
>>>  {
>>>         CT_WARN_ON(ct_state() != CONTEXT_USER);
>>>         __user_exit();
>>
>> I wonder if an extern inline *declaration* is needed as well in this C
>> file.  At least C99 suggests it is.  Maybe __visible is sufficient to
>> force an external definition to be emitted.
>
> An extern inline declaration is not needed because the kernel uses
> -std=gnu89 (or, if you prefer, because prepare_exit_to_usermode didn't
> have one :)).
>
> It's awesomely perverted:
>
>   __attribute__((externally_visible)) inline void f(void) {}
>
>   inline void g(void) {}
>
>   extern inline void h(void);
>   extern inline void h(void) {}
>
>   inline void i(void);
>   inline void i(void) {}
>
>   extern inline void j(void);
>   inline void j(void) {}
>
> This patch (and the preexisting prepare_exit_to_usermode code) are
> equivalent to "f".
>
> Compile the above file with "--std=gnu89" or "--std=gnu99
> -fgnu89-inline" and f/g/i/j are emitted.
>
> Compile it with "--std=gnu99 -fno-gnu89-inline" and h/j is emitted.
>
> Yes, the standard is _almost exactly_ the opposite of the preexisting
> GCC implementation.  The only case which achieves the same effect is
> when declarations are "extern inline" and definitions must always be
> "inline".  Or of course just use "static inline".  At least it's
> decently documented in the GCC info documentation.

OK.

In any event, if this ever messes up, it'll fail to build and we'll notice.

--Andy

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Andy Lutomirski]

On Mon, Jun 20, 2016 at 7:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Thanks to all the work that was done by Andy Lutomirski and others,
> enter_from_user_mode and prepare_exit_to_usermode are now called only with
> interrupts disabled.  Let's provide them a version of user_enter/user_exit
> that skips saving and restoring the interrupt flag.

You're also skipping the in_interrupt() check, but that appears to be fine.

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

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

On Mon, 2016-06-20 at 16:58 +0200, Paolo Bonzini wrote:
> Thanks to all the work that was done by Andy Lutomirski and others,
> enter_from_user_mode and prepare_exit_to_usermode are now called only
> with
> interrupts disabled.  Let's provide them a version of
> user_enter/user_exit
> that skips saving and restoring the interrupt flag.
> 
> On an AMD-based machine I tested this patch on, with force-enabled
> context tracking, the speed-up in system calls was 90 clock cycles or
> 6%,
> measured with the following simple benchmark:
> 
>     #include <sys/signal.h>
>     #include <time.h>
>     #include <unistd.h>
>     #include <stdio.h>
> 
>     unsigned long rdtsc()
>     {
>         unsigned long result;
>         asm volatile("rdtsc; shl $32, %%rdx; mov %%eax, %%eax\n"
>                      "or %%rdx, %%rax" : "=a" (result) : : "rdx");
>         return result;
>     }
> 
>     int main()
>     {
>         unsigned long tsc1, tsc2;
>         int pid = getpid();
>         int i;
> 
>         tsc1 = rdtsc();
>         for (i = 0; i < 100000000; i++)
>             kill(pid, SIGWINCH);
>         tsc2 = rdtsc();
> 
>         printf("%ld\n", tsc2 - tsc1);
>     }
> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All Rights Reversed.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Paolo Bonzini]

Thanks to all the work that was done by Andy Lutomirski and others,
enter_from_user_mode and prepare_exit_to_usermode are now called only with
interrupts disabled.  Let's provide them a version of user_enter/user_exit
that skips saving and restoring the interrupt flag.

On an AMD-based machine I tested this patch on, with force-enabled
context tracking, the speed-up in system calls was 90 clock cycles or 6%,
measured with the following simple benchmark:

    #include <sys/signal.h>
    #include <time.h>
    #include <unistd.h>
    #include <stdio.h>

    unsigned long rdtsc()
    {
        unsigned long result;
        asm volatile("rdtsc; shl $32, %%rdx; mov %%eax, %%eax\n"
                     "or %%rdx, %%rax" : "=a" (result) : : "rdx");
        return result;
    }

    int main()
    {
        unsigned long tsc1, tsc2;
        int pid = getpid();
        int i;

        tsc1 = rdtsc();
        for (i = 0; i < 100000000; i++)
            kill(pid, SIGWINCH);
        tsc2 = rdtsc();

        printf("%ld\n", tsc2 - tsc1);
    }

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/entry/common.c          |  4 ++--
 include/linux/context_tracking.h | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index ec138e538c44..618bc61d35b7 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -43,7 +43,7 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
 __visible void enter_from_user_mode(void)
 {
 	CT_WARN_ON(ct_state() != CONTEXT_USER);
-	user_exit();
+	user_exit_irqoff();
 }
 #else
 static inline void enter_from_user_mode(void) {}
@@ -274,7 +274,7 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
 	ti->status &= ~TS_COMPAT;
 #endif
 
-	user_enter();
+	user_enter_irqoff();
 }
 
 #define SYSCALL_EXIT_WORK_FLAGS				\
diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index d259274238db..d9aef2a0ec8e 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -31,6 +31,19 @@ static inline void user_exit(void)
 		context_tracking_exit(CONTEXT_USER);
 }
 
+/* Called with interrupts disabled.  */
+static inline void user_enter_irqoff(void)
+{
+	if (context_tracking_is_enabled())
+		__context_tracking_enter(CONTEXT_USER);
+
+}
+static inline void user_exit_irqoff(void)
+{
+	if (context_tracking_is_enabled())
+		__context_tracking_exit(CONTEXT_USER);
+}
+
 static inline enum ctx_state exception_enter(void)
 {
 	enum ctx_state prev_ctx;
@@ -69,6 +82,8 @@ static inline enum ctx_state ct_state(void)
 #else
 static inline void user_enter(void) { }
 static inline void user_exit(void) { }
+static inline void user_enter_irqoff(void) { }
+static inline void user_exit_irqoff(void) { }
 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; }
-- 
1.8.3.1