[PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Stephane Eranian]

Hello,

Following my discussion with Andi. Here is a patch that introduces
two new TIF flags to simplify the context switch code in __switch_to().
The idea is to minimize the number of cache lines accessed in the common
case, i.e., when neither the debug registers nor the I/O bitmap are used.

This patch covers the x86-64 modifications. A patch for i386 follows.

Changelog:
	- add TIF_DEBUG to track when debug registers are active
	- add TIF_IO_BITMAP to track when I/O bitmap is used
	- modify __switch_to() to use the new TIF flags

<signed-off-by>: eranian@hpl.hp.com

diff -urNp linux-2.6.17.2.orig/arch/x86_64/ia32/ptrace32.c linux-2.6.17.2-tif/arch/x86_64/ia32/ptrace32.c
--- linux-2.6.17.2.orig/arch/x86_64/ia32/ptrace32.c	2006-06-17 18:49:35.000000000 -0700
+++ linux-2.6.17.2-tif/arch/x86_64/ia32/ptrace32.c	2006-06-30 09:02:16.000000000 -0700
@@ -118,6 +118,10 @@ static int putreg32(struct task_struct *
 			if ((0x5454 >> ((val >> (16 + 4*i)) & 0xf)) & 1)
 			       return -EIO;
 		child->thread.debugreg7 = val; 
+		if (val)
+			set_tsk_thread_flag(child, TIF_DEBUG);
+		else
+			clear_tsk_thread_flag(child, TIF_DEBUG);
 		break; 
 		    
 	default:
diff -urNp linux-2.6.17.2.orig/arch/x86_64/kernel/ioport.c linux-2.6.17.2-tif/arch/x86_64/kernel/ioport.c
--- linux-2.6.17.2.orig/arch/x86_64/kernel/ioport.c	2006-06-17 18:49:35.000000000 -0700
+++ linux-2.6.17.2-tif/arch/x86_64/kernel/ioport.c	2006-07-03 02:06:59.000000000 -0700
@@ -56,6 +56,7 @@ asmlinkage long sys_ioperm(unsigned long
 
 		memset(bitmap, 0xff, IO_BITMAP_BYTES);
 		t->io_bitmap_ptr = bitmap;
+		set_thread_flag(TIF_IO_BITMAP);
 	}
 
 	/*
diff -urNp linux-2.6.17.2.orig/arch/x86_64/kernel/process.c linux-2.6.17.2-tif/arch/x86_64/kernel/process.c
--- linux-2.6.17.2.orig/arch/x86_64/kernel/process.c	2006-06-17 18:49:35.000000000 -0700
+++ linux-2.6.17.2-tif/arch/x86_64/kernel/process.c	2006-07-03 23:48:08.000000000 -0700
@@ -351,6 +351,7 @@ void exit_thread(void)
 
 		kfree(t->io_bitmap_ptr);
 		t->io_bitmap_ptr = NULL;
+		clear_thread_flag(TIF_IO_BITMAP);
 		/*
 		 * Careful, clear this in the TSS too:
 		 */
@@ -366,7 +367,7 @@ void flush_thread(void)
 	struct thread_info *t = current_thread_info();
 
 	if (t->flags & _TIF_ABI_PENDING)
-		t->flags ^= (_TIF_ABI_PENDING | _TIF_IA32);
+		t->flags ^= (_TIF_ABI_PENDING | _TIF_IA32 | _TIF_DEBUG);
 
 	tsk->thread.debugreg0 = 0;
 	tsk->thread.debugreg1 = 0;
@@ -459,7 +460,7 @@ int copy_thread(int nr, unsigned long cl
 	asm("mov %%es,%0" : "=m" (p->thread.es));
 	asm("mov %%ds,%0" : "=m" (p->thread.ds));
 
-	if (unlikely(me->thread.io_bitmap_ptr != NULL)) { 
+	if (unlikely(test_tsk_thread_flag(me, TIF_IO_BITMAP))) { 
 		p->thread.io_bitmap_ptr = kmalloc(IO_BITMAP_BYTES, GFP_KERNEL);
 		if (!p->thread.io_bitmap_ptr) {
 			p->thread.io_bitmap_max = 0;
@@ -467,6 +468,7 @@ int copy_thread(int nr, unsigned long cl
 		}
 		memcpy(p->thread.io_bitmap_ptr, me->thread.io_bitmap_ptr,
 				IO_BITMAP_BYTES);
+		set_tsk_thread_flag(p, TIF_IO_BITMAP);
 	} 
 
 	/*
@@ -496,6 +498,40 @@ out:
  */
 #define loaddebug(thread,r) set_debugreg(thread->debugreg ## r, r)
 
+static inline void __switch_to_xtra(struct task_struct *prev_p,
+			     	    struct task_struct *next_p,
+			     	    struct tss_struct *tss)
+{
+	struct thread_struct *prev, *next;
+
+	prev = &prev_p->thread,
+	next = &next_p->thread;
+
+	if (test_tsk_thread_flag(next_p, TIF_DEBUG)) {
+		loaddebug(next, 0);
+		loaddebug(next, 1);
+		loaddebug(next, 2);
+		loaddebug(next, 3);
+		/* no 4 and 5 */
+		loaddebug(next, 6);
+		loaddebug(next, 7);
+	}
+
+	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);
+	}
+}
+
 /*
  *	switch_to(x,y) should switch tasks from x to y.
  *
@@ -584,37 +620,11 @@ __switch_to(struct task_struct *prev_p, 
 		  task_stack_page(next_p) + THREAD_SIZE - PDA_STACKOFFSET);
 
 	/*
-	 * Now maybe reload the debug registers
+	 * Now maybe reload the debug registers and handle I/O bitmaps
 	 */
-	if (unlikely(next->debugreg7)) {
-		loaddebug(next, 0);
-		loaddebug(next, 1);
-		loaddebug(next, 2);
-		loaddebug(next, 3);
-		/* no 4 and 5 */
-		loaddebug(next, 6);
-		loaddebug(next, 7);
-	}
-
-
-	/* 
-	 * Handle the IO bitmap 
-	 */ 
-	if (unlikely(prev->io_bitmap_ptr || next->io_bitmap_ptr)) {
-		if (next->io_bitmap_ptr)
-			/*
-			 * 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 {
-			/*
-			 * Clear any possible leftover bits:
-			 */
-			memset(tss->io_bitmap, 0xff, prev->io_bitmap_max);
-		}
-	}
+	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
+	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
+		__switch_to_xtra(prev_p, next_p, tss);
 
 	return prev_p;
 }
