lkml.org 
[lkml]   [2004]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: 2.6.8.1-mm2
From
Date
On Fri, 2004-08-20 at 18:14, Ingo Molnar wrote:
> * Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> > + /* Delayed cleanup on death of task with uncaring parent. */
> > + struct list_head death;
>
> > +/* Keventd handles tasks whose parent won't ever release_task them. */
> > +static DEFINE_PER_CPU(struct list_head, unreleased_tasks);
> > +static DEFINE_PER_CPU(struct work_struct, release_task_work);
>
> no! this removes the whole performance optimization of self-reaping and
> re-introduces the context-switches. Just measure the
> creation/destruction performance of threads. Strong NACK.

OK, that patch sucked. But this self-reaping optimization has caused
more than one subtle bug; I am determined to get rid of it.

What do you think of this? (against 2.6.8.1-mm2)
Rusty.

Name: Don't Sleep After We're Out Of Task List
Status: Booted on 2.6.8.1-mm2
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (authored)
Version: -mm

Ingo recently accidentally broke CPU hotplug by enabling preemption
around release_task(), which can be called on the current task if the
parent isn't interested. This is because release_task() can now
sleep.

The problem is, the task can be preempted and then the CPU can go
down: it's not in the task list any more, and so it won't get migrated
after the CPU goes down. It stays on the down CPU, which triggers a
BUG_ON.

We have had previous problems with tasks releasing themselves:
oprofile has a comment about it, and we had the case of trying to
deliver SIGXCPU in the timer tick to the current task which had called
release_task(). I tried shuffling release_task off the
finish_arch_switch, but that can't sleep either. I tried using rcu,
but same problem. Finally, I just use a workqueue and a per-cpu list,
which also guarantees the task has actually finished running.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .6239-linux-2.6.8.1-mm2/include/linux/sched.h .6239-linux-2.6.8.1-mm2.updated/include/linux/sched.h
--- .6239-linux-2.6.8.1-mm2/include/linux/sched.h 2004-08-20 17:33:26.000000000 +1000
+++ .6239-linux-2.6.8.1-mm2.updated/include/linux/sched.h 2004-08-21 17:12:34.000000000 +1000
@@ -615,7 +615,6 @@ do { if (atomic_dec_and_test(&(tsk)->usa
/* Not implemented yet, only for 486*/
#define PF_STARTING 0x00000002 /* being created */
#define PF_EXITING 0x00000004 /* getting shut down */
-#define PF_DEAD 0x00000008 /* Dead */
#define PF_SELFREAP 0x00000010 /* Never a zombie, must be released */
#define PF_FORKNOEXEC 0x00000040 /* forked but didn't exec */
#define PF_SUPERPRIV 0x00000100 /* used super-user privileges */
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .6239-linux-2.6.8.1-mm2/kernel/exit.c .6239-linux-2.6.8.1-mm2.updated/kernel/exit.c
--- .6239-linux-2.6.8.1-mm2/kernel/exit.c 2004-08-20 17:33:27.000000000 +1000
+++ .6239-linux-2.6.8.1-mm2.updated/kernel/exit.c 2004-08-21 17:10:52.000000000 +1000
@@ -646,13 +646,14 @@ static inline void forget_original_paren
}
}

+static DEFINE_PER_CPU(struct task_struct *, last_exited_task);
+
/*
* Send signals to all our closest relatives so that they know
* to properly mourn us..
*/
static void exit_notify(struct task_struct *tsk)
{
- int state;
struct task_struct *t;
struct list_head ptrace_dead, *_p, *_n;

@@ -749,18 +750,11 @@ static void exit_notify(struct task_stru
do_notify_parent(tsk, SIGCHLD);
}

- state = TASK_ZOMBIE;
- if (tsk->exit_signal == -1 && tsk->ptrace == 0)
- state = TASK_DEAD;
- tsk->state = state;
-
- /*
- * Clear these here so that update_process_times() won't try to deliver
- * itimer, profile or rlimit signals to this task while it is in late exit.
- */
- tsk->it_virt_value = 0;
- tsk->it_prof_value = 0;
- tsk->rlim[RLIMIT_CPU].rlim_cur = RLIM_INFINITY;
+ if (tsk->exit_signal == -1 && tsk->ptrace == 0) {
+ tsk->state = TASK_DEAD;
+ tsk->flags |= PF_SELFREAP;
+ } else
+ tsk->state = TASK_ZOMBIE;

write_unlock_irq(&tasklist_lock);

@@ -770,12 +764,25 @@ static void exit_notify(struct task_stru
release_task(t);
}

+again:
preempt_disable();
- /* PF_DEAD says drop ref after we schedule. */
- tsk->flags |= PF_DEAD;
- /* PF_SELFREAP says there's no parent to wait4() for us. */
- if (state == TASK_DEAD)
- tsk->flags |= PF_SELFREAP;
+
+ /* Now it's safe to do final put on last task which exited. */
+ t = __get_cpu_var(last_exited_task);
+ if (t) {
+ if (t->flags & PF_SELFREAP) {
+ /* release_task can sleep: release and retry. */
+ __get_cpu_var(last_exited_task) = NULL;
+ preempt_enable();
+ release_task(t);
+ put_task_struct(t);
+ goto again;
+ }
+ put_task_struct(t);
+ }
+
+ /* Once we've set this, we must not be preempted. */
+ __get_cpu_var(last_exited_task) = tsk;
}

asmlinkage NORET_TYPE void do_exit(long code)
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .6239-linux-2.6.8.1-mm2/kernel/sched.c .6239-linux-2.6.8.1-mm2.updated/kernel/sched.c
--- .6239-linux-2.6.8.1-mm2/kernel/sched.c 2004-08-20 17:33:27.000000000 +1000
+++ .6239-linux-2.6.8.1-mm2.updated/kernel/sched.c 2004-08-21 17:01:31.000000000 +1000
@@ -1467,30 +1467,12 @@ static void finish_task_switch(task_t *p
{
runqueue_t *rq = this_rq();
struct mm_struct *mm = rq->prev_mm;
- unsigned long prev_task_flags;

rq->prev_mm = NULL;

- /*
- * A task struct has one reference for the use as "current".
- * If a task dies, then it sets TASK_ZOMBIE in tsk->state and calls
- * schedule one last time. The schedule call will never return,
- * and the scheduled task must drop that reference.
- * The test for TASK_ZOMBIE must occur while the runqueue locks are
- * still held, otherwise prev could be scheduled on another cpu, die
- * there before we look at prev->state, and then the reference would
- * be dropped twice.
- * Manfred Spraul <manfred@colorfullife.com>
- */
- prev_task_flags = prev->flags;
finish_arch_switch(rq, prev);
if (mm)
mmdrop(mm);
- if (unlikely(prev_task_flags & PF_DEAD)) {
- if (prev_task_flags & PF_SELFREAP)
- release_task(prev);
- put_task_struct(prev);
- }
}

/**
--
Anyone who quotes me in their signature is an idiot -- Rusty Russell

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:05    [W:0.086 / U:0.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site