Re: [PATCH 1/2] Implement basic sleeping locks for gnumach.

[Samuel Thibault]

Agustina Arzille, on Sun 29 Jan 2017 16:46:45 -0300, wrote:
> >T1 atomic_cas(), gets the mutex
> >T2 fails atomic_cas()
> >T2 simple_lock()
> >T1 atomic_cas() to release the mutex
> >T2 reads KMUTEX_AVAIL in locked_swap
> >T3 atomic_cas(), gets the mutex
> >T2 writes KMUTEX_CONTENDED, gets KMUTEX_AVAIL
> >
> >now T2 believes it has the mutex, while it was given to T3.
> 
> Ahh, that's a good point. We can use atomic_swap in place of locked_swap.
> Do you think that will be enough?

Yes.

There is just one thing I'm a bit afraid of: the code is assuming that
the only reason why the locker thread is awakened is that it was given
the mutex control. Is that guaranteed?

> >Don't use thread_sleep() as a whole: after failing to acquire simply,
> >assert_wait() the event, then use atomic_cas to try to put
> >KMUTEX_CONTENDED, and on success thread_block().
> 
> This I don't see. thread_sleep() calls assert_wait() before unlocking the
> interlock and blocking. I'm not sure how using assert_wait() would improve
> things.

It'd allow to avoid simple_lock() completely on that side. I've been
thinking about avoiding simple_lock() completely, but it's not easy
without modifying thread_block/thread_wakeup_one deeply. So for now
let's keep the simple_lock, and then yes thread_sleep is enough.

> >>+void kmutex_unlock (struct kmutex *mtxp)
> >>+{
> >>+  if (atomic_cas (&mtxp->state, KMUTEX_LOCKED, KMUTEX_AVAIL))
> >>+    /* No waiters - We're done. */
> >>+    return;
> >>+
> >>+  simple_lock (&mtxp->lock);
> >>+
> >>+  if (!thread_wakeup_one ((event_t)mtxp))
> >>+    /* The waiter was interrupted and left - Reset the mutex state. */
> >>+    mtxp->state = KMUTEX_AVAIL;
> >
> >And what about other threads which were waiting on the mutex?  We need
> >to make sure that at least one wakes up to get the mutex, otherwise
> >they'll stay asleep indefinitely.
> 
> I modified thread_wakeup_prim() so that it returns TRUE if it managed
> to wake up at least a thread. So, if it returns false, we can be assert
> that there were no waiters.

The comment above is misleading then: "The waiter was interrupted and
left". Does it mean that *at best* there were some waiters but they were
interrupted?

Samuel