Re: [PATCH] preemptive kernel behavior change: don't be rude

Re: [PATCH] preemptive kernel behavior change: don't be rude

[Robert Love]

Linus and company,

Attached find a final version of the new behavior patch.  It meets
everyone's concerns and it compiles, boots, and survived a basic stress
test here.

The next step would be to push the preempt_count check back into
preempt_schedule and duplicate preempt_schedule in entry.S and then have
it call schedule directly.  This would be both a space and time
optimization.  I will do this, but I'd like to give this as it is some
testing first.

Linus, if this is how you want it, please apply.  Patch is against
2.5.8-pre1.

	Robert Love

diff -urN linux-2.5.8-pre1/arch/i386/kernel/entry.S linux/arch/i386/kernel/entry.S
--- linux-2.5.8-pre1/arch/i386/kernel/entry.S	Wed Apr  3 20:57:24 2002
+++ linux/arch/i386/kernel/entry.S	Thu Apr  4 20:44:40 2002
@@ -240,9 +240,7 @@
 	jnz restore_all
 	incl TI_PRE_COUNT(%ebx)
 	sti
-	movl TI_TASK(%ebx), %ecx		# ti->task
-	movl $0,(%ecx)			# current->state = TASK_RUNNING
-	call SYMBOL_NAME(schedule)
+	call SYMBOL_NAME(preempt_schedule)
 	jmp ret_from_intr
 #endif
 
diff -urN linux-2.5.8-pre1/arch/i386/kernel/ptrace.c linux/arch/i386/kernel/ptrace.c
--- linux-2.5.8-pre1/arch/i386/kernel/ptrace.c	Wed Apr  3 20:57:24 2002
+++ linux/arch/i386/kernel/ptrace.c	Thu Apr  4 20:44:40 2002
@@ -455,11 +455,9 @@
 	   between a syscall stop and SIGTRAP delivery */
 	current->exit_code = SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
 					? 0x80 : 0);
-	preempt_disable();
 	current->state = TASK_STOPPED;
 	notify_parent(current, SIGCHLD);
 	schedule();
-	preempt_enable();
 	/*
 	 * this isn't the same as continuing with a signal, but it will do
 	 * for normal use.  strace only continues with a signal if the
diff -urN linux-2.5.8-pre1/arch/i386/kernel/signal.c linux/arch/i386/kernel/signal.c
--- linux-2.5.8-pre1/arch/i386/kernel/signal.c	Wed Apr  3 20:57:24 2002
+++ linux/arch/i386/kernel/signal.c	Thu Apr  4 20:44:40 2002
@@ -610,11 +610,9 @@
 		if ((current->ptrace & PT_PTRACED) && signr != SIGKILL) {
 			/* Let the debugger run.  */
 			current->exit_code = signr;
-			preempt_disable();
 			current->state = TASK_STOPPED;
 			notify_parent(current, SIGCHLD);
 			schedule();
-			preempt_enable();
 
 			/* We're back.  Did the debugger cancel the sig?  */
 			if (!(signr = current->exit_code))
@@ -669,14 +667,12 @@
 
 			case SIGSTOP: {
 				struct signal_struct *sig;
+				current->state = TASK_STOPPED;
 				current->exit_code = signr;
 				sig = current->parent->sig;
-				preempt_disable();
-				current->state = TASK_STOPPED;
 				if (sig && !(sig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDSTOP))
 					notify_parent(current, SIGCHLD);
 				schedule();
-				preempt_enable();
 				continue;
 			}
 
diff -urN linux-2.5.8-pre1/include/linux/sched.h linux/include/linux/sched.h
--- linux-2.5.8-pre1/include/linux/sched.h	Wed Apr  3 20:57:27 2002
+++ linux/include/linux/sched.h	Thu Apr  4 21:56:11 2002
@@ -91,6 +91,7 @@
 #define TASK_UNINTERRUPTIBLE	2
 #define TASK_ZOMBIE		4
 #define TASK_STOPPED		8
