simple custom made mutex failing

Question

Can you spot the error in the code? tickets ends up going below 0 causing long stalls.

struct SContext {
    volatile unsigned long* mutex;
    volatile long* ticket;
    volatile bool* done;
};

static unsigned int MyThreadFunc(SContext* ctxt) {

    // -- keep going until we signal for thread to close
    while(*ctxt->done == false) {

        while(*ctxt->ticket) { // while we have tickets waiting
            unsigned int lockedaquired = 0;
            do {
                if(*ctxt->mutex == 0) { // only try if someone doesn't have mutex locked
                    // -- if the compare and swap doesn't work then the function returns
                    // -- the value it expects
                    lockedaquired = InterlockedCompareExchange(ctxt->mutex, 1, 0);
                }
            } while(lockedaquired !=  0); // loop while we didn't aquire lock
            // -- enter critical section

            // -- grab a ticket
            if(*ctxt->ticket > 0);
                     (*ctxt->ticket)--;

            // -- exit critical section
            *ctxt->mutex = 0; // release lock
        }
     }

     return 0;
}

Calling function waiting for threads to finish

    for(unsigned int loops = 0; loops < eLoopCount; ++loops) {
        *ctxt.ticket = eNumThreads; // let the threads start!

        // -- wait for threads to finish
        while(*ctxt.ticket != 0)
            ; 
    }
    done = true;

EDIT:

The answer to this question is simple and unfortunately after I spent the time trimming down the example to post a simplified version I immediately find the answer after I post the question. Sigh..

I initialize lockaquired to 0. Then as an optimization to not take up bus bandwith I don't do the CAS if the mutex is taken.

Unfortunately, in that case where the lock is taken the while loop will let the second thread through!

Sorry for the extra question. I thought I didn't understand windows low level synchronization primitives but really I just had a simple mistake.

Answer 1

I see another race in your code: One thread can cause *ctxt.ticket to hit 0, allowing the parent loop to go back and re-set *ctxt.ticket = eNumThreads without holding *ctxt.mutex. Some other thread may already now hold the mutex (in fact, it probably does) and operate on *ctxt.ticket. For your simplified example this only prevents "batches" from being cleanly separated, but if you had more complex initialization (as in more complex than a single word write) at the top of the loops loop you could see strange behavior.