4
\$\begingroup\$

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);
    }
}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ made the change - now I won't need the volatile keyword, right? Also, having a third thread also signal based on the conditional from another method (like in the main method here) shoudn't induce any priority inversion if all threads have the same priority, right? \$\endgroup\$ – user2635088 Dec 7 '16 at 16:30
  • 1
    \$\begingroup\$ volatile has nothing to do with atomic access. The problem of threads using a shared variable is about many threads accessing the same variable, and has to be solved with mutexes. volatile is for hardware. \$\endgroup\$ – Cacahuete Frito Jun 27 at 13:45
1
\$\begingroup\$

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);
    }
}
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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