Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
}
share|improve this question
    
How are TRUE and FALSE defined? Unless they have weird definitions (and I’d question that), their use in the above code is totally redundant. You can simply write return mutex->owner == NULL; etc. No need for the conditional. –  Konrad Rudolph Aug 9 '12 at 10:08
    
thanks, will do that, TRUE and FALSE are defined as 1 and 0 respectively in kernel.h –  Shantanu Aug 9 '12 at 15:16
    
If it's uniprocessor you can just disable interrupts. –  asveikau Sep 8 '12 at 6:53
    
Isn't that a spinlock implementation for UP ? –  Shantanu Sep 9 '12 at 6:16

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

share|improve this answer
    
1) My atomic function doesn't return till success, there are _try and _timeout modifiers if you can't wait. 2) I am not using atomic for lock, it is for marking the mutex busy or ready. 3) unlikely, likely, restrict are just keywords to let compiler know more about your code and can also tell the person reading your code what you're expecting and atleast on ARM it can have performance impact by saving a branch which can stall the pipeline for few cycles. Apart from that, I've mentioned "I've tested it on a Cortex-A8 (Uni Processor) cpu" –  Shantanu Aug 7 '12 at 13:32
    
I've edited the post, It's a work in progress so wait some time, this one is not the copy which worked. 1) by freed it means freeing the memory where it's allocated, maybe you meant "release" which is legal without using deinit 2) It is just for future purpose, I plan on expanding the code. 3) You're more welcome to ask any question you have about code, commenting it was a big issue since i need to manually indent each line with 4 spaces to mark as code. –  Shantanu Aug 9 '12 at 8:36
    
4) I don't understand you, atomic functions don't wait for mutex availability, it keeps retrying till it is able to spot "MUTX_READY" and then atomically swap it with "MUTX_BUSY" and from then on any other atomic function keeps looping till it finds 'MUTX_READY' again. Yes, it should but if the lock is free it will have no effect so there's a check for owner if you read the code. keep waiting till mutex is released and the queue is empty and hence there are no clients depending on this mutex and it can be freed. there is no MUTX_LOCKED and MUTX_SUCESS are return codes if you look closely. –  Shantanu Aug 9 '12 at 8:38
    
MUTX_DEAD marks a deinit mutex so functions will not end in infinite loop if called on a deinit mutex (not yet uploaded the code). BUSY -> mutex data is being worked on, saving us a critical section and interrupt latency. again, they are not flags, they're return codes..... LOCKED means the _try failed since it was already locked, BUSY means the mutex data is being exclusively worked on by some other function, so two mutex can be processed in multiple threads unlike when using a critical section. I just wrote timeout, wait for the next version Seriously ? read 2nd line in mutex_release... –  Shantanu Aug 9 '12 at 8:42
    
I am writing this for my own kernel (And I'm sure i could re-use this in future) which i'm going to submit as my thesis, well it's a long time since I'm not even in a college right now, Oh and please try to break things down in comments, It's pain typing multiple comments especially since these are void of text formatting. –  Shantanu Aug 9 '12 at 9:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.