0
\$\begingroup\$

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

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.

\$\endgroup\$
  • \$\begingroup\$ I mistakenly did not intended properly in the if loop. Good point i did not cancel thread resources and locks. What do you mean { 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 } ..?? implementing different buffers for each producer are more efficient ??? \$\endgroup\$ – Vishnu Reddy Jul 12 '16 at 0:10
  • \$\begingroup\$ @VishnuReddy I mean that if your producers were working at different rates (so one produced an 1 output per second, whilst another produced 10 outputs a second) then it may be more efficient to process outputs as they completed, or as each producer completed, rather than waiting for all producers to complete their production of 10 items. \$\endgroup\$ – forsvarir Jul 12 '16 at 7:12

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.