cv_timedwait() & exiting procs
Julian Elischer
julian at elischer.org
Mon Mar 31 13:52:00 PST 2003
(sounds of all massive context switch while head reloads)
On Mon, 31 Mar 2003, John Baldwin wrote:
>
> On 31-Mar-2003 Andrew Gallatin wrote:
> >
> >
> > FreeBSD's cv_timedwait() function helpfully notices that a process is
> > exiting and returns EWOULDBLOCK if it is.
> >
> > However, if you call cv_timedwait() in the context of a process which
> > is already exiting, you always get back EWOULDBLOCK, regardless of
> > whether or not the timeout expired. Similarly for the cv_wait_sig()
> > and cv_timedwait_sig(), except they set EINTR.
> >
> > Does anyone else consider this behaviour to be a bug? I think it
> > should only return EWOULDBLOCK/EINTR because a process is exiting if
> > the process wasn't already exiting when it entered the cv_*wait*
> > routine, but perhaps I'm misguided...
OK, the thing we are trying to take into account is the fact that a
multithreaded program may call exit() in one thread while another thread
is in, or about to enter a sleep (either the CV-kind or the normal one).
In that situation the thread calling exit() sets a 'single threading'
condition in the process and tries to wake up all threads of that
process that are in interruptible sleeps. it then (assuming that it is
not the only remaining thread already) suspends itself and awaits the
other threads to suicide. (no point in letting then go back to userland
as it is calling exit()..) Thus it is pointless continuing the syscall
so if we are exiting, we always abort the syscall.
The sleeps are probably called from some function that can cope with
signals and will (we hope) unwind correctly on receiving one. One would
think that it would return EINTR in this case. When it unwinds to the
user boundary, it will notice that the process is exiting and fall on
its sword.
Some comments to your thoughts.. It would be a valid thought to allow
the syscall to complete if the sleep completed. but why? Also, Maybe the
test should also test if we are:
1/ threading
and
2/ not the thread that is actually DOING the exit()
(though exit doesn't do any sleeps that I KNOW of)
>
> I think you are referring to these changes?
>
> revision 1.23
> date: 2002/06/29 17:26:18; author: julian; state: Exp; lines: +76 -13
> Part 1 of KSE-III
>
> The ability to schedule multiple threads per process
> (one one cpu) by making ALL system calls optionally asynchronous.
> to come: ia64 and power-pc patches, patches for gdb, test program (in tools)
>
> Reviewed by: Almost everyone who counts
> (at various times, peter, jhb, matt, alfred, mini, bernd,
> and a cast of thousands)
>
> NOTE: this is still Beta code, and contains lots of debugging stuff.
> expect slight instability in signals..
>
> Things like:
>
> @@ -293,6 +328,8 @@
> rval = ERESTART;
> }
> PROC_UNLOCK(p);
> + if (p->p_flag & P_WEXIT)
> + rval = EINTR;
>
> #ifdef KTRACE
> if (KTRPOINT(td, KTR_CSW))
> @@ -363,6 +400,8 @@
> mi_switch();
> }
>
> + if (td->td_proc->p_flag & P_WEXIT)
> + rval = EWOULDBLOCK;
> mtx_unlock_spin(&sched_lock);
> #ifdef KTRACE
> if (KTRPOINT(td, KTR_CSW))
> @@ -450,6 +488,9 @@
> }
> PROC_UNLOCK(p);
>
> + if (p->p_flag & P_WEXIT)
> + rval = EINTR;
> +
> #ifdef KTRACE
> if (KTRPOINT(td, KTR_CSW))
> ktrcsw(0, 0);
For the life of me I can't see why I thought it needed to return
EWOULDBLOCK in that case, unless there is something special about
EWOULDBLOCK processing further downstream. Hmm looks like the original
code is in the non-interruptable case and responds with EWOULDBLOCK if
it times out so I just did the same. Assuming that the caller would know
how to deal with that..
In fact this is the WRONG THING to do.. the testr should be removed
becasue returning early may cause the caller to do things like
reset hardware and enter timeout routines.. better to let it complete
So, what happens in msleep()?
if (p->p_flag & P_THREADED) {
/*
* Just don't bother if we are exiting
* and not the exiting thread or thread was marked as
* interrupted.
*/
if (catch &&
(((p->p_flag & P_WEXIT) && (p->p_singlethread !=
td)) ||
(td->td_flags & TDF_INTERRUPT))) {
td->td_flags &= ~TDF_INTERRUPT;
return (EINTR);
}
}
this is just before we lock the schedlock (and that may be a bug..
maybe it should be after).. but kkkbefore the mi_switch().
After the mi_switch() there is:
if (td->td_flags & TDF_TIMEOUT) {
td->td_flags &= ~TDF_TIMEOUT;
if (sig == 0)
rval = EWOULDBLOCK;
} else if (td->td_flags & TDF_TIMOFAIL) {
td->td_flags &= ~TDF_TIMOFAIL;
} else if (timo && callout_stop(&td->td_slpcallout) == 0) {
/*
* This isn't supposed to be pretty. If we are here,
then
* the endtsleep() callout is currently executing on
another
* CPU and is either spinning on the sched_lock or will
be
* soon. If we don't synchronize here, there is a
chance
* that this process may msleep() again before the
callout
* has a chance to run and the callout may end up waking
up
* the wrong msleep(). Yuck.
*/
TD_SET_SLEEPING(td);
p->p_stats->p_ru.ru_nivcsw++;
mi_switch();
td->td_flags &= ~TDF_TIMOFAIL;
}
if ((td->td_flags & TDF_INTERRUPT) && (priority & PCATCH) &&
(rval == 0)) {
td->td_flags &= ~TDF_INTERRUPT;
rval = EINTR;
}
mtx_unlock_spin(&sched_lock);
if (rval == 0 && catch) {
PROC_LOCK(p);
/* XXX: shouldn't we always be calling cursig() */
if (sig != 0 || (sig = cursig(td))) {
if (SIGISMEMBER(p->p_sigacts->ps_sigintr, sig))
rval = EINTR;
else
rval = ERESTART;
}
PROC_UNLOCK(p);
}
We seem to rely on the fact that if another thread aborted us, we should
end up returning EINTR anyhow..
But that will not be true unless TDF_INTERRUPT is also set.
so that looks a bit like a bug to me...
The result would be that the caller will get returned to early, but if
it is written well, it should call msleep again, which will then return
EINTR, so its probably benign.
I think there are bugs in both msleep and cv_XXX
in msleep, I think it may return to the caller without letting the
caller know that it was aborted, and in cv_xxx it may return to the
caller having aborted in a non threaded app
which would be 'bad' and it may also return a bad result in the
case of a non-interruptable wait rather than letting
it return it's result.
BTW
I think it would be more readble if the cv_wait stuff were consulidated
like the msleep() code.. one gets confused following all those
twisty little tunnels.... (*)
>
> I guess this has something to do with single threading? If so it
> seems broken since it should only affect the singlethreading case
> while you are actually doing singlethreading. Maybe Julian can help
> fix it?
>
> --
>
> John Baldwin <jhb at FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
> "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
> _______________________________________________
> freebsd-arch at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe at freebsd.org"
>
More information about the freebsd-arch
mailing list