Thread synchronization with mutex

Question

This program prints odd numbers by thread1 and even numbers by thread2 sequentially. Can this code be optimized or made more efficient?

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>

pthread_mutex_t count_mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t condition_var = PTHREAD_COND_INITIALIZER;

void *function_count1();
void *function_count2();

int count = 1;

int main()
{
    pthread_t thread1, thread2;
    int ret;

    if( (ret=pthread_create( &thread1, NULL, &function_count1, "thread1")) )
    {
        printf("Thread 1 creation failed: %d\n", ret);
        exit (EXIT_FAILURE);
    }

    if( (ret=pthread_create( &thread2, NULL, &function_count2, "thread2")) )
    {
        printf("Thread 2 creation failed: %d\n", ret);
        exit (EXIT_FAILURE);
    }

    pthread_join( thread1, NULL);
    pthread_join( thread2, NULL);

    exit(EXIT_SUCCESS);
}

void *function_count1(void *arg)
{
    while(count <= 10)
    {
        pthread_mutex_lock( &count_mutex );
        if (count % 2 != 0)
        {
            pthread_cond_wait(&condition_var, &count_mutex);
            printf("%s : counter = %d\n", (char *)arg, count++);
        }
        else
        {
            pthread_cond_signal(&condition_var);
        }
        pthread_mutex_unlock( &count_mutex );
    }
}

void *function_count2(void *arg)
{
    while(count <= 10)
    {
        pthread_mutex_lock( &count_mutex );

        if (count % 2 == 0)
        {
            pthread_cond_wait(&condition_var, &count_mutex);
            printf("%s : counter = %d\n", (char *)arg, count++);
        }
        else
        {
            pthread_cond_signal(&condition_var);
        }
        pthread_mutex_unlock(&count_mutex);
    }
}

Answer 1

• Since pthread_create() returns an error number on failure, you can put ret into strerror() to get the associated error message instead of just printing the number:

  fprintf(stderr, "Thread 1 creation failed: %s\n", strerror(ret));
  

  Notice that I'm calling fprintf() instead of printf(). Since you're printing an error, it must be sent to stderr, not stdin (used by default with printf()).

  Also, while you'd normally not need all this code for printing an error, it's necessary since pthread_create() doesn't set errno (which is why ret is useful here).

  For code that does set errno (which would be stated on man pages), you'd only need to call perror() to print the corresponding error message:

  perror("<insert source of failure here>");
  

  This would also eliminate the need for variables like ret in such code. However, with pthread_create() and related functions, you could probably just put errno in its place and then use perror().

• The functions called by pthread_create() should still return something. Since the return value won't be useful here, you can just return NULL.

• There's no need to call exit() right at the end of main(). You're preventing it from returning a value, and it'll always return 0 at this point anyway.

• This isn't a criticism, but a tip: make sure you're running this through valgrind to check for any memory leaks along the way.

  Actually, this may already cause a leak, but I haven't checked to make sure. If it does, then you may need to call both pthread_mutex_destroy() and pthread_cond_destroy() before exiting the program at any point.

Answer 2

This solution actually has a data race on count. count is accessed outside the mutex from both threads:

while(count <= 10)

Compile the program with the thread sanitizer (or the corresponding tool in Visual Studio) and it should throw a warning.

The program doesn't necessarily exit. When I run it, thread1 hangs forever at the call to pthread_cond_wait().

It looks like thread1 is blocked at pthread_mutex_lock() while thread2 increments count to 11. After thread2 unlocks the mutex, it exits because count <= 10 is false. thread1, on the other hand, moves on to pthread_cond_wait() and waits forever because thread2 isn't around to send a signal to wake it.

Answer 3

My initial impression is that this is a really bad way to have threads interacting. There's no real reason why either thread can't print out the next number. Assuming this is some kind of learning exercise and putting this issue to the side, I'd introduce a new mutex.

At the moment, both of your threads share a single mutex which means that when one is done processing it unlocks the mutex which results in a thread being released. The thread released may be the second thread which is already waiting on the mutex, or it could be that the current thread manages to process the rest of the while loop and reacquire the lock. If you change it so that you have two mutexes, you can have the following pattern:

