How can I have a shared counter in multithreading using structs?

Question

I'm pretty new with multithreading and I was trying to increment a shared counter whithout using global variables, my goal is try to maximize the concurrency among the different threads and increment the variable until a number I give in arguments... Sorry if is a lame question, but I would like a help here, when I compile my code and run it i get a segmentation fault... I think the error is in the variable count that I create and the shared counter!

#include <stdio.h>
#include <pthread.h>
pthread_mutex_t mutex;
typedef struct {
    long *cnt;  /* pointer to shared counter */
    long n;     /* no of times to increment */
    int nr;
    pthread_t id;       /* application-specific thread-id */
} targ_t;
void *sfun(void *arg) {
    targ_t *est = (targ_t *) arg;
    here:
    pthread_mutex_lock(&mutex);
    (*(est->cnt))++;
    pthread_mutex_unlock(&mutex);
    if(*(est->cnt)<est->n)
        goto here;
    return NULL;
}


int main(int argc, char *argv[]){
    targ_t real[3];
    int c=0;
    long count=0;
    real[0].cnt=&count;
    pthread_mutex_init(&mutex, NULL);
    for(c=0;c<3;c++){
        real[c].n=atoi(argv[1]);
        real[c].nr=c+1;
        pthread_create(&real[c].id,NULL,&sfun,&real[c]);
    }
    for(c=0;c<3;c++){
        pthread_join(real[c].id,NULL);
    }
    pthread_mutex_destroy(&mutex);
    printf("OVERALL %lu\n", count);

    return 0;
}

TY in advance.

a common way to implement the "shared counter is to pass the counter in an argument to pthread_create (i.e. the 2nd arg). Then you can use atomic add operations to increment the counter. — bruceg, Sep 10 '16 at 00:20
Writing out loops with a label `here:` and a `gote here;` is not a particularly good idea. There are occasions (some, but not many, occasions) when it is appropriate to use `goto` — this is not one of those rare occasions. — Jonathan Leffler, Sep 10 '16 at 00:44
You don't actually validate that your code was given an `argv[1]` to convert; could it be that you forgot to pass that argument? — Jonathan Leffler, Sep 10 '16 at 00:46
However, your primary problem is that you initialize `real[0].cnt` but you do not initialize `real[1].cnt` or `real[2].cnt`, so those threads are accessing who knows what memory — it might be that they're using null pointers, or it might be pointers to anywhere in memory, allocated or not, writable or not. You're also missing ``. — Jonathan Leffler, Sep 10 '16 at 00:50
http://stackoverflow.com/questions/19232957/pthread-create-passing-an-integer-as-the-last-argument — bruceg, Sep 10 '16 at 00:54
@JonathanLeffler thank you very much! Now that i think about it, it makes so much sense! Btw I had a while loop in the label, the label was just one of my desperate tests to fix it — sh1ftz, Sep 10 '16 at 00:58

score 0 · Accepted Answer · answered Sep 10 '16 at 01:01

There are a number of problems identified in the comments:

Writing out loops with a label here: and a goto here; is not a particularly good idea. There are occasions (some, but not many, occasions) when it is appropriate to use goto — this is not one of those rare occasions.
You don't actually validate that your code was given an argv[1] to convert; could it be that you forgot to pass that argument?
However, your primary problem is that you initialize real[0].cnt but you do not initialize real[1].cnt or real[2].cnt, so those threads are accessing who knows what memory — it might be that they're using null pointers, or they might be pointers to anywhere in memory, allocated or not, aligned or not, writable or not.
You're also missing <stdlib.h>
You're testing *(est->cnt) outside the scope of mutual exclusion.

This code fixes those and some other issues:

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

pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

typedef struct
{
    long *cnt;  /* pointer to shared counter */
    long n;     /* no of times to increment */
    int nr;
    pthread_t id;       /* application-specific thread-id */
} targ_t;

static void *sfun(void *arg)
{
    targ_t *est = (targ_t *)arg;
    while (1)
    {
        pthread_mutex_lock(&mutex);
        long cval = *est->cnt;
        if (cval < est->n)
            ++*est->cnt;
        pthread_mutex_unlock(&mutex);
        if (cval >= est->n)
            break;
    }
    return NULL;
}

int main(int argc, char *argv[])
{
    targ_t real[3];
    long count = 0;

    if (argc != 2)
    {
        fprintf(stderr, "Usage: %s count\n", argv[0]);
        return(EXIT_FAILURE);
    }

    for (int c = 0; c < 3; c++)
    {
        real[c].cnt = &count;
        real[c].n = atoi(argv[1]);
        real[c].nr = c + 1;
        if (pthread_create(&real[c].id, NULL, &sfun, &real[c]) != 0)
            break;
    }

    for (int c = 0; c < 3; c++)
        pthread_join(real[c].id, NULL);

    pthread_mutex_destroy(&mutex);
    printf("OVERALL %lu\n", count);

    return 0;
}

When run (for example, the program is pth59):

$ pth59 100
OVERALL 100
$

Before moving the test (now on cval) so that the read of *est->cnt was done inside the scope of the mutex, I got an output OVERALL 102 from the same command line. It is important to access shared variables with proper mutual exclusion, even if it is read-only access.

How can I have a shared counter in multithreading using structs?

1 Answers1