[patch 0/3] x86/process: Optimize __switch_to_extra()

[patch 0/3] x86/process: Optimize __switch_to_extra()

[Thomas Gleixner]

GCC generates lousy code in __switch_to_extra(). Aside of that some of the
operations there are implemented suboptimal.

This series, inspired by a patch from Kyle, helps the compiler to be less
stupid by explicitely giving the hints to optimize and replaces the open
coded bit toggle mechanisms with proper helper functions.

The resulting change in text size:

	 64bit	    32bit
Before:	  3726	     9388
After:	  3646	     9324
Delta:	    80	      152	    

The number of conditional jumps is also reduced:

	 64bit	    32bit
Before:	     8	       13
After:	     5	       10

Thanks,

	tglx

---
 include/asm/processor.h |   12 ++++++++
 include/asm/tlbflush.h  |   10 ++++++
 kernel/process.c        |   70 +++++++++++++++++++-----------------------------
 3 files changed, 51 insertions(+), 41 deletions(-)

[patch 2/3] x86/process: Optimize TIF_BLOCKSTEP switch

[Thomas Gleixner]

[-- Attachment #1: x86-process--Optimize-TIF_BLOCKSTEP-switch.patch --]
[-- Type: text/plain, Size: 1677 bytes --]

Provide and use a seperate helper for toggling the DEBUGCTLMSR_BTF bit
instead of doing it open coded with a branch and eventually evaluating
boot_cpu_data twice.

x86_64:
3694	   8505	     16	  12215	   2fb7	Before
3662	   8505	     16	  12183	   2f97 After

i386:
5986	   9388	   1804	  17178	   431a	Before
5906	   9388	   1804	  17098	   42ca	After

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/processor.h |   12 ++++++++++++
 arch/x86/kernel/process.c        |   10 ++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -676,6 +676,18 @@ static inline void update_debugctlmsr(un
 	wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctlmsr);
 }
 