Thread1 - Wait on Mutex 1
Thread1 - Print Current value (Always Odd)
Thread1 - Unlock Mutex 2

Thread2 - Wait on Mutex 2
Thread2 - Print Current Value (Always even)
Thread2 - Unlock Mutex 1

This way, by each thread having its own mutex, threads are only ever allowed into the area when they have something to do.

Answer 4

void *function_count2();

Don't use this deprecated feature bug. It has been deprecated since C90, so I see absolutely no reason to use it, ever.

Answer 5

Design

The thread functions function_count1 and function_count2 are practically identical. It would be simpler to have one function that is prameterized based on some input argument.

I don't like global modifiable state. Pass in the variables as parameters to your function. This allows your functions to be much more flexable in how they work.

Implementation

You do things multiple times (I know its only two in both cases). But it would be simpler to write things in terms of loops. This will make the code easier to modify.

Code Review

Global state is never a good idea.

pthread_mutex_t count_mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t condition_var = PTHREAD_COND_INITIALIZER;
int count = 1;

---

Multiple variables number like this implies an array.

    pthread_t thread1, thread2;

If we use a loop here:

    if( (ret=pthread_create( &thread1, NULL, &function_count1, "thread1")) )
    {..}

    if( (ret=pthread_create( &thread2, NULL, &function_count2, "thread2")) )
    {...}

And here:

    pthread_join( thread1, NULL);
    pthread_join( thread2, NULL);

---

Here you are checking something from multiple loops outside the confines of a mutex.

    while(count <= 10)
    {
        pthread_mutex_lock( &count_mutex );

---

Normally when using conditional variable you use a loop (not an if). The loop checks the condition until it is true. While false you wait on the condition. Your solution works because the main code is inside the else and you repeat using the outer loop. I think using a while loops makes it neater.

        if (count % 2 != 0)
        {
            pthread_cond_wait(&condition_var, &count_mutex);
            printf("%s : counter = %d\n", (char *)arg, count++);
        }
        else
        {
            pthread_cond_signal(&condition_var);
        }
        // Or use this:
        while (count % 2 != 0)
        {
            pthread_cond_wait(&condition_var, &count_mutex);
        }
        printf("%s : counter = %d\n", (char *)arg, count++);
        pthread_cond_signal(&condition_var);

---

I would implement like this:

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>

void* threadAction(void* arg);


typedef struct ThreadInfo
{
    char const*         name;
    int*                counter;
    int                 div;
    int                 rem;
    pthread_mutex_t*    mutex;
    pthread_cond_t*     cond;
    pthread_t           thread;
} ThreadInfo;

int main()
{
    int             count           = 1;
    pthread_mutex_t count_mutex     = PTHREAD_MUTEX_INITIALIZER;
    pthread_cond_t  condition_var   = PTHREAD_COND_INITIALIZER;

    ThreadInfo  threads[2] = {{"thread-0", &count, 2, 0, &count_mutex, &condition_var}, {"thread-1", &count, 2, 1, &count_mutex, &condition_var}};

    for (int loop = 0; loop < 2; ++loop)
    {
        int ret;
        ret=pthread_create(&threads[loop].thread, NULL, &threadAction, threads + loop);
        if (ret != 0)
        {
            printf("%s creation failed: %d\n", threads[loop].name, ret);
            exit (EXIT_FAILURE);
        }
    }

    for (int loop = 0; loop < 2; ++loop)
    {
        pthread_join(threads[loop].thread, NULL);
    }

    exit(EXIT_SUCCESS);
}
void* threadAction(void* arg)
{
    ThreadInfo* info = (ThreadInfo*)arg;

    pthread_mutex_lock(info->mutex);
    while((*info->counter) <= 10)
    {
        while ((*info->counter) % info->div != info->rem)
        {
            pthread_cond_wait(info->cond, info->mutex);
        }
        printf("%s : counter = %d\n", info->name, (*info->counter)++);
        pthread_cond_signal(info->cond);
    }
    pthread_mutex_unlock(info->mutex);
    return NULL;
}