Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1542878 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [patch 0/3] x86/process: Optimize __switch_to_extra() Thomas Gleixner <tglx@linutronix.de> - 2016-12-15 17:50 +0100
    [patch 2/3] x86/process: Optimize TIF_BLOCKSTEP switch Thomas Gleixner <tglx@linutronix.de> - 2016-12-15 17:50 +0100
      Re: [patch 2/3] x86/process: Optimize TIF_BLOCKSTEP switch Andy Lutomirski <luto@amacapital.net> - 2016-12-15 18:30 +0100
        Re: [patch 2/3] x86/process: Optimize TIF_BLOCKSTEP switch Thomas Gleixner <tglx@linutronix.de> - 2016-12-16 10:00 +0100
          Re: [patch 2/3] x86/process: Optimize TIF_BLOCKSTEP switch Andy Lutomirski <luto@amacapital.net> - 2016-12-16 20:30 +0100
    [patch 1/3] x86/process: Optimize TIF checks in switch_to_extra() Thomas Gleixner <tglx@linutronix.de> - 2016-12-15 17:50 +0100
      Re: [patch 1/3] x86/process: Optimize TIF checks in  switch_to_extra() Thomas Gleixner <tglx@linutronix.de> - 2016-12-15 18:30 +0100
        Re: [patch 1/3] x86/process: Optimize TIF checks in switch_to_extra() Peter Zijlstra <peterz@infradead.org> - 2016-12-15 18:40 +0100
      Re: [patch 1/3] x86/process: Optimize TIF checks in switch_to_extra() Andy Lutomirski <luto@amacapital.net> - 2016-12-15 18:30 +0100
      Re: [patch 1/3] x86/process: Optimize TIF checks in switch_to_extra() Peter Zijlstra <peterz@infradead.org> - 2016-12-15 18:30 +0100

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

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(-)

[toc] | [next] | [standalone]

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

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)

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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);
 }
 
 /*

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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);
	}

?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web