+#define PREEMPT_ACTIVE		0x4000000
 
 #define __set_task_state(tsk, state_value)		\
 	do { (tsk)->state = (state_value); } while (0)
diff -urN linux-2.5.8-pre1/include/linux/spinlock.h linux/include/linux/spinlock.h
--- linux-2.5.8-pre1/include/linux/spinlock.h	Wed Apr  3 20:57:34 2002
+++ linux/include/linux/spinlock.h	Thu Apr  4 21:56:11 2002
@@ -177,8 +177,9 @@
 do { \
 	--current_thread_info()->preempt_count; \
 	barrier(); \
-	if (unlikely(test_thread_flag(TIF_NEED_RESCHED))) \
-		preempt_schedule(); \
+	if (unlikely(!(current_thread_info()->preempt_count) && \
+		test_thread_flag(TIF_NEED_RESCHED))) \
+			preempt_schedule(); \
 } while (0)
 
 #define spin_lock(lock)	\
diff -urN linux-2.5.8-pre1/kernel/exit.c linux/kernel/exit.c
--- linux-2.5.8-pre1/kernel/exit.c	Wed Apr  3 20:57:37 2002
+++ linux/kernel/exit.c	Thu Apr  4 21:55:29 2002
@@ -499,6 +495,7 @@
 	acct_process(code);
 	__exit_mm(tsk);
 
+	preempt_disable();
 	sem_exit();
 	__exit_files(tsk);
 	__exit_fs(tsk);
diff -urN linux-2.5.8-pre1/kernel/sched.c linux/kernel/sched.c
--- linux-2.5.8-pre1/kernel/sched.c	Wed Apr  3 20:57:37 2002
+++ linux/kernel/sched.c	Thu Apr  4 20:54:17 2002
@@ -764,6 +764,13 @@
 	prev->sleep_timestamp = jiffies;
 	spin_lock_irq(&rq->lock);
 
