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

[Samuel Thibault]

Hello,

Agustina Arzille, on Thu 19 Jan 2017 10:00:32 -0500, wrote:
> +/* Atomically compare *PTR with EXP and set it to NVAL if they're equal.
> + * Evaluates to a boolean, indicating whether the comparison was 
> successful.*/
> +#define atomic_cas(ptr, exp, nval)   \
> +  ({   \
> +     typeof(exp) __e = (exp);   \
> +     __atomic_compare_exchange_n ((ptr), &__e, (nval), 0,   \
> +       __ATOMIC_SEQ_CST, __ATOMIC_RELAXED);   \
> +   })
> +
> +/* Atomically exchange the value of *PTR with VAL, evaluating to
> + * its previous value. */
> +#define atomic_swap(ptr, val)   \
> +  __atomic_exchange_n ((ptr), (val), __ATOMIC_SEQ_CST)
> +
> +/* Atomically store VAL in *PTR. */
> +#define atomic_store(ptr, val)   \
> +  __atomic_store_n ((ptr), (val), __ATOMIC_SEQ_CST)

I'd say suffix them with _seq, to express that they have SEQ ordering.
We may later want to define non-SEQ versions.

Actually, for best performance you'd already want to define both the
_acq (for mutex_(try?)lock) and the _rel versions (for mutex_unlock).

> +int kmutex_lock (struct kmutex *mtxp, int interruptible)
> +{
> +  if (atomic_cas (&mtxp->state, KMUTEX_AVAIL, KMUTEX_LOCKED))
> +    /* Unowned mutex - We're done. */
> +    return (0);
> +
> +  /* The mutex is locked. We may have to sleep. */
> +  simple_lock (&mtxp->lock);
> +  if (locked_swap (&mtxp->state, KMUTEX_CONTENDED) == KMUTEX_AVAIL)

But this will not be safe against others using atomic operations on
state. Think about:

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.

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().

> +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.

Samuel