diff -urNp linux-2.6.17.2.orig/arch/x86_64/kernel/ptrace.c linux-2.6.17.2-tif/arch/x86_64/kernel/ptrace.c
--- linux-2.6.17.2.orig/arch/x86_64/kernel/ptrace.c	2006-06-17 18:49:35.000000000 -0700
+++ linux-2.6.17.2-tif/arch/x86_64/kernel/ptrace.c	2006-07-04 00:21:47.000000000 -0700
@@ -420,9 +420,13 @@ long arch_ptrace(struct task_struct *chi
 				if ((0x5554 >> ((data >> (16 + 4*i)) & 0xf)) & 1)
 					break;
 			if (i == 4) {
-				child->thread.debugreg7 = data;
+			  child->thread.debugreg7 = data;
+			  if (data)
+			  	set_tsk_thread_flag(child, TIF_DEBUG);
+			  else
+			  	clear_tsk_thread_flag(child, TIF_DEBUG);
 			  ret = 0;
-		  }
+		  	}
 		  break;
 		}
 		break;
diff -urNp linux-2.6.17.2.orig/include/asm-x86_64/thread_info.h linux-2.6.17.2-tif/include/asm-x86_64/thread_info.h
--- linux-2.6.17.2.orig/include/asm-x86_64/thread_info.h	2006-06-17 18:49:35.000000000 -0700
+++ linux-2.6.17.2-tif/include/asm-x86_64/thread_info.h	2006-07-03 02:32:25.000000000 -0700
@@ -106,6 +106,8 @@ static inline struct thread_info *stack_
 #define TIF_FORK		18	/* ret_from_fork */
 #define TIF_ABI_PENDING		19
 #define TIF_MEMDIE		20
+#define TIF_DEBUG		21	/* uses debug registers */
+#define TIF_IO_BITMAP		22	/* uses I/O bitmap */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -119,6 +121,8 @@ static inline struct thread_info *stack_
 #define _TIF_IA32		(1<<TIF_IA32)
 #define _TIF_FORK		(1<<TIF_FORK)
 #define _TIF_ABI_PENDING	(1<<TIF_ABI_PENDING)
