Simple mutex and conditional variable signal in C

Question

I'm new to multithreading in C so I made a toy program that uses a mutex and a conditional variable to communicate between two threads. do_work performs a task every 1 second (task could take longer than 1 second). Is this implementation free from deadlocks and race conditions? Anything else I might be missing?

#include <pthread.h>
#include "errors.h"

pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
int predicate_value = 0;

void *do_work(void *work) {
    int status = 0;
    int i = 0;
    while (1) {
        status = pthread_mutex_lock(&mutex);
        if (status != 0) {
            err_abort(status, "Lock mutex");
        }
        while (predicate_value == 0) {
            printf("%s\n", "waiting");
            status = pthread_cond_wait(&cond, &mutex);
            if (status != 0) {
                err_abort(status, "Wait on condition");
            }
        }
        if (predicate_value != 0) {
            printf("doing some work: %d\n", i);
            ++i;
            predicate_value = 0;
        }
        status = pthread_mutex_unlock(&mutex);
        if (status != 0) {
            err_abort(status, "Unlock mutex");
        }
    }
}

int main () {
    int status = 0;
    pthread_t work_thread_id;
    status = pthread_create(&work_thread_id, NULL, do_work, NULL);
    if (status != 0) {
        err_abort(status, "Create work thread");
    }
    while (1) {
        if (predicate_value == 0) {
            status = pthread_mutex_lock(&mutex);
            if (status != 0) {
                err_abort(status, "Lock mutex main");
            }
            printf("%s\n", "changed value");
            predicate_value = 1;
            status = pthread_cond_signal(&cond);
            if (status != 0) {
                err_abort(status, "signal condition");
            }
            status = pthread_mutex_unlock(&mutex);
            if (status != 0) {
                err_abort(status, "Unlock mutex main");
            }
            printf("%s\n", "time to sleep");            
        }
        sleep(1);
    }
}

Answer 1

Needs volatile keyword

The way your program is written, predicate_value needs to be a volatile int instead of a normal int. This is because you are reading it without locking the mutex in main(), and writing to it in do_work() from another thread.

Another way to avoid the problem is to ensure that you only ever read the variable with the mutex locked.

Answer 2

Here is the updated version as suggested by JS1 without the volatile keyword but making sure the mutex is locked in this case.

#include <pthread.h>
#include "errors.h"

pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
int predicate_value = 0;

void *do_work(void *work) {
    int status = 0;
    int i = 0;
    while (1) {
        status = pthread_mutex_lock(&mutex);
        if (status != 0) {
            err_abort(status, "Lock mutex");
        }
        while (predicate_value == 0) {
            printf("%s\n", "waiting");
            status = pthread_cond_wait(&cond, &mutex);
            if (status != 0) {
                err_abort(status, "Wait on condition");
            }
        }
        if (predicate_value != 0) {
            printf("doing some work: %d\n", i);
            ++i;
            predicate_value = 0;
        }
        status = pthread_mutex_unlock(&mutex);
        if (status != 0) {
            err_abort(status, "Unlock mutex");
        }
    }
}

int main () {
    int status = 0;
    pthread_t work_thread_id;
    status = pthread_create(&work_thread_id, NULL, do_work, NULL);
    if (status != 0) {
        err_abort(status, "Create work thread");
    }
    while (1) {
        status = pthread_mutex_lock(&mutex);
        if (predicate_value == 0) {
            if (status != 0) {
                err_abort(status, "Lock mutex main");
            }
            printf("%s\n", "changed value");
            predicate_value = 1;
            status = pthread_cond_signal(&cond);
            if (status != 0) {
                err_abort(status, "signal condition");
            }
        }
        status = pthread_mutex_unlock(&mutex);
        if (status != 0) {
            err_abort(status, "Unlock mutex main");
        }
        printf("%s\n", "time to sleep");
        sleep(1);
    }
}

Answer 3

I think the conditional

if (predicate_value != 0)

in do_work() is unnecessary. The while loop only exits when predicate_value is non-zero.

We shouldn't have to worry about the content of predicate_value changing between the while and the if. When pthread_cond_wait() returns it locks the mutex so predicate_value is consistent until we unlock the mutex.

From the pthread_cond_wait() docs:

Upon successful return, the mutex shall have been locked and shall be owned by the calling thread.