Date: Thu, 26 Apr 2007 14:18:00 -0700 From: Julian Elischer <julian@elischer.org> To: Hans Petter Selasky <hselasky@c2i.net> Cc: freebsd-hackers@freebsd.org Subject: Re: msleep() on recursivly locked mutexes Message-ID: <46311708.5030002@elischer.org> In-Reply-To: <200704262136.33196.hselasky@c2i.net> References: <200704262136.33196.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Hans Petter Selasky wrote:
> Hi,
>
> In the new USB stack I have defined the following:
>
> u_int32_t
> mtx_drop_recurse(struct mtx *mtx)
> {
> u_int32_t recurse_level = mtx->mtx_recurse;
> u_int32_t recurse_curr = recurse_level;
>
> mtx_assert(mtx, MA_OWNED);
>
> while(recurse_curr--) {
> mtx_unlock(mtx);
> }
>
> return recurse_level;
> }
The reason that mutexes ever recurse in the first place is usually because one
piece of code calls itself (or a related piece of code) in a blind manner.. in other
words, it doesn't know it is doing so. The whole concept of recursing mutexes is a bit
gross. The concept of blindly unwinding them is I think just asking for trouble.
Further the idea that holding a mutex "except for when we sleep" is a generally
bright idea is also a bit odd to me.. If you hold a mutex and release it
during sleep you probably should invalidate all assumptions you made during the
period before you slept as whatever you were protecting has possibly been raped
while you slept. I have seen too many instances where people just called msleep
and dropped the mutex they held, picked it up again on wakeup, and then blithely
continued on without checking what happened while they were asleep.
It seems to me that if someone else held that lock for some purpose when you slept,
then you don't know what it was that they were trying to protect, so you don't
know what to recheck when you wake up.. I think this is extremely dangerous as you
don't know when you are in this situation..
Personally I think Matt Dillon had a good idea when he suggested that the
need to sleep when holding a lock should require the lock holder to back out as
far as needed as to be able to see why the lock was held and to comfortably cope
with the situaution when the protected structures got frobbed while it slept.
(He actually at one time committed some primitives to do this. I forget their names
and htey were never used, but the idea has some merrit.)
This change may be OK in the situations you plan but it makes me very uncomfortable.
it basically should have a big sign on it saying "You could spend years looking
for the wierd bugs you are likely to get if you use this" on it.
>
> void
> mtx_pickup_recurse(struct mtx *mtx, u_int32_t recurse_level)
> {
> mtx_assert(mtx, MA_OWNED);
>
> while(recurse_level--) {
> mtx_lock(mtx);
> }
> return;
> }
>
> When I do a msleep() I do it like this:
>
> level = mtx_drop_recurse(ctd->p_mtx);
>
> error = msleep(ctd, ctd->p_mtx, 0,
> "config td sleep", timeout);
>
> mtx_pickup_recurse(ctd->p_mtx, level);
>
> Are there any comments on integrating this functionality into msleep(), and
> adding mtx_drop_recurse() and mtx_pickup_recurse() to the FreeBSD kernel?
>
> --HPS
> _______________________________________________
> freebsd-hackers@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?46311708.5030002>