[lkml]   [2003]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: Fw: 2.5.61 oops running SDET

    On Sat, 15 Feb 2003, Martin J. Bligh wrote:
    > OK, I did the following, which is what I think you wanted, plus Zwane's
    > observation that task_state acquires the task_struct lock (we're the only
    > caller, so I just removed it), but I still get the same panic and this time
    > the box hung.

    Yeah, a closer look shows that the exit path doesn't actually take the
    task lock at all around any of the signal stuff, so the lock protects
    "task->mm", "task->files" and "task->fs", but it does NOT protect
    "task->signal" or "task->sighand" (illogical, but true).

    Oh, damn.

    The core that checks for "p->sighand" takes the tasklist lock for this
    reason (see "collect_sigign_sigcatch()"

    So the choice seems to be either:

    - make the exit path hold the task lock over the whole exit path, not
    just over mm exit.

    - take the "tasklist_lock" over more of "task_sig()" (not just the
    collect_sigign_sigcatch() thing, but the "&p->signal->shared_pending"
    rendering too.

    The latter is a two-liner. The former is the "right thing" for multiple

    The reason I'd _like_ to see the task lock held over _all_ of the fields
    in the exit() path is:

    - right now we actually take it and drop it multiple times in exit. See
    __exit_files(), __exit_fs(), __exit_mm(). Which all take it just to
    serialize setting ot "mm/files/fs" to NULL.

    - task_lock is a nice local lock, no global scalability impact.

    So it really would be much nicer to do something like this in do_exit():

    struct mm_struct *mm;
    struct files_struct *files;
    struct fs_struct *fs;
    struct signal_struct *signal;
    struct sighand_struct *sighand;

    mm = task->mm;
    files = task->files;
    fs = task->fs;
    signal = task->signal;
    sighand = task->sighand;

    task->mm = NULL;
    task->files = NULL;
    task->fs = NULL;
    task->signal = NULL;
    task->sighand = NULL;

    .. actually do the "__exit_mm(task, mm)" etc here ..

    which would make things a lot more readable, and result in us taking the
    lock only _once_ instead of three times, and would properly protect
    "signal" and "sighand" so that the /proc code wouldn't need to take the
    heavily used "tasklist_lock" just to read the signal state for a single

    But fixing up exit to do the above would require several (trivial) calling
    convention changes, like changing

    static inline void __exit_mm(struct task_struct * tsk)
    struct mm_struct *mm = tsk->mm;


    static inline void __exit_mm(struct task_struct * tsk,
    struct mm_struct *mm)

    instead and updatign the callers.

    Is anybody willing to do that (hopefully fairly trivial) fixup and test
    it, or should we go with the stupid "take the 'tasklist_lock'" approach?


    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2005-03-22 13:33    [W:0.021 / U:2.452 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site