Mutex implementation for uniprocessor bare metal embedded OS

Question

I wrote this recently for one of my projects, any error you guys can spot or a feature which could be implemented without eating up resources or some optimisations ? Oh and it isn't meant for Multi cores.

/*******************************************************************************
 * @file    mutex.c
 * @date    31st August 2012
 * @brief   Generic implementation of mutex, read /notes/thread-safety
 ******************************************************************************/

#include <mutex.h>

OS_ERR mutex_acquire_try(mutex_t *mutex)
{
    if(unlikely(cpu_atomic_cmpxchg(&mutex->lock, OS_UNLOCKED, OS_LOCKED) != OS_OKAY))
    {
        if(mutex->owner == current_thread)
            goto recursive;

        return OS_EBUSY;
    }

    mutex->owner = current_thread;

recursive:
    mutex->recnt++;
    return OS_OKAY;
}

static inline OS_ERR mutex_acquire_helper(mutex_t *mutex, time_t timeout)
{
    while(unlikely(cpu_atomic_cmpxchg(&mutex->lock, OS_UNLOCKED, OS_LOCKED) != OS_OKAY))
    {
        if(mutex->owner == current_thread)
            goto recursive;

        if(mutex->lock == OS_DEAD)
            return OS_EINVAL;

        if(current_thread->priority >= mutex->owner->priority)
            thread_boost(mutex->owner);

        if(thread_queue_block(&mutex->queue, timeout) != OS_OKAY)
            return OS_ETIMEOUT;
    }

    mutex->owner = current_thread;

recursive:
    mutex->recnt++;
    return OS_OKAY;
}

OS_ERR mutex_acquire_timeout(mutex_t *mutex, time_t timeout)
{
    return mutex_acquire_helper(mutex, timeout);
}

OS_ERR mutex_acquire(mutex_t *mutex)
{
    return mutex_acquire_helper(mutex, OS_TIME_INFINITE);
}

OS_ERR mutex_release(mutex_t *mutex)
{
    if(mutex->owner != current_thread)
        return OS_EINVAL;

    if(--mutex->recnt)
        goto exit;

    thread_deboost(mutex->owner);
    mutex->owner = NULL;
    cpu_atomic_set(&mutex->lock, OS_UNLOCKED);
    thread_queue_wake_one_now(&mutex->queue);

exit:
    return OS_OKAY;
}

void mutex_init(mutex_t *mutex)
{
    mutex->recnt = 0;
    mutex->owner = NULL;
    thread_queue_init(&mutex->queue);
    cpu_atomic_set(&mutex->lock, OS_UNLOCKED);
}

void mutex_deinit(mutex_t *mutex)
{
    if(mutex->owner == current_thread)
    {
        if(mutex->recnt > 1)
            LOGE("Mutex: Deinit called on recursive lock @ %p\n", mutex);

        mutex_release(mutex);
    }

    do {
        while(thread_queue_count(&mutex->queue))
            thread_yield();
    } while(cpu_atomic_cmpxchg(&mutex->lock, OS_DEAD, OS_UNLOCKED) != OS_OKAY);
}

Answer 1

Does this really work? You call cpu_atomic_cmpxchg without taking any notice of its return value. I guess the function must return true or false (1 or 0 etc) according to whether the lock was already taken or not and it seems likely that you should take notice of that.

Also, if it were my code I'd drop all the unlikely() and likely() calls. For my money, they just obscure the code and I doubt they are necessary.

I haven't read the functions in detail, so maybe I misunderstood the code...

EDIT: Read it again and have some comments:

• Your point 10: "Mutex MUST not be freed without calling mutex_deinit." is strange. Isn't there a difference between freeing the mutex (lock/free is what you normally do, no?) and de-initialising it (which you would do when it is no longer needed, if at all)?

• 'restrict' used in functions with a single parameter has no purpose

• Reviewers and readers would be more likely to understand what is going on if your functions had some commets as to their intended purpose/use/return values etc.

• How do the 'atomic' functions wait for mutex availability?

• Shouldn't mutex_init fail on an already-initialised mutex?

• How do you know when it is safe to deinit a mutex?

• Your mutex flags are confusing. You have DEAD, BUSY, LOCKED, SUCCESS, READY. Seems likely to me that there is some redundancy here. Or conflating state flags and return values.

• What is the point in all those MUTX flags if you assume the value of one of them (in if(unlikely(ret))) - assuming the atomic function returns one.

• What is the difference between BUSY and LOCKED?

• mutex_acquire_timeout doesn't tell the caller whether a timeout occurred! (or whether enquing falied!)

• So if the mutex has an owner already in mutex_acquire_timeout, you put the thread onto a queue, with a timeout. Presumably thread_queue_enqueue returns either when awoken by thread_queue_wake_highest_priority in mutex_release, or when there is a timeout. But you carry on and replace the mutex owner whatever the case...

• Your point 4 says a mutex can only be released by its owner, and yet mutex_release goes ahead and plays with the mutex queue whoever calls it. Also sets state (ie releasing the local lock) before playing with the queue; surely there will be a race condition there.

Sorry, but I don't believe this code 'works' in a meaningful way. Why are you writing these primitives anyway? Why not use an existing kernel? It is generally a mistake to write your own (although I've worked at several that have done, with bad consequences).