Multi Producer - Single Consumer

Question

I have written multi producer single consumer code with bounded buffer. Please review the code for any improvements or mistakes.

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <unistd.h>
#include <semaphore.h>
#include <string.h>
#define SIZE 50
#define PRODUCERS 5
#define TOTAL 2 
#define ENTER printf("\n-- %s -- %d --", __func__, __LINE__);

char (*buf_write)[20], (*buf_read)[20];
char act_buf[2][SIZE][20];

pthread_mutex_t  lock = PTHREAD_MUTEX_INITIALIZER;
pthread_mutex_t     mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t      cond  = PTHREAD_COND_INITIALIZER;

pthread_barrier_t barrier;

int write_index = 0, writer_cnt = 0;
void init();

/*
 * Consumer
 */

void *consumer(void *vargp)
{

    int i = 0, index = 0;

    while (1) {

        pthread_mutex_lock(&mutex);
        if (writer_cnt != PRODUCERS)
            pthread_cond_wait(&cond, &mutex);
        pthread_mutex_unlock(&mutex);

        writer_cnt = 0;
        write_index = 0;
        buf_read = &act_buf[index];
        index = index + 1;  
        index = index % TOTAL;

        buf_write = &act_buf[index];
        pthread_barrier_wait(&barrier);

        i = 0;
        while (i < SIZE) {
            printf("%s\t",buf_read[i]);
            i++;
            if (i%10 == 0)
                printf("\n");
        }
        printf("\n");
        memset(buf_read, 0, sizeof(act_buf) / 2);
    }   
}

/*
 * Producer 
 */
void *producer(void *vargp)
{
    int i = 0;
    char *str = (char *) vargp;


    while (1) {

          i = 0;    
        while (i < 10) {
            pthread_mutex_lock(&lock);
            str[0] = '0' + i;
            strcpy(buf_write[write_index], str);    
            ++write_index;
            pthread_mutex_unlock(&lock);
            ++i;
        }

        pthread_mutex_lock(&mutex);
            writer_cnt++;   
            if (writer_cnt == PRODUCERS)
                pthread_cond_signal(&cond);
        pthread_mutex_unlock(&mutex);

        pthread_barrier_wait(&barrier);
    }
}

int main()
{

    int i = 0;
    pthread_t prouder_tid[PRODUCERS], consumer_tid;

    char str[5][20] = {" -producer1"," -producer2"," -producer3"," -producer4"," -producer5"};

    buf_write = &act_buf[0];

    init();

    for (i = 0; i < PRODUCERS; i++) {
        pthread_create(&prouder_tid[i], NULL, producer, &str[i]);
    }


    pthread_create(&consumer_tid, NULL, consumer, NULL);

    pthread_join(consumer_tid, NULL);

    for (i = 0; i < PRODUCERS; i++) {
        pthread_join(prouder_tid[i], NULL);
    }

    return 0;
}


void init()
{
    //pthread_mutex_init(&lock, NULL);
    //pthread_mutex_init(&writer_lock, NULL);
    int s = 0;

    s = pthread_barrier_init(&barrier, NULL, PRODUCERS + 1);

    if (s != 0) {
            printf("pthread_barrier_init failied");
        exit(-1);
    }   
}

Answer 1

Tabs Vs Spaces

Your code formatting isn't consistent. Looking at your code, this appears to be because you're using both tabs and spaces for white space. Which is why you end up with code that looks like this (notice the printf doesn't line up with the exit):

if (s != 0) {
        printf("pthread_barrier_init failied");
    exit(-1);
}   

Different people prefer one or the other, but don't mix and match pick one and stick with it.

Termination

It's unclear to me how you're expecting your threads to exit. Both your consumers and your producers go into while(true) loops, which don't have any break conditions in them. This is inconsistent with your main, which attempts to join with the consumer and producer threads.

Pointer Assignment

My compiler isn't a huge fan of this line:

buf_write = &act_buf[index]

buf_write is char(*buf_write)[20] and you're trying to assign it to char*[50][20]. I think you should probably be doing this:

buf_write = &act_buf[index][0]

Synchronisation

I think overall, the way you've synchronised between the consumer and producers is fairly tidy, although your variable names could be improved to describe what their function is. mutex is clearly a mutex, but what's it being used for, how is it different from lock? For the scenario you've built, having the consumer wait until all of the producers have completed seems to work ok, however in the general case, where producers might take different amounts of time it might make more sense for the consumer to handle information from each producer as it is complete.