+#define _TIF_DEBUG		(1<<TIF_DEBUG)
+#define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
@@ -126,6 +130,9 @@ static inline struct thread_info *stack_
 /* work to do on any return to user space */
 #define _TIF_ALLWORK_MASK (0x0000FFFF & ~_TIF_SECCOMP)
 
+/* flags to check in __switch_to() */
+#define _TIF_WORK_CTXSW (_TIF_DEBUG|_TIF_IO_BITMAP)
+
 #define PREEMPT_ACTIVE     0x10000000
 
 /*

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Arjan van de Ven]

> -		}
> -	}
> +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> +		__switch_to_xtra(prev_p, next_p, tss);

well isn't this replacing an if() (which isn't cheap but also not
expensive, due to unlikely()) with an atomic operation (which *is*
expensive) ?

That to me doesn't make this sound like an actual win....

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Andi Kleen]

On Tuesday 04 July 2006 09:51, Arjan van de Ven wrote:
> > -		}
> > -	}
> > +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> > +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> > +		__switch_to_xtra(prev_p, next_p, tss);
> 
> well isn't this replacing an if() (which isn't cheap but also not
> expensive, due to unlikely()) with an atomic operation (which *is*
> expensive) ?

Where do you see an atomic operation?

Also on x86-64 unlikely is an no-op.

-Andi

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Stephane Eranian]

On Tue, Jul 04, 2006 at 09:51:49AM +0200, Arjan van de Ven wrote:
> > -		}
> > -	}
> > +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> > +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> > +		__switch_to_xtra(prev_p, next_p, tss);
> 
> well isn't this replacing an if() (which isn't cheap but also not
> expensive, due to unlikely()) with an atomic operation (which *is*
> expensive) ?
> 
> That to me doesn't make this sound like an actual win....
> 
Although the two if were marked unlikely, you had to do the test anyway.
So you had to touch next->debugreg[7], next->io_bitmap_ptr, and prev->io_bitmap_ptr.
Now the first two are collapsed into one cache line in thread_info->flags.

Yet, I see your point about the test_tsk_thread_flag() and I am wondering if we
do need the atomicity in this case and whether we could simplify by using the
same expression as for next, i.e, task_thread_info(prev_p)->flags & TIF_IO_BITMAP?

-- 
-Stephane

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Stephane Eranian]

Arjan,

On Tue, Jul 04, 2006 at 09:51:49AM +0200, Arjan van de Ven wrote:
> > -		}
> > -	}
> > +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> > +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> > +		__switch_to_xtra(prev_p, next_p, tss);
> 
> well isn't this replacing an if() (which isn't cheap but also not
> expensive, due to unlikely()) with an atomic operation (which *is*
> expensive) ?
> 
Andi is right. I double checked the test_tsk_thread_flag() and it does not
use atomic ops.

-- 
-Stephane

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Andi Kleen]

> diff -urNp linux-2.6.17.2.orig/arch/x86_64/ia32/ptrace32.c linux-2.6.17.2-tif/arch/x86_64/ia32/ptrace32.c
> --- linux-2.6.17.2.orig/arch/x86_64/ia32/ptrace32.c	2006-06-17 18:49:35.000000000 -0700
> +++ linux-2.6.17.2-tif/arch/x86_64/ia32/ptrace32.c	2006-06-30 09:02:16.000000000 -0700

Added thanks. But I had to fix it up by hand because of conflicts. 
Please submit patches against mainline, not stable.

>        if (t->flags & _TIF_ABI_PENDING)
>-               t->flags ^= (_TIF_ABI_PENDING | _TIF_IA32);
>+               t->flags ^= (_TIF_ABI_PENDING | _TIF_IA32 | _TIF_DEBUG);

This xor must be obviously outside the if(). I fixed that up too
by changing it to an unconditional clear.

-Andi

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Stephane Eranian]

Andi,

On Tue, Jul 04, 2006 at 10:14:30AM +0200, Andi Kleen wrote:
> 
> > diff -urNp linux-2.6.17.2.orig/arch/x86_64/ia32/ptrace32.c linux-2.6.17.2-tif/arch/x86_64/ia32/ptrace32.c
> > --- linux-2.6.17.2.orig/arch/x86_64/ia32/ptrace32.c	2006-06-17 18:49:35.000000000 -0700
> > +++ linux-2.6.17.2-tif/arch/x86_64/ia32/ptrace32.c	2006-06-30 09:02:16.000000000 -0700
> 
> Added thanks. But I had to fix it up by hand because of conflicts. 
> Please submit patches against mainline, not stable.
> 
Sorry about that.

> >        if (t->flags & _TIF_ABI_PENDING)
> >-               t->flags ^= (_TIF_ABI_PENDING | _TIF_IA32);
> >+               t->flags ^= (_TIF_ABI_PENDING | _TIF_IA32 | _TIF_DEBUG);
> 
> This xor must be obviously outside the if(). I fixed that up too
> by changing it to an unconditional clear.

Yes, my mistake.

-- 
-Stephane

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Arjan van de Ven]

On Tue, 2006-07-04 at 10:09 +0200, Andi Kleen wrote:
> On Tuesday 04 July 2006 09:51, Arjan van de Ven wrote:
> > > -		}
> > > -	}
> > > +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> > > +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> > > +		__switch_to_xtra(prev_p, next_p, tss);
> > 
> > well isn't this replacing an if() (which isn't cheap but also not
> > expensive, due to unlikely()) with an atomic operation (which *is*
> > expensive) ?
> 
> Where do you see an atomic operation?

test_tsk_thread_flag() ends up doing an atomic op afaics

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Mikael Pettersson]

On Tue, 4 Jul 2006 01:14:13 -0700, Stephane Eranian wrote:
>On Tue, Jul 04, 2006 at 09:51:49AM +0200, Arjan van de Ven wrote:
>> > -		}
>> > -	}
>> > +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
>> > +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
>> > +		__switch_to_xtra(prev_p, next_p, tss);
>> 
>> well isn't this replacing an if() (which isn't cheap but also not
>> expensive, due to unlikely()) with an atomic operation (which *is*
>> expensive) ?
>> 
>Andi is right. I double checked the test_tsk_thread_flag() and it does not
>use atomic ops.

The test_tsk_thread_flag() does not, but what about all the
other places in the patch where currently unsychronised loads
or stores of ->io_bitmap_ptr or ->debugreg7 get replaced or
extended with locked-on-SMP {set,clear}_{tsk_,}thread_flag()
operations?

They should all just be plain C bitops (&, |, ^, etc).

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Mikael Pettersson]

I wrote:
>On Tue, 4 Jul 2006 01:14:13 -0700, Stephane Eranian wrote:
>>On Tue, Jul 04, 2006 at 09:51:49AM +0200, Arjan van de Ven wrote:
>>> > -		}
>>> > -	}
>>> > +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
>>> > +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
>>> > +		__switch_to_xtra(prev_p, next_p, tss);
>>> 
>>> well isn't this replacing an if() (which isn't cheap but also not
>>> expensive, due to unlikely()) with an atomic operation (which *is*
>>> expensive) ?
>>> 
>>Andi is right. I double checked the test_tsk_thread_flag() and it does not
>>use atomic ops.
>
>The test_tsk_thread_flag() does not, but what about all the
>other places in the patch where currently unsychronised loads
>or stores of ->io_bitmap_ptr or ->debugreg7 get replaced or
>extended with locked-on-SMP {set,clear}_{tsk_,}thread_flag()
>operations?
>
>They should all just be plain C bitops (&, |, ^, etc).

Scratch that. You're stuffing bits in the ->flags field which
already is accessed concurrently and thus synchronised. Mea culpa.
As long as the accesses in __switch_to() aren't locked I'm OK with it.

Re: [PATCH 1/2] x86-64 TIF flags for debug regs and io bitmap in ctxsw

[Andi Kleen]

On Tuesday 04 July 2006 11:05, Mikael Pettersson wrote:
> On Tue, 4 Jul 2006 01:14:13 -0700, Stephane Eranian wrote:
> >On Tue, Jul 04, 2006 at 09:51:49AM +0200, Arjan van de Ven wrote:
> >> > -		}
> >> > -	}
> >> > +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> >> > +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> >> > +		__switch_to_xtra(prev_p, next_p, tss);
> >> 
> >> well isn't this replacing an if() (which isn't cheap but also not
> >> expensive, due to unlikely()) with an atomic operation (which *is*
> >> expensive) ?
> >> 
> >Andi is right. I double checked the test_tsk_thread_flag() and it does not
> >use atomic ops.
> 
> The test_tsk_thread_flag() does not, but what about all the
> other places in the patch where currently unsychronised loads
> or stores of ->io_bitmap_ptr or ->debugreg7 get replaced or
> extended with locked-on-SMP {set,clear}_{tsk_,}thread_flag()
> operations?

They are all slow paths where it doesn't matter.

-Andi