msleep() on recursivly locked mutexes

Hans Petter Selasky hselasky at c2i.net
Fri Apr 27 05:53:24 UTC 2007


On Thursday 26 April 2007 23:18, Julian Elischer wrote:
> 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.

Yes, but could we factor this code out into a msleep_recurse() function then? 
And the mtx_pickup/mtx_drop functions I would rather see in "kern_synch.c", 
because it is so much faster to change the recurse level, than it is to do a 
while loop.

--HPS


More information about the freebsd-hackers mailing list