+	/*
+	 * if entering from preempt_schedule, off a kernel preemption,
+	 * go straight to picking the next task.
+	 */
+	if (unlikely(preempt_get_count() & PREEMPT_ACTIVE))
+		goto pick_next_task;
+
 	switch (prev->state) {
 	case TASK_INTERRUPTIBLE:
 		if (unlikely(signal_pending(prev))) {
@@ -775,9 +782,7 @@
 	case TASK_RUNNING:
 		;
 	}
-#if CONFIG_SMP
 pick_next_task:
-#endif
 	if (unlikely(!rq->nr_running)) {
 #if CONFIG_SMP
 		load_balance(rq, 1);
@@ -841,10 +846,12 @@
  */
 asmlinkage void preempt_schedule(void)
 {
-	if (unlikely(preempt_get_count()))
-		return;
-	current->state = TASK_RUNNING;
-	schedule();
+	do {
+		current_thread_info()->preempt_count += PREEMPT_ACTIVE;
+		schedule();
+		current_thread_info()->preempt_count -= PREEMPT_ACTIVE;
+		barrier();
+	} while(test_thread_flag(TIF_NEED_RESCHED));
 }
 #endif /* CONFIG_PREEMPT */
 

Re: [PATCH] preemptive kernel behavior change: don't be rude

[Roger Larsson]

Hi Robert,

This does not look symmetrical, add and sub and bitops...

> +#define PREEMPT_ACTIVE		0x4000000


> +	/*
> +	 * if entering from preempt_schedule, off a kernel preemption,
> +	 * go straight to picking the next task.
> +	 */
> +	if (unlikely(preempt_get_count() & PREEMPT_ACTIVE))
> +		goto pick_next_task;
> +


> +	do {
> +		current_thread_info()->preempt_count += PREEMPT_ACTIVE;
> +		schedule();
> +		current_thread_info()->preempt_count -= PREEMPT_ACTIVE;
> +		barrier();

And since it has to be zero to end up in the routine anyway...

asmlinkage void preempt_schedule(void)
 {
	BUG_ON(current_thread_info()->preempt_count != 0);

	do {
		/* Problem: suppose a new interrupt happens before we got to set
		 * preempt_count... then we will call this routine recursively.
		 * innermost will select the correct process, but wont it be scheduled
		 * away in enclosing preempt_schedule() - schedule() will be called
		 * with PREEMPT_ACTIVE but not TIF_NEED_RESCHED...
		 * (goto pick_next_task) */

		current_thread_info()->preempt_count = PREEMPT_ACTIVE;

		/* interrupts here might cause calls to preempt_schedule() but those will
		   bounce off above since preempt_count != 0 */

		schedule();
		current_thread_info()->preempt_count = 0;

		/* interrupt here will possibly preempt the right process - no problem */

		/* need to check if any interrupt happened during the preemption off time,
		 * this case is in fact unlikely  */
	while (unlikely(test_thread_flag(TIF_NEED_RESCHED)));
}

/RogerL

-- 
Roger Larsson
Skellefteå
Sweden

Re: Patch: linux-2.5.8-pre1/kernel/exit.c change caused BUG() at boot time

[Linus Torvalds]

On 4 Apr 2002, Robert Love wrote:
> 
> Hm, how so?  I contend not to rudely set the task state but instead mark
> the task as "preempted" in preempt_schedule and handle this case in
> schedule.

Ahh, I misunderstood - I thought you meant setting the bit when setting 
current->state.

Fair enough. Send me a patch to look at.

		Linus

[PATCH] preemptive kernel behavior change: don't be rude

[Robert Love]

On Thu, 2002-04-04 at 18:55, Linus Torvalds wrote:
> Fair enough. Send me a patch to look at.

Linus et all,

Here we go:

- do not manually set task->state
- instead, in preempt_schedule, set a flag in preempt_count that
  denotes that this task is entering schedule off a kernel preemption.
- use this flag in schedule to jump to pick_next_task
- in preempt_schedule, upon return from schedule, unset the flag
- have entry.S just call preempt_schedule and not duplicate this work,
  as Linus suggested.  I agree.  Note this makes debugging easier as
  we keep a single point of entry for kernel preemptions.

The result: we can safely preempt non-TASK_RUNNING tasks.  If one is
preempted, we can safely survive schedule because we won't handle the
special casing of non-TASK_RUNNING at the top of schedule.  Thus other
tasks can run as desired and our non-TASK_RUNNING task will eventually
be rescheduled, in its original state, and complete happily.

This is the behavior we have in the 2.4 patches and 2.5 until
~2.5.6-pre.  This works.  It requires no other changes elsewhere (it
actually removes some special-casing Ingo did in the signal code).

This patch works in theory and compiles, but received minimal testing.

	Robert Love

diff -urN linux-2.5.8-pre1/arch/i386/kernel/entry.S linux/arch/i386/kernel/entry.S
--- linux-2.5.8-pre1/arch/i386/kernel/entry.S	Wed Apr  3 20:57:24 2002
+++ linux/arch/i386/kernel/entry.S	Thu Apr  4 18:32:03 2002
@@ -240,9 +240,7 @@
 	jnz restore_all
 	incl TI_PRE_COUNT(%ebx)
 	sti
-	movl TI_TASK(%ebx), %ecx		# ti->task
-	movl $0,(%ecx)			# current->state = TASK_RUNNING
-	call SYMBOL_NAME(schedule)
+	call SYMBOL_NAME(preempt_schedule)
 	jmp ret_from_intr
 #endif
 
diff -urN linux-2.5.8-pre1/arch/i386/kernel/ptrace.c linux/arch/i386/kernel/ptrace.c
--- linux-2.5.8-pre1/arch/i386/kernel/ptrace.c	Wed Apr  3 20:57:24 2002
+++ linux/arch/i386/kernel/ptrace.c	Thu Apr  4 18:31:17 2002
@@ -455,11 +455,9 @@
 	   between a syscall stop and SIGTRAP delivery */
 	current->exit_code = SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
 					? 0x80 : 0);
-	preempt_disable();
 	current->state = TASK_STOPPED;
 	notify_parent(current, SIGCHLD);
 	schedule();
-	preempt_enable();
 	/*
 	 * this isn't the same as continuing with a signal, but it will do
 	 * for normal use.  strace only continues with a signal if the
diff -urN linux-2.5.8-pre1/arch/i386/kernel/signal.c linux/arch/i386/kernel/signal.c
--- linux-2.5.8-pre1/arch/i386/kernel/signal.c	Wed Apr  3 20:57:24 2002
+++ linux/arch/i386/kernel/signal.c	Thu Apr  4 18:31:00 2002
@@ -610,11 +610,9 @@
 		if ((current->ptrace & PT_PTRACED) && signr != SIGKILL) {
 			/* Let the debugger run.  */
 			current->exit_code = signr;
-			preempt_disable();
 			current->state = TASK_STOPPED;
 			notify_parent(current, SIGCHLD);
 			schedule();
-			preempt_enable();
 
 			/* We're back.  Did the debugger cancel the sig?  */
 			if (!(signr = current->exit_code))
@@ -669,14 +667,12 @@
 
 			case SIGSTOP: {
 				struct signal_struct *sig;
+				current->state = TASK_STOPPED;
 				current->exit_code = signr;
 				sig = current->parent->sig;
-				preempt_disable();
-				current->state = TASK_STOPPED;
 				if (sig && !(sig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDSTOP))
 					notify_parent(current, SIGCHLD);
 				schedule();
-				preempt_enable();
 				continue;
 			}
 
diff -urN linux-2.5.8-pre1/include/linux/sched.h linux/include/linux/sched.h
--- linux-2.5.8-pre1/include/linux/sched.h	Wed Apr  3 20:57:27 2002
+++ linux/include/linux/sched.h	Thu Apr  4 18:29:53 2002
@@ -91,6 +91,7 @@
 #define TASK_UNINTERRUPTIBLE	2
 #define TASK_ZOMBIE		4
 #define TASK_STOPPED		8
+#define PREEMPT_ACTIVE		0x4000000
 
 #define __set_task_state(tsk, state_value)		\
 	do { (tsk)->state = (state_value); } while (0)
diff -urN linux-2.5.8-pre1/kernel/sched.c linux/kernel/sched.c
--- linux-2.5.8-pre1/kernel/sched.c	Wed Apr  3 20:57:37 2002
+++ linux/kernel/sched.c	Thu Apr  4 18:29:24 2002
@@ -764,6 +764,13 @@
 	prev->sleep_timestamp = jiffies;
 	spin_lock_irq(&rq->lock);
 
+	/*
+	 * if entering from preempt_schedule, off a kernel preemption,
+	 * go straight to picking the next task.
+	 */
+	if (unlikely(preempt_get_count() & PREEMPT_ACTIVE))
+		goto pick_next_task;
+	
 	switch (prev->state) {
 	case TASK_INTERRUPTIBLE:
 		if (unlikely(signal_pending(prev))) {
@@ -775,7 +782,7 @@
 	case TASK_RUNNING:
 		;
 	}
-#if CONFIG_SMP
+#if CONFIG_SMP || CONFIG_PREEMPT
 pick_next_task:
 #endif
 	if (unlikely(!rq->nr_running)) {
@@ -843,8 +850,11 @@
 {
 	if (unlikely(preempt_get_count()))
 		return;
-	current->state = TASK_RUNNING;
+
+	current_thread_info()->preempt_count += PREEMPT_ACTIVE;
 	schedule();
+	current_thread_info()->preempt_count -= PREEMPT_ACTIVE;
+	barrier();
 }
 #endif /* CONFIG_PREEMPT */
 

Re: [PATCH] preemptive kernel behavior change: don't be rude

[george anzinger]

Robert Love wrote:
> 
> On Thu, 2002-04-04 at 18:55, Linus Torvalds wrote:
> > Fair enough. Send me a patch to look at.
> 
> Linus et all,
> 
> Here we go:
> 
> - do not manually set task->state
> - instead, in preempt_schedule, set a flag in preempt_count that
>   denotes that this task is entering schedule off a kernel preemption.
> - use this flag in schedule to jump to pick_next_task
> - in preempt_schedule, upon return from schedule, unset the flag
> - have entry.S just call preempt_schedule and not duplicate this work,
>   as Linus suggested.  I agree.  Note this makes debugging easier as
>   we keep a single point of entry for kernel preemptions.
> 
> The result: we can safely preempt non-TASK_RUNNING tasks.  If one is
> preempted, we can safely survive schedule because we won't handle the
> special casing of non-TASK_RUNNING at the top of schedule.  Thus other
> tasks can run as desired and our non-TASK_RUNNING task will eventually
> be rescheduled, in its original state, and complete happily.
> 
> This is the behavior we have in the 2.4 patches and 2.5 until
> ~2.5.6-pre.  This works.  It requires no other changes elsewhere (it
> actually removes some special-casing Ingo did in the signal code).
> 
> This patch works in theory and compiles, but received minimal testing.
> 
>         Robert Love
> 
> diff -urN linux-2.5.8-pre1/arch/i386/kernel/entry.S linux/arch/i386/kernel/entry.S
> --- linux-2.5.8-pre1/arch/i386/kernel/entry.S   Wed Apr  3 20:57:24 2002
> +++ linux/arch/i386/kernel/entry.S      Thu Apr  4 18:32:03 2002
> @@ -240,9 +240,7 @@
>         jnz restore_all
>         incl TI_PRE_COUNT(%ebx)
>         sti
> -       movl TI_TASK(%ebx), %ecx                # ti->task
> -       movl $0,(%ecx)                  # current->state = TASK_RUNNING
> -       call SYMBOL_NAME(schedule)
> +       call SYMBOL_NAME(preempt_schedule)
>         jmp ret_from_intr
>  #endif
> 
> diff -urN linux-2.5.8-pre1/arch/i386/kernel/ptrace.c linux/arch/i386/kernel/ptrace.c
> --- linux-2.5.8-pre1/arch/i386/kernel/ptrace.c  Wed Apr  3 20:57:24 2002
> +++ linux/arch/i386/kernel/ptrace.c     Thu Apr  4 18:31:17 2002
> @@ -455,11 +455,9 @@
>            between a syscall stop and SIGTRAP delivery */
>         current->exit_code = SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
>                                         ? 0x80 : 0);
> -       preempt_disable();
>         current->state = TASK_STOPPED;
>         notify_parent(current, SIGCHLD);
>         schedule();
> -       preempt_enable();
>         /*
>          * this isn't the same as continuing with a signal, but it will do
>          * for normal use.  strace only continues with a signal if the
> diff -urN linux-2.5.8-pre1/arch/i386/kernel/signal.c linux/arch/i386/kernel/signal.c
> --- linux-2.5.8-pre1/arch/i386/kernel/signal.c  Wed Apr  3 20:57:24 2002
> +++ linux/arch/i386/kernel/signal.c     Thu Apr  4 18:31:00 2002
> @@ -610,11 +610,9 @@
>                 if ((current->ptrace & PT_PTRACED) && signr != SIGKILL) {
>                         /* Let the debugger run.  */
>                         current->exit_code = signr;
> -                       preempt_disable();
>                         current->state = TASK_STOPPED;
>                         notify_parent(current, SIGCHLD);
>                         schedule();
> -                       preempt_enable();
> 
>                         /* We're back.  Did the debugger cancel the sig?  */
>                         if (!(signr = current->exit_code))
> @@ -669,14 +667,12 @@
> 
>                         case SIGSTOP: {
>                                 struct signal_struct *sig;
> +                               current->state = TASK_STOPPED;
>                                 current->exit_code = signr;
>                                 sig = current->parent->sig;
> -                               preempt_disable();
> -                               current->state = TASK_STOPPED;
>                                 if (sig && !(sig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDSTOP))
>                                         notify_parent(current, SIGCHLD);
>                                 schedule();
> -                               preempt_enable();
>                                 continue;
>                         }
> 
> diff -urN linux-2.5.8-pre1/include/linux/sched.h linux/include/linux/sched.h
> --- linux-2.5.8-pre1/include/linux/sched.h      Wed Apr  3 20:57:27 2002
> +++ linux/include/linux/sched.h Thu Apr  4 18:29:53 2002
> @@ -91,6 +91,7 @@
>  #define TASK_UNINTERRUPTIBLE   2
>  #define TASK_ZOMBIE            4
>  #define TASK_STOPPED           8
> +#define PREEMPT_ACTIVE         0x4000000
> 
>  #define __set_task_state(tsk, state_value)             \
>         do { (tsk)->state = (state_value); } while (0)
> diff -urN linux-2.5.8-pre1/kernel/sched.c linux/kernel/sched.c
> --- linux-2.5.8-pre1/kernel/sched.c     Wed Apr  3 20:57:37 2002
> +++ linux/kernel/sched.c        Thu Apr  4 18:29:24 2002
> @@ -764,6 +764,13 @@
>         prev->sleep_timestamp = jiffies;
>         spin_lock_irq(&rq->lock);
> 
> +       /*
> +        * if entering from preempt_schedule, off a kernel preemption,
> +        * go straight to picking the next task.
> +        */
> +       if (unlikely(preempt_get_count() & PREEMPT_ACTIVE))
> +               goto pick_next_task;
> +
>         switch (prev->state) {
>         case TASK_INTERRUPTIBLE:
>                 if (unlikely(signal_pending(prev))) {
> @@ -775,7 +782,7 @@
>         case TASK_RUNNING:
>                 ;
>         }
> -#if CONFIG_SMP
> +#if CONFIG_SMP || CONFIG_PREEMPT
>  pick_next_task:
>  #endif
>         if (unlikely(!rq->nr_running)) {
> @@ -843,8 +850,11 @@
>  {
>         if (unlikely(preempt_get_count()))
>                 return;

This line implies that entry.S calls with preempt count of 0.  It use to
call with 1 or was that WAY back there?  

If this is the way it is and it works, then the += and -= below can be
changed to = and the second reference to PREEMPT_ACTIVE becomes 0.

I think I would rather have entry.S set preempt_count to PREEMPT_ACTIVE
with the interrupt system off, turn it on, make the call directly to
schedule(), and then set to zero on return.  I really am concerned with
taking an interrupt during the call, i.e. between the interrupt on and
the store below.  This can lead to stack overflow rather easily.

> -       current->state = TASK_RUNNING;
> +
> +       current_thread_info()->preempt_count += PREEMPT_ACTIVE;
>         schedule();
> +       current_thread_info()->preempt_count -= PREEMPT_ACTIVE;
> +       barrier();
>  }
>  #endif /* CONFIG_PREEMPT */
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [PATCH] preemptive kernel behavior change: don't be rude

[Robert Love]

On Thu, 2002-04-04 at 20:51, george anzinger wrote:

> This line implies that entry.S calls with preempt count of 0.  It use to
> call with 1 or was that WAY back there?  

No you are right, this line needs to be pushed back out to
preempt_enable OR we need to duplicate preempt_schedule in the entry.S
code.  For now, I will just push this back into preempt_enable.

> If this is the way it is and it works, then the += and -= below can be
> changed to = and the second reference to PREEMPT_ACTIVE becomes 0.
> 
> I think I would rather have entry.S set preempt_count to PREEMPT_ACTIVE
> with the interrupt system off, turn it on, make the call directly to
> schedule(), and then set to zero on return.  I really am concerned with
> taking an interrupt during the call, i.e. between the interrupt on and
> the store below.  This can lead to stack overflow rather easily.

Agreed.  This is probably the best way to do it ... for now I'll do it
like above.

	Robert Love