Remixt Remixt - 1 month ago 14
C Question

Shouldn't this mutex "lock" if you are trying to access a global variable?

My source code below works exactly as its supposed to. The problem is that I managed to get it working only because I used print f statements to find out where the code was getting "stuck". I don't understand why in the inner while loops the mutex must be unlocked instead of locked by the function using it. I'm sorry if that isn't very clear, but I'm having problems understanding it myself.

For sanity sake I'll put the snip of code in question at the top on its own, and the full source code below it.

// if buffer is full, release mutex lock and check again
if (shared_count == NITEMS)
{
// Signal consumer thread and sleep
pthread_cond_signal(&bufferNotEmpty);
pthread_cond_wait(&bufferNotFull,&mutex);
pthread_mutex_unlock(&mutex);
break;
}


Here is the full source code:

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

#define NITEMS 10 // number of items in shared buffer

// shared variables
char shared_buffer[NITEMS]; // echo buffer
int shared_count; // item count

pthread_mutex_t mutex; // pthread mutex
pthread_cond_t bufferNotEmpty; // pthread condition variable for consumer thread
pthread_cond_t bufferNotFull; // pthread condition variable for producer thread

unsigned int prod_index = 0; // producer index into shared buffer
unsigned int cons_index = 0; // consumer index into shared buffer

// function prototypes
void * producer(void *arg);
void * consumer(void *arg);

int main()
{
pthread_t prod_tid, cons_tid1, cons_tid2;

// initialize pthread variables
pthread_mutex_init(&mutex, NULL);
pthread_cond_init(&bufferNotEmpty, NULL);
pthread_cond_init(&bufferNotFull, NULL);

// start producer thread
pthread_create(&prod_tid, NULL, producer, NULL);

// start consumer threads
pthread_create(&cons_tid1, NULL, consumer, NULL);
pthread_create(&cons_tid2, NULL, consumer, NULL);

// wait for threads to finish
pthread_join(prod_tid, NULL);
pthread_join(cons_tid1, NULL);
pthread_join(cons_tid2, NULL);

// clean up
pthread_mutex_destroy(&mutex);
pthread_cond_destroy(&bufferNotEmpty);
pthread_cond_destroy(&bufferNotFull);

return 0;
}

// producer thread executes this function
void * producer(void *arg)
{
char key;

while (1)
{
// read input key
scanf("%c", &key);

while (1)
{
// acquire mutex lock
pthread_mutex_lock(&mutex);

// if buffer is full, release mutex lock and check again
if (shared_count == NITEMS)
{
// Signal consumer thread and sleep
pthread_cond_signal(&bufferNotEmpty);
pthread_cond_wait(&bufferNotFull,&mutex);
pthread_mutex_unlock(&mutex);
break;
}

else
break;
}

// store key in shared buffer
shared_buffer[prod_index] = key;

// update shared count variable
shared_count++;


// update producer index
if (prod_index == NITEMS - 1)
prod_index = 0;
else
prod_index++;

// release mutex lock and signal consumer thread
pthread_mutex_unlock(&mutex);
pthread_cond_signal(&bufferNotEmpty);
}

return NULL;
}

// consumer thread executes this function
void * consumer(void *arg)
{
char key;

int id = (int)pthread_self();

while (1)
{
while (1)
{
// acquire mutex lock
pthread_mutex_lock(&mutex);

// if buffer is empty, wake producer thread and sleep
if (shared_count == 0)
{
pthread_cond_signal(&bufferNotFull);
pthread_cond_wait(&bufferNotEmpty,&mutex);
pthread_mutex_unlock(&mutex);
}

else
break;
}

// read key from shared buffer
key = shared_buffer[cons_index];

// echo key
printf("consumer %d %c\n", id, key);

// update shared count variable
shared_count--;

// update consumer index
if (cons_index == NITEMS - 1)
cons_index = 0;
else
cons_index++;

// release mutex lock and signal producer thread
pthread_mutex_unlock(&mutex);
pthread_cond_signal(&bufferNotFull);
}

return NULL;
}

Answer

You have the wait pattern wrong. It's:

    // acquire mutex lock
    pthread_mutex_lock(&mutex);

    // while buffer is empty, wait
    while (shared_count == 0)
    {
        pthread_cond_wait(&bufferNotEmpty,&mutex);
    } 

    // read key from shared buffer
    key = shared_buffer[cons_index];

    // echo key
    printf("consumer %d %c\n", id, key);

    // update shared count variable
    shared_count--;

    // update consumer index
    if (cons_index == NITEMS - 1)
        cons_index = 0;
    else
        cons_index++;

    // release mutex lock and signal producer thread
    pthread_mutex_unlock(&mutex);
    pthread_cond_signal(&bufferNotFull);

Fix the other side the same way. You need a loop that waits until the condition is satisfied, and you want to hold the (possibly re-acquired) mutex while you mess with the shared state.

Notice I removed the first call to pthread_cond_signal. Whatever made the shared_count equal zero would already have woken the other thread. We didn't change anything yet, so there's nobody we need to tell anything to yet.

There is no point in notifying another thread if you haven't done anything that it needs to be notified about. You call pthread_cond_signal when you need to tell another thread that you changed some shared state that it might be waiting for.