Ingo Molnar wrote: > * Kirill Korotaev wrote: >>>the BUG() is useful for all the code that uses next_thread() - you can >>>only do a safe next_thread() iteration if you've locked ->sighand. > >>1. I don't see spin_lock() on p->sighand->siglock in do_task_stat() >>before calling next_thread(). And the check inside next_thread() permits >>only one of the locks to be taken: >> >> if (!spin_is_locked(&p->sighand->siglock) && >> !rwlock_is_locked(&tasklist_lock)) >> >>which is probably wrong, since tasklist_lock is always required! > > It's not 'wrong' in terms of correctness it's simply too restrictive for > no reason. I agree that we should check for the tasklist lock only. that is what I wanted to say :) I removed check for siglock being locked and changed check for sighand != NULL to pid.nr check as we discussed below. >>2. I think the idea of checking sighand is quite obscure. Probably it >>would be better to call pid_alive() for check at such places in proc, >>isn't it? > yeah, it's just as good of a check. So I replaced the check in your patch with pid_alive() one, ok? >>But I would propose to reorganize these checks in next_thread() to >>something like this: >> >>if (!rwlock_is_locked(&tasklist_lock) || p->pids[PIDTYPE_TGID].nr == 0) >> BUG(); >> >>the last check ensures that we are still hashed and this check is more >>straithforward for understanding, agree? > yep - please send a new patch to Andrew. here it is, please review it as well. There are 2 patches here: diff-next_thread (for both linus and 2.6.9-rc1-mm4 trees) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This patch changes obscure BUG() checks in next_thread() with pid checks meaning exactly the same (It checks for task being hashed). Signed-Off-By: Kirill Korotaev diff-task_stat (for 2.6.9-rc1-mm4 tree) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This patch fixes BUG() happening in do_task_stat()->next_thread(), since tsk->sighand can be NULL there. It adds check for pid_alive() in do_task_stat() to prevent thread loop for already unhashed task. Signed-Off-By: Kirill Korotaev Kirill