producer-consumer c - segmentation fault for bigger values

Question

I am doing the one producer-one consumer problem with two threads in c. I did it using a shared linked list in which producer puts something and consumer takes it from the same list. I have to run my code with 2 values N and B(N being the size of data to be transferred and B being the maximum size of the shared linked list). (./thread-a1 N B) This code runs fine for small values. But, for N = 20,000 and B = 16, it gives Segmentation fault: 11 I cannot figure out why is this happening. Please help

#include<time.h>
#include<sys/time.h>
#include<stdlib.h>
#include<stdio.h>
#include<pthread.h>
#include<unistd.h>

struct job
{
    int data;
    struct job* next;
};

struct job* head;
int maxSize;
int n;
pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;

int getCount()
{
    int count = 0;
    struct job* p = head;
    while(p != NULL)
    {
        p = p -> next;
        count++;
    }
    return count;
}

void Insert(int x)
{

    struct job* temp = (struct job*)malloc(sizeof(struct job));
    temp -> data = x;
    if (head == NULL)
    {
        temp -> next = NULL;
    }
    else
    {
        temp -> next = head;
    }
    head = temp;
}

void Delete()
{

    struct job *toDelete, *secondLastNode;

    if(head == NULL)
    {

    }
    else
    {
        toDelete = head;
        secondLastNode = head;


        while(toDelete->next != NULL)
        {

            secondLastNode = toDelete;
            toDelete = toDelete->next;
        }
        printf("%d, ", toDelete -> data);

        if(toDelete == head)
        {
            free(toDelete);
            head = NULL;
        }
        else
        {
            secondLastNode->next = NULL;
            free(toDelete);
        }

    }
}

void* producer(void* arg)
{
    pthread_mutex_lock(&m);
    head = NULL; 

    int i;
    for (i = 0; i<n; i++)
    {

        while(getCount() >= maxSize)
        {
            pthread_mutex_unlock(&m);
        }
        Insert(i);
    }

    return NULL;
}

void* consumer(void* arg)
{
    pthread_mutex_lock(&m);
    int i;
    for (i=0; i<n; i++)
    {
        while(getCount() <= 0)
        {
            pthread_mutex_unlock(&m);   
        }
        Delete();
    }


    return NULL;
}

int main(int argc, char *argv[])
{

    struct timeval start_init, end_init;
    gettimeofday(&start_init, NULL);
    n = atoi(argv[1]);
    maxSize = atoi(argv[2]);

    pthread_t thread1;
    pthread_t thread2;
    gettimeofday(&end_init, NULL);
    printf("Time elapsed for initialization is: %ld\n", 
        (long)(end_init.tv_sec*1000000 + end_init.tv_usec) -    (start_init.tv_sec*1000000 + start_init.tv_usec));

    struct timeval start_trans, end_trans;
    gettimeofday(&start_trans, NULL);
    pthread_create(&thread1, NULL, &producer, NULL);
    pthread_create(&thread2, NULL, &consumer, NULL);

    pthread_join(thread1, NULL);
    pthread_join(thread2, NULL);
    gettimeofday(&end_trans, NULL);
    printf("Time elapsed for transmission is: %ld\n", 
        (long)(end_trans.tv_sec*1000000 + end_trans.tv_usec) - (start_trans.tv_sec*1000000 + start_trans.tv_usec));


    return 0;
}

You should check the return value of `malloc`. If it is zero, you are out of memory. — Paul Ogilvie, Oct 03 '16 at 16:25
@PaulOgilvie: I don't think 20K very small structs will exhaust memory, even if none of them are ever freed. — Scott Hunter, Oct 03 '16 at 16:27
@ScottHunter: sometimes when it starts running, or sometimes in between while running — wander, Oct 03 '16 at 16:48
@PaulOgilvie: I added a condition statement that if malloc returns 0, program returns. But, the fault still occurs — wander, Oct 03 '16 at 16:50

score 3 · Accepted Answer · answered Oct 03 '16 at 16:30

3

You need to revisit your locking; you only acquire locks at the start of each producer & consumer, and from then on only unlock them (repeatedly!), meaning no further synchronization happens. The longer your program runs, the more likely that this lack of synchronization will trip up your program.

answered Oct 03 '16 at 16:30

Scott Hunter

48,888
12
60
101

1

In addition to this, you should also read some about 'busy waiting' and how to avoid it. – johni Oct 03 '16 at 16:33
1

@SukhmanWaraich It is an error to attempt to unlock a mutex which is already unlocked. Putting the `pthread_mutex_unlock` call in a loop as you have it is only going to lead to trouble: https://linux.die.net/man/3/pthread_mutex_lock – yano Oct 03 '16 at 16:39
1

As @johni said. In particular, a condition variable would be a good choice here, or maybe a pair of semaphores. – John Bollinger Oct 03 '16 at 16:40
@ScottHunter: Even if I remove all the locks, the error occurs – wander Oct 03 '16 at 16:52
Don't *remove* the locks; use them (or some other mechanism) properly. – Scott Hunter Oct 03 '16 at 16:55
got it. Thank you so much :) – wander Oct 03 '16 at 17:23

score 0 · Answer 2 · answered Oct 03 '16 at 17:16

I think your locking should be:

void* producer(void* arg)
{
    int i;
    for (i = 0; i<n; i++)
    {
        pthread_mutex_lock(&m);
        while(getCount() >= maxSize)
        {
            pthread_mutex_unlock(&m);   // allow consumer to acquire lock
            pthread_mutex_lock(&m);     // re-acquire lock
        }
        Insert(i);
        pthread_mutex_lock(&m);
    }
    return NULL;
}

void* consumer(void* arg)
{
    int i;
    for (i=0; i<n; i++)
    {
        pthread_mutex_lock(&m);
        while(getCount() <= 0)
        {
            pthread_mutex_unlock(&m);   // allow producer to acquire lock  
            pthread_mutex_lock(&m);     // re-acquire lock
        }
        Delete();
        pthread_mutex_lock(&m);
    }
    return NULL;
}

Note that all operations, including getCount, must be done with the lock acquired. (Consider also replacing getCount with a global variable; only manipulate it while locked.)

producer-consumer c - segmentation fault for bigger values

2 Answers2