+static inline void toggle_debugctlmsr(unsigned long mask)
+{
+	unsigned long msrval;
+
+#ifndef CONFIG_X86_DEBUGCTLMSR
+	if (boot_cpu_data.x86 < 6)
+		return;
+#endif
+	rdmsrl(MSR_IA32_DEBUGCTLMSR, msrval);
+	wrmsrl(MSR_IA32_DEBUGCTLMSR, msrval ^ mask);
+}
+
 extern void set_task_blockstep(struct task_struct *task, bool on);
 
 /* Boot loader type from the setup header: */
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -209,14 +209,8 @@ void __switch_to_xtra(struct task_struct
 
 	propagate_user_return_notify(prev_p, next_p);
 
-	if ((tifp ^ tifn) & _TIF_BLOCKSTEP) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl &= ~DEBUGCTLMSR_BTF;
-		if (tifn & _TIF_BLOCKSTEP)
-			debugctl |= DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-	}
+	if ((tifp ^ tifn) & _TIF_BLOCKSTEP)
+		toggle_debugctlmsr(DEBUGCTLMSR_BTF);
 
 	if ((tifp ^ tifn) & _TIF_NOTSC) {
 		if (tifn & _TIF_NOTSC)

[patch 1/3] x86/process: Optimize TIF checks in switch_to_extra()

[Thomas Gleixner]

[-- Attachment #1: x86-process--Optimize-TIF-checks-in-switch_to_extra--.patch --]
[-- Type: text/plain, Size: 2883 bytes --]

Help the compiler to avoid reevaluating the thread flags for each checked
bit by reordering the bit checks and providing an explicit xor for
evaluation.

x8664: arch/x86/kernel/process.o
text	   data	    bss	    dec	    hex
3726	   8505	     16	  12247	   2fd7	Before
3694	   8505	     16	  12215	   2fb7	After

i386:  No change

Originally-from: Kyle Huey <khuey@kylehuey.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/process.c |   54 ++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -174,48 +174,56 @@ int set_tsc_mode(unsigned int val)
 	return 0;
 }
 
+static inline void switch_to_bitmap(struct tss_struct *tss,
+				    struct thread_struct *prev,
+				    struct thread_struct *next,
+				    unsigned long tifp, unsigned long tifn)
+{
+	if (tifn & _TIF_IO_BITMAP) {
+		/*
+		 * Copy the relevant range of the IO bitmap.
+		 * Normally this is 128 bytes or less:
+		 */
+		memcpy(tss->io_bitmap, next->io_bitmap_ptr,
+		       max(prev->io_bitmap_max, next->io_bitmap_max));
+	} else if (tifp & _TIF_IO_BITMAP) {
+		/*
+		 * Clear any possible leftover bits:
+		 */
+		memset(tss->io_bitmap, 0xff, prev->io_bitmap_max);
+	}
+}
+
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss)
 {
 	struct thread_struct *prev, *next;
+	unsigned long tifp, tifn;
 
 	prev = &prev_p->thread;
 	next = &next_p->thread;
 
-	if (test_tsk_thread_flag(prev_p, TIF_BLOCKSTEP) ^
-	    test_tsk_thread_flag(next_p, TIF_BLOCKSTEP)) {
+	tifn = task_thread_info(next_p)->flags;
+	tifp = task_thread_info(prev_p)->flags;
+	switch_to_bitmap(tss, prev, next, tifp, tifn);
+
+	propagate_user_return_notify(prev_p, next_p);
+
+	if ((tifp ^ tifn) & _TIF_BLOCKSTEP) {
 		unsigned long debugctl = get_debugctlmsr();
 
 		debugctl &= ~DEBUGCTLMSR_BTF;
-		if (test_tsk_thread_flag(next_p, TIF_BLOCKSTEP))
+		if (tifn & _TIF_BLOCKSTEP)
 			debugctl |= DEBUGCTLMSR_BTF;
-
 		update_debugctlmsr(debugctl);
 	}
 
-	if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^
-	    test_tsk_thread_flag(next_p, TIF_NOTSC)) {
-		/* prev and next are different */
-		if (test_tsk_thread_flag(next_p, TIF_NOTSC))
+	if ((tifp ^ tifn) & _TIF_NOTSC) {
+		if (tifn & _TIF_NOTSC)
 			hard_disable_TSC();
 		else
 			hard_enable_TSC();
 	}
-
-	if (test_tsk_thread_flag(next_p, TIF_IO_BITMAP)) {
-		/*
-		 * Copy the relevant range of the IO bitmap.
-		 * Normally this is 128 bytes or less:
-		 */
-		memcpy(tss->io_bitmap, next->io_bitmap_ptr,
-		       max(prev->io_bitmap_max, next->io_bitmap_max));
-	} else if (test_tsk_thread_flag(prev_p, TIF_IO_BITMAP)) {
-		/*
-		 * Clear any possible leftover bits:
-		 */
-		memset(tss->io_bitmap, 0xff, prev->io_bitmap_max);
-	}
-	propagate_user_return_notify(prev_p, next_p);
 }
 
 /*

[patch 3/3] x86/process: Optimize TIF_NOTSC switch

[Thomas Gleixner]

[-- Attachment #1: x86-process--Optimize-TIF_NOTSC-switch.patch --]
[-- Type: text/plain, Size: 2111 bytes --]

Provide and use a toggle helper instead of doing it with a branch.

x86_64:
3662       8505      16   12183    2f97 Before
3646	   8505	     16	  12167	   2f87	After 

i386:
5906	   9388	   1804	  17098	   42ca	Before
5834	   9324	   1740	  16898	   4202	After

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/tlbflush.h |   10 ++++++++++
 arch/x86/kernel/process.c       |   22 ++++------------------
 2 files changed, 14 insertions(+), 18 deletions(-)

--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -110,6 +110,16 @@ static inline void cr4_clear_bits(unsign
 	}
 }
 
+static inline void cr4_toggle_bits(unsigned long mask)
+{
+	unsigned long cr4;
+
+	cr4 = this_cpu_read(cpu_tlbstate.cr4);
+	cr4 ^= mask;
+	this_cpu_write(cpu_tlbstate.cr4, cr4);
+	__write_cr4(cr4);
+}
+
 /* Read the CR4 shadow. */
 static inline unsigned long cr4_read_shadow(void)
 {
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -116,11 +116,6 @@ void flush_thread(void)
 	fpu__clear(&tsk->thread.fpu);
 }
 
-static void hard_disable_TSC(void)
-{
-	cr4_set_bits(X86_CR4_TSD);
-}
-
 void disable_TSC(void)
 {
 	preempt_disable();
@@ -129,15 +124,10 @@ void disable_TSC(void)
 		 * Must flip the CPU state synchronously with
 		 * TIF_NOTSC in the current running context.
 		 */
-		hard_disable_TSC();
+		cr4_set_bits(X86_CR4_TSD);
 	preempt_enable();
 }
 
-static void hard_enable_TSC(void)
-{
-	cr4_clear_bits(X86_CR4_TSD);
-}
-
 static void enable_TSC(void)
 {
 	preempt_disable();
@@ -146,7 +136,7 @@ static void enable_TSC(void)
 		 * Must flip the CPU state synchronously with
 		 * TIF_NOTSC in the current running context.
 		 */
-		hard_enable_TSC();
+		cr4_clear_bits(X86_CR4_TSD);
 	preempt_enable();
 }
 
@@ -212,12 +202,8 @@ void __switch_to_xtra(struct task_struct
 	if ((tifp ^ tifn) & _TIF_BLOCKSTEP)
 		toggle_debugctlmsr(DEBUGCTLMSR_BTF);
 
-	if ((tifp ^ tifn) & _TIF_NOTSC) {
-		if (tifn & _TIF_NOTSC)
-			hard_disable_TSC();
-		else
-			hard_enable_TSC();
-	}
+	if ((tifp ^ tifn) & _TIF_NOTSC)
+		cr4_toggle_bits(X86_CR4_TSD);
 }
 
 /*

Re: [patch 1/3] x86/process: Optimize TIF checks in switch_to_extra()

[Peter Zijlstra]

On Thu, Dec 15, 2016 at 04:44:02PM -0000, Thomas Gleixner wrote:
>  void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
>  		      struct tss_struct *tss)
>  {
>  	struct thread_struct *prev, *next;
> +	unsigned long tifp, tifn;
>  
>  	prev = &prev_p->thread;
>  	next = &next_p->thread;
>  
> +	tifn = task_thread_info(next_p)->flags;
> +	tifp = task_thread_info(prev_p)->flags;
> +	switch_to_bitmap(tss, prev, next, tifp, tifn);
> +
> +	propagate_user_return_notify(prev_p, next_p);
> +
> +	if ((tifp ^ tifn) & _TIF_BLOCKSTEP) {
>  		unsigned long debugctl = get_debugctlmsr();
>  
>  		debugctl &= ~DEBUGCTLMSR_BTF;
> +		if (tifn & _TIF_BLOCKSTEP)
>  			debugctl |= DEBUGCTLMSR_BTF;
>  		update_debugctlmsr(debugctl);
>  	}

Going by the toggle patter you have elsewhere, wouldn't that then be:

	if ((tifp ^ tifn) & _TIF_BLOCKSTEP) {
		unsigned long debugctl = get_debugctlmsr();

		debugctl ^= DEBUGCTLMSR_BTF;
		update_debugctlmsr(debugctl);
	}

?

Re: [patch 1/3] x86/process: Optimize TIF checks in switch_to_extra()

[Andy Lutomirski]

On Thu, Dec 15, 2016 at 8:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> -       if (test_tsk_thread_flag(prev_p, TIF_BLOCKSTEP) ^
> -           test_tsk_thread_flag(next_p, TIF_BLOCKSTEP)) {
> +       tifn = task_thread_info(next_p)->flags;
> +       tifp = task_thread_info(prev_p)->flags;

Minor nit, but I think that a sufficiently clever compiler could
interpret this to mean "no one else is modifying these flags, so I can
do clever crazy things".  Wrapping these in READ_ONCE might be
helpful.

Re: [patch 1/3] x86/process: Optimize TIF checks in switch_to_extra()

[Thomas Gleixner]

On Thu, 15 Dec 2016, Peter Zijlstra wrote:
> On Thu, Dec 15, 2016 at 04:44:02PM -0000, Thomas Gleixner wrote:
> >  void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
> >  		      struct tss_struct *tss)
> >  {
> >  	struct thread_struct *prev, *next;
> > +	unsigned long tifp, tifn;
> >  
> >  	prev = &prev_p->thread;
> >  	next = &next_p->thread;
> >  
> > +	tifn = task_thread_info(next_p)->flags;
> > +	tifp = task_thread_info(prev_p)->flags;
> > +	switch_to_bitmap(tss, prev, next, tifp, tifn);
> > +
> > +	propagate_user_return_notify(prev_p, next_p);
> > +
> > +	if ((tifp ^ tifn) & _TIF_BLOCKSTEP) {
> >  		unsigned long debugctl = get_debugctlmsr();
> >  
> >  		debugctl &= ~DEBUGCTLMSR_BTF;
> > +		if (tifn & _TIF_BLOCKSTEP)
> >  			debugctl |= DEBUGCTLMSR_BTF;
> >  		update_debugctlmsr(debugctl);
> >  	}
> 
> Going by the toggle patter you have elsewhere, wouldn't that then be:
> 
> 	if ((tifp ^ tifn) & _TIF_BLOCKSTEP) {
> 		unsigned long debugctl = get_debugctlmsr();
> 
> 		debugctl ^= DEBUGCTLMSR_BTF;
> 		update_debugctlmsr(debugctl);
> 	}

See the next patch

Re: [patch 2/3] x86/process: Optimize TIF_BLOCKSTEP switch

[Andy Lutomirski]

On Thu, Dec 15, 2016 at 8:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Provide and use a seperate helper for toggling the DEBUGCTLMSR_BTF bit
> instead of doing it open coded with a branch and eventually evaluating
> boot_cpu_data twice.
>
> x86_64:
> 3694       8505      16   12215    2fb7 Before
> 3662       8505      16   12183    2f97 After
>
> i386:
> 5986       9388    1804   17178    431a Before
> 5906       9388    1804   17098    42ca After
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/processor.h |   12 ++++++++++++
>  arch/x86/kernel/process.c        |   10 ++--------
>  2 files changed, 14 insertions(+), 8 deletions(-)
>
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -676,6 +676,18 @@ static inline void update_debugctlmsr(un
>         wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctlmsr);
>  }
>
> +static inline void toggle_debugctlmsr(unsigned long mask)
> +{
> +       unsigned long msrval;
> +
> +#ifndef CONFIG_X86_DEBUGCTLMSR
> +       if (boot_cpu_data.x86 < 6)
> +               return;
> +#endif
> +       rdmsrl(MSR_IA32_DEBUGCTLMSR, msrval);
> +       wrmsrl(MSR_IA32_DEBUGCTLMSR, msrval ^ mask);
> +}
> +

This scares me.  If the MSR ever gets out of sync with the TI flag,
this will malfunction.  And IIRC the MSR is highly magical and the CPU
clears it all by itself under a variety of not-so-well documented
circumstances.

How about adding a real feature bit and doing:

if (!static_cpu_has(X86_FEATURE_BLOCKSTEP))
  return;

rdmsrl(MSR_IA32_DEBUGCTLMSR, msrval);
msrval &= DEBUGCTLMSR_BTF;
msrval |= (tifn >> TIF_BLOCKSTEP) << DEBUGCTLMSR_BIT;

--Andy

Re: [patch 3/3] x86/process: Optimize TIF_NOTSC switch

[Andy Lutomirski]

On Thu, Dec 15, 2016 at 8:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Provide and use a toggle helper instead of doing it with a branch.
>
> x86_64:
> 3662       8505      16   12183    2f97 Before
> 3646       8505      16   12167    2f87 After
>
> i386:
> 5906       9388    1804   17098    42ca Before
> 5834       9324    1740   16898    4202 After
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/tlbflush.h |   10 ++++++++++
>  arch/x86/kernel/process.c       |   22 ++++------------------
>  2 files changed, 14 insertions(+), 18 deletions(-)
>
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -110,6 +110,16 @@ static inline void cr4_clear_bits(unsign
>         }
>  }
>
> +static inline void cr4_toggle_bits(unsigned long mask)
> +{
> +       unsigned long cr4;
> +
> +       cr4 = this_cpu_read(cpu_tlbstate.cr4);
> +       cr4 ^= mask;
> +       this_cpu_write(cpu_tlbstate.cr4, cr4);
> +       __write_cr4(cr4);
> +}

This scares me for the same reason as BTF, although this should at
least be less fragile.  But how about:

static inline void cr4_set_bit_to(unsigned long mask, bool set)
{
  ...
  cr4 &= ~mask;
  cr4 ^= (set << ilog2(mask));
  ...
}

This should generate code that's nearly as good.

Re: [patch 1/3] x86/process: Optimize TIF checks in switch_to_extra()

[Peter Zijlstra]

On Thu, Dec 15, 2016 at 06:26:28PM +0100, Thomas Gleixner wrote:
> See the next patch

Duh, I'm an idiot. For some reason I though this one got missed.

Re: [patch 2/3] x86/process: Optimize TIF_BLOCKSTEP switch

[Thomas Gleixner]

On Thu, 15 Dec 2016, Andy Lutomirski wrote:
> On Thu, Dec 15, 2016 at 8:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > +static inline void toggle_debugctlmsr(unsigned long mask)
> > +{
> > +       unsigned long msrval;
> > +
> > +#ifndef CONFIG_X86_DEBUGCTLMSR
> > +       if (boot_cpu_data.x86 < 6)
> > +               return;
> > +#endif
> > +       rdmsrl(MSR_IA32_DEBUGCTLMSR, msrval);
> > +       wrmsrl(MSR_IA32_DEBUGCTLMSR, msrval ^ mask);
> > +}
> > +
> 
> This scares me.  If the MSR ever gets out of sync with the TI flag,
> this will malfunction.  And IIRC the MSR is highly magical and the CPU
> clears it all by itself under a variety of not-so-well documented
> circumstances.

If that is true, then the code today is broken as well, when the flag has
been cleared and both prev and next have the flag set. Then it won't be
updated for the next task.

The we should not use the TIF flag and store a debugmask in thread info and
do:

	if (prev->debugmask || next->debugmask) {
    		if (static_cpu_has(X86_FEATURE_BLOCKSTEP)) {
			rdmsrl(MSR_IA32_DEBUGCTLMSR, msrval);
			msrval &= DEBUGCTLMSR_BTF;
			msrval |= next->debugmask;
		}
	}

Thanks,

	tglx

Re: [patch 3/3] x86/process: Optimize TIF_NOTSC switch

[Thomas Gleixner]

On Thu, 15 Dec 2016, Andy Lutomirski wrote:
> On Thu, Dec 15, 2016 at 8:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > +static inline void cr4_toggle_bits(unsigned long mask)
> > +{
> > +       unsigned long cr4;
> > +
> > +       cr4 = this_cpu_read(cpu_tlbstate.cr4);
> > +       cr4 ^= mask;
> > +       this_cpu_write(cpu_tlbstate.cr4, cr4);
> > +       __write_cr4(cr4);
> > +}
> 
> This scares me for the same reason as BTF, although this should at
> least be less fragile.  But how about:

If that is fragile then all cr4 manipulation code is fragile because it
relies on cpu_tlbstate.cr4. The TIF flag and that per cpu thing are kept in
sync.

Thanks,

	tglx

Re: [patch 3/3] x86/process: Optimize TIF_NOTSC switch

[Andy Lutomirski]

On Fri, Dec 16, 2016 at 12:50 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 15 Dec 2016, Andy Lutomirski wrote:
>> On Thu, Dec 15, 2016 at 8:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > +static inline void cr4_toggle_bits(unsigned long mask)
>> > +{
>> > +       unsigned long cr4;
>> > +
>> > +       cr4 = this_cpu_read(cpu_tlbstate.cr4);
>> > +       cr4 ^= mask;
>> > +       this_cpu_write(cpu_tlbstate.cr4, cr4);
>> > +       __write_cr4(cr4);
>> > +}
>>
>> This scares me for the same reason as BTF, although this should at
>> least be less fragile.  But how about:
>
> If that is fragile then all cr4 manipulation code is fragile because it
> relies on cpu_tlbstate.cr4. The TIF flag and that per cpu thing are kept in
> sync.

True.

Re: [patch 2/3] x86/process: Optimize TIF_BLOCKSTEP switch

[Andy Lutomirski]

On Fri, Dec 16, 2016 at 12:47 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 15 Dec 2016, Andy Lutomirski wrote:
>> On Thu, Dec 15, 2016 at 8:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > +static inline void toggle_debugctlmsr(unsigned long mask)
>> > +{
>> > +       unsigned long msrval;
>> > +
>> > +#ifndef CONFIG_X86_DEBUGCTLMSR
>> > +       if (boot_cpu_data.x86 < 6)
>> > +               return;
>> > +#endif
>> > +       rdmsrl(MSR_IA32_DEBUGCTLMSR, msrval);
>> > +       wrmsrl(MSR_IA32_DEBUGCTLMSR, msrval ^ mask);
>> > +}
>> > +
>>
>> This scares me.  If the MSR ever gets out of sync with the TI flag,
>> this will malfunction.  And IIRC the MSR is highly magical and the CPU
>> clears it all by itself under a variety of not-so-well documented
>> circumstances.
>
> If that is true, then the code today is broken as well, when the flag has
> been cleared and both prev and next have the flag set. Then it won't be
> updated for the next task.
>
> The we should not use the TIF flag and store a debugmask in thread info and
> do:
>
>         if (prev->debugmask || next->debugmask) {
>                 if (static_cpu_has(X86_FEATURE_BLOCKSTEP)) {
>                         rdmsrl(MSR_IA32_DEBUGCTLMSR, msrval);
>                         msrval &= DEBUGCTLMSR_BTF;
>                         msrval |= next->debugmask;
>                 }
>         }

Seems reasonable to me.  Although keeping it in flags might simplify
the logic a bit.  FWIW, I doubt we care about performance much when
either prev or next has the bit set.

--Andy