question on asymmetric mtx_[un]lock_sleep

[Marc Lörner]

On Tuesday 09 September 2008 21:38, John Baldwin wrote:
> On Thursday 04 September 2008 08:00:04 am Marc Lörner wrote:
> > Hello,
> > I just read through the code of mutexes and turnstiles
> > and it seems to me that _mtx_lock_sleep and _mtx_unlock_sleep
> > are some kind of asymmetric when turning SMP and adaptive mutexes
> > on in kernel-configuration.
> >
> > On locking the mutex, we try to "quick" obtain the lock.
> > If we can't do this, we look, whether some other thread, that's running,
> > holds the lock and spin until either lock is freed or thread is not
> > running anymore. In that case we try again to obtain the lock "quick".
> > If the thread only stopped running but still holds the lock, we use
>
> turnstiles
>
> > to wake us up, when the thread unlocks the mutex.
> > => That seems to be fine and quite symmetric with _mtx_unlock_sleep!!
> >
> > But if we're spinning and the other thread gave the mutex free,
> > we quick-lock the mutex and don't set up a turnstile.
> >
> > Now on mtx_unlock_sleep:
> > - in FreeBSD6/until revision 1.200 turnstiles were tested on existence.
> >   => if turnstile_lookup return NULL we only released the lock quick.
> >
> > - But now, it's never tested if turnstile exists instead we
> > broadcast/wakeup all threads pending on the turnstile. If this turnstile
> > is NULL => we
>
> access
>
> >   wrong memory.
> >
> > Now my question is: Why can we be sure (in new source) that
> > turnstile_lookup always returns a valid pointer to an turnstile and can
> > use returned pointer to call turnstile_broadcast? Am I missing something?
> >
> > Because it seems that following scenario may occur:
> > - on locking same scenario as above (=> thread1 now holds the lock)
> > - thread1 is put off the runqueue
> > - thread2 now tries to quick unlock mutex and sees that thread1 holds it
> > => call to mtx_unlock_sleep
> > - now we try to use turnstile-mechanism and call turnstile_lookup =>
> > returns NULL because no thread waits for wakeup => we call
> > turnstile_broadcast and crash.
>
> Newer locks don't set the CONTESTED flag unless they are actually going to
> go to sleep.  If they succeed in setting CONTESTED or it is already set
> when they test for it, then they will block on the turnstile.  The
> turnstile chain lock will prevent a concurrent unlock from grabbing the
> turnstile until the blocking thread is fully asleep.

I see the setting of CONTESTED in case of sleeping in mtx_lock_sleep!
But there is still the possibility that mtx_lock_sleep "just" obtains the lock 
and doesn't set contested-bit! See described case above (we reach first 
continue in mtx_lock_sleep and test on obtain_lock ends while-loop).

Why is this bit not tested in mtx_unlock_sleep?

I think, it's still possible that turnstile_lookup returns NULL.
And we still have "MPASS(ts != NULL);" in mtx_unlock_sleep that is not
turned on for GENERIC-kernel (no INVARIANTS support).

And I'm still wondering why the former test on "ts != NULL" went away?

Regards,
Marc Loerner