Messages in this thread | | | Date | Sat, 17 Nov 2007 19:38:35 +0300 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2/3] wait_task_stopped: tidy up the noreap case |
| |
On 11/16, Roland McGrath wrote: > > This is good, but not quite enough. The original intent behind having the > test was never to return mismatched stale/fresh data. (Not that it ever > really worked as intended.) That is, it's fine if the task has woken up > and done other things while WNOWAIT reports it as stopped--that's stale > data, but it just means the waitid call happened "before" the resumption. > However, it should not report anything that could not possibly have been > true before the resumption. i.e. a changed exit_code, which now means an > normal termination status or a death signal, not the stop signal. This > also applies to the uid, in case the thread called setuid upon resuming > (and even to ptracedness, not that that one really matters). (It doesn't > matter for rusage, since that's not really an exact change of state with > reliable ordering anyway.) > > So the setting of uid and why should also move before read_unlock.
Yes I agree, and I also realized this. In fact, I already tried to do this a long ago: http://marc.info/?l=linux-kernel&m=112809846204068, please note that !noreap branch should be changed as well.
This time I'am trying to cleanup (remove) the games with ->exit_state first. I am mostly concerned about 3/3 patch, what do you think about it?
And. Please note that 3/3 removes the "It must also be done with the write lock held to prevent a race with the EXIT_ZOMBIE case" comment. Afaics, we don't need write_lock(tasklist) any longer, we can simplify things further and remove the EGAIN case completely.
However, wait_task_stopped does:
/* move to end of parent's list to avoid starvation */ remove_parent(p); add_parent(p);
That is why we need write_lock(). Is this really so important? Yes, the next do_wait() can find another "interesting" task a bit faster, but only a little bit. wait_task_continued() could be optimized in a same manner...
Also. I think the locking is not complete. {read,write}_lock(tasklist) can't really pin the task in TRACED/STOPPED state. We need ->siglock to ensure that the child can't escape from get_signal_to_deliver() at least, so it can't do exit/setuid/etc. I was going to try to do this later, because this needs nasty changes...
Oh well. OK, we can ignore patches 2-3 for now. I'd like to know your opinion before going further, perhaps I missed something else.
> While you're at it, you could fix the status argument to wait_noreap_copyout. > It should be just exit_code, not the WIFSTOPPED bit format it does now.
OK, unless Scott is going to do this.
Oleg.
- 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/
| |