sh1ftz sh1ftz - 2 months ago 11
C Question

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

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.

Answer

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.

Comments