* [PATCH] preemptive kernel behavior change: don't be rude
2002-04-04 23:55 Patch: linux-2.5.8-pre1/kernel/exit.c change caused BUG() at boot time Linus Torvalds
@ 2002-04-05 0:03 ` Robert Love
2002-04-05 1:51 ` george anzinger
0 siblings, 1 reply; 5+ messages in thread
From: Robert Love @ 2002-04-05 0:03 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andrew Morton, george, linux-kernel
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 */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] preemptive kernel behavior change: don't be rude
2002-04-05 0:03 ` [PATCH] preemptive kernel behavior change: don't be rude Robert Love
@ 2002-04-05 1:51 ` george anzinger
2002-04-05 2:06 ` Robert Love
0 siblings, 1 reply; 5+ messages in thread
From: george anzinger @ 2002-04-05 1:51 UTC (permalink / raw)
To: Robert Love, Andrew Morton, george, linux-kernel, Linus Torvalds
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] preemptive kernel behavior change: don't be rude
2002-04-05 1:51 ` george anzinger
@ 2002-04-05 2:06 ` Robert Love
0 siblings, 0 replies; 5+ messages in thread
From: Robert Love @ 2002-04-05 2:06 UTC (permalink / raw)
To: george anzinger; +Cc: Andrew Morton, linux-kernel, Linus Torvalds
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] preemptive kernel behavior change: don't be rude
[not found] <Pine.LNX.4.33.0204041740220.7731-100000@penguin.transmeta.com>
@ 2002-04-05 3:09 ` Robert Love
2002-04-05 20:03 ` Roger Larsson
0 siblings, 1 reply; 5+ messages in thread
From: Robert Love @ 2002-04-05 3:09 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andrew Morton, George Anzinger, lkml
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 */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] preemptive kernel behavior change: don't be rude
2002-04-05 3:09 ` [PATCH] preemptive kernel behavior change: don't be rude Robert Love
@ 2002-04-05 20:03 ` Roger Larsson
0 siblings, 0 replies; 5+ messages in thread
From: Roger Larsson @ 2002-04-05 20:03 UTC (permalink / raw)
To: Robert Love; +Cc: lkml
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2002-04-05 20:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <Pine.LNX.4.33.0204041740220.7731-100000@penguin.transmeta.com>
2002-04-05 3:09 ` [PATCH] preemptive kernel behavior change: don't be rude Robert Love
2002-04-05 20:03 ` Roger Larsson
2002-04-04 23:55 Patch: linux-2.5.8-pre1/kernel/exit.c change caused BUG() at boot time Linus Torvalds
2002-04-05 0:03 ` [PATCH] preemptive kernel behavior change: don't be rude Robert Love
2002-04-05 1:51 ` george anzinger
2002-04-05 2:06 ` Robert Love
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox