Multiple C threads not returning correct values

Question

I am trying to multiply two matrices using a different thread for each member of the resultant matrix. I have this code:

struct data{
    int p;
    int linie[20];
    int coloana[20];
};

void *func(void *args){

    struct data *st = (struct data *) args;
    int c = 0;

    for(int k = 0; k < st->p; k++){
        c += st->linie[k] * st->coloana[k];
    }

    char *rez = (char*) malloc(5);
    sprintf(rez, "%d", c);

    return rez;
}

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

    int n = 2;
    int m = 2;

    int A[2][2] = {{1, 2},  
                   {4, 5}};

    int B[2][2] = {{7, 3},  
                  {7, 5}};

    int C[n][m];

    char *res[n * m];
    char *rez[n * m];

    pthread_t threads[n * m];
    int count = 0;

    for(int i = 0; i < n; i++){
        for(int j = 0; j < m; j++){
            struct data st;
            st.p = 2;
            for(int x = 0; x < st.p; x++){
                st.linie[x] = A[i][x];
                st.coloana[x] = B[x][j]; 
            }

            pthread_create(&threads[count], NULL, func, &st);   

            count++;
        }   
    }

    for(int i = 0; i < n * m; i++){
        pthread_join(threads[i], (void**) &rez[i]);
        printf("%d ", atoi(rez[i]));

    }

    return 0;
}

But the correct result is never put into rez[i]. For example I get output "63 37 37 37". The code works perfectly if I don't choose to wait for every thread to finish, i.e. I put that pthread_join right after pthread_create in the nested for loop. What should I do? Thanks for reading!

Answer 1

Your first threading problem is here:

for(int i = 0; i < n; i++){
    for(int j = 0; j < m; j++){
        struct data st;
        st.p = 2;
        for(int x = 0; x < st.p; x++){
            st.linie[x] = A[i][x];
            st.coloana[x] = B[x][j]; 
        }
        pthread_create(&threads[count], NULL, func, &st);   
        count++;
    }   
}

All the threads get passed a pointer to the same variable, &st, which goes out of scope after each call to pthread_create(). You need to ensure that each thread gets its own variable, and that the variable lasts until the thread exits.

To fix this, for example, you could try:

struct data st[n * m];

for (int i = 0; i < n; i++)
{
    for (int j = 0; j < m; j++)
    {
        st[count].p = 2;
        for (int x = 0; x < st[count].p; x++)
        {
            st[count].linie[x] = A[i][x];
            st[count].coloana[x] = B[x][j]; 
        }
        int rc = pthread_create(&threads[count], NULL, func, &st[count]);
        if (rc != 0)
            …report pthread creation error…  
        count++;
    }   
}

This gives each thread its own struct data to work on, and the structure outlasts the pthread_join() loop.

I'm not completely that it is a good scheme to make one copy of the relevant parts of the two arrays for each thread. It's not too painful at size 2x2, but at 20x20, it begins to be painful. The threads should be told which row and column to process, and should be given pointers to the source matrices, and so on. As long as no thread modifies the source matrices, there isn't a problem reading the data.

---

Updated answer which replaces the previous invalid code related to pthread_join() (as noted by oftigus in a comment) with this working code. There's a reason I normally test before I post!

On the whole, casts like (void **) should be avoided in the pthread_join() loop. One correct working way to handle this is:

    for (int i = 0; i < n * m; i++)
    {
        void *vp;
        int rc = pthread_join(threads[i], &vp);
        if (rc == 0 && vp != NULL)
        {
            rez[i] = vp;
            printf("(%s) %d ", rez[i], atoi(rez[i]));
            free(rez[i]);
        }
    }
    putchar('\n');

This passes a pointer to a void * variable to pthread_join(). If it finds the information for the requested thread, then pthread_join() makes that void * variable hold the value returned by the thread function. This can then be used as shown — note the error handling (though I note that the example in the POSIX specification for pthread_join()ignores the return value from pthread_join() with a (void) cast on the result).

---

I don't see where you use res or C.

---

The result I get is:

(21) 21 (13) 13 (63) 63 (37) 37 

where the value in parentheses is a string and the value outside is converted by atoi(). That looks like the correct answer for multiplying A by B (in that order).

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

struct data
{
    int p;
    int linie[20];
    int coloana[20];
};

static void *func(void *args)
{
    struct data *st = (struct data *)args;
    int c = 0;

    for (int k = 0; k < st->p; k++)
    {
        c += st->linie[k] * st->coloana[k];
    }

    char *rez = (char *)malloc(5);
    sprintf(rez, "%d", c);

    return rez;
}

int main(void)
{
    int n = 2;
    int m = 2;
    int A[2][2] = {{1, 2},  {4, 5}};
    int B[2][2] = {{7, 3},  {7, 5}};
    char *rez[n * m];

    pthread_t threads[n * m];
    int count = 0;
    struct data st[n * m];

    for (int i = 0; i < n; i++)
    {
        for (int j = 0; j < m; j++)
        {
            st[count].p = 2;
            for (int x = 0; x < st[count].p; x++)
            {
                st[count].linie[x] = A[i][x];
                st[count].coloana[x] = B[x][j];
            }
            int rc = pthread_create(&threads[count], NULL, func, &st[count]);
            if (rc != 0)
            {
                fprintf(stderr, "Failed to create thread %d for cell C[%d][%d]\n", count, i, j);
                exit(1);
            }
            count++;
        }
    }

    for (int i = 0; i < n * m; i++)
    {
        void *vp;
        int rc = pthread_join(threads[i], &vp);
        if (rc == 0 && vp != NULL)
        {
            rez[i] = vp;
            printf("(%s) %d ", rez[i], atoi(rez[i]));
            free(rez[i]);
        }
    }
    putchar('\n');

    return 0;
}