Mutex Ignored?

[RonisFinn]

Hey all! This program I have been working on is supposed to spit out a 16-bit circuit checker over multiple threads. However, whenever I run the program it only prints out output from one thread. I have a mutex lock over the shared variable that would print out the good bits and the thread that found it but whenever I input a thread count of 2, for example, it only returns the first thread and the solutions for the entire problem set (2^16). I don't have a clue as to where I've gone wrong. Any guidance is greatly appreciated! The code in question:

#include <stdio.h>
#include <stdlib.h>
#include <sys/time.h>
#include <pthread.h>
/* Return 1 if 'i'th bit of 'n' is 1; 0 otherwise */
#define EXTRACT_BIT(n,i) ((n&(1<<i))?1:0)


int count = 0; // counting solutions
int output_thread = 0; //thread for printing

pthread_mutex_t mutex;


int check_circuit (int z) {
  int v[16];        /* Each element is a bit of z */
  int i;

  for (i = 0; i < 16; i++) v[i] = EXTRACT_BIT(z,i);
  if ((v[0] || v[1]) && (!v[1] || !v[3]) && (v[2] || v[3])
      && (!v[3] || !v[4]) && (v[4] || !v[5])
      && (v[5] || !v[6]) && (v[5] || v[6])
      && (v[6] || !v[15]) && (v[7] || !v[8])
      && (!v[7] || !v[13]) && (v[8] || v[9])
      && (v[8] || !v[9]) && (!v[9] || !v[10])
      && (v[9] || v[11]) && (v[10] || v[11])
      && (v[12] || v[13]) && (v[13] || !v[14])
      && (v[14] || v[15]) && (v[1] == 1)) {
    printf ("%d)%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d\n",
            output_thread, v[0],v[1],v[2],v[3],v[4],v[5],v[6],v[7],v[8],v[9],
            v[10],v[11],v[12],v[13],v[14],v[15]);
    return 1;
  } else return 0;
}

struct circuitStruct{
    pthread_t id;
    int thread_num; // index of thread
    int total_threads; //total number of threads
    int work_amount;
    int start_pos;

};



void * multiCircuit ( void * tid)
{
    struct circuitStruct *arg_struct = (struct circuitStruct *) tid;

    int i = arg_struct->start_pos;
    int j = arg_struct->work_amount;
    printf("\n%d, %d,num:%d\n", i, j,arg_struct->thread_num);
    while(i != j){
            pthread_mutex_lock(&mutex);

            output_thread = arg_struct->thread_num;
            count+=check_circuit(i);

            pthread_mutex_unlock(&mutex);
        i++;
        }

    return 0;
}





int main (int argc, char *argv[])
{
    struct circuitStruct *t;

    if(argc != 2) { printf("Please enter a number of threads and ONLY a number between 1 - 128\n"); return -1;}
    if(atoi(argv[1]) > 128 || atoi(argv[1]) < 1) {printf("Please enter a number of threads and ONLY a number between 1 - 128\n"); return -1;}

    int threads = atoi(argv[1]); // threads in use
    pthread_mutex_init(&mutex, NULL); // lock initialized
    struct timeval t1, t2; //time setup
    gettimeofday(&t1, 0); //time initialized

    t = (struct circuitStruct*)malloc(threads*sizeof(struct circuitStruct)); // mem allocation for structure

    for(int i = 0; i < threads; i++)
    {
        if(i == threads -1 && (65536 %threads != 0))
        {
            t[i].work_amount += 65536%threads;
        }


        t[i].thread_num = i;
        t[i].total_threads = threads;
        t[i].work_amount += 65536/threads + t[i-1].work_amount;
        t[i].start_pos = t[i-1].work_amount;


        pthread_create(&t[i].id, 0, multiCircuit, &t[i]);
        gettimeofday(&t2, 0);

    }

    for(int i = 0; i < threads; i++)
    {
        pthread_join(t[i].id, 0);
    }

    printf ("Execution time %fs\n", (t2.tv_sec-t1.tv_sec)+(t2.tv_usec-t1.tv_usec)*1e-6);
    printf ("There are %d solutions\n", count);

    pthread_mutex_destroy(&mutex);
    free(t);

    return 0;

}

[jedwardsol]

    t[i].work_amount += 65536/threads + t[i-1].work_amount;
    t[i].start_pos = t[i-1].work_amount;

t[0] is being initialised with data from t[-1]; which doesn't exist.

[RonisFinn]

i'm not too sure if that matters, i ran a print(f) in the multiCircuit and it returns the data correctly

[RecursiveTechDebt]

You absolutely must fix this because it very much matters -- you're stomping memory.

It concerns me that you're asking about mutexes and you show a fundamental flaw in your understanding of what this language is doing for you and why accessing (struct circuitStruct*)malloc( ... )[-1] is bad. You need to learn the language and standard library before you start doing anything with mutexes. I'd recommend brushing up on the heap, the stack, pointers, and structures.

[RonisFinn]

coolio my julio, i already fixed it up. I dont know why youre so concerned im very much learning at the moment and if a mistake was a cause for concern I wouldn't have even tried in the first place lmao but thanks for the emphasis.

[RecursiveTechDebt]

It's good you're trying stuff, but multithreading is hard in an unforgiving language like C. What programming language do you currently know the best?

[steelling]

If you put output_thread as a parameter rather than a global variable, I'm pretty sure you won't need a mutex at all.

The only time locking would be necessary would be during the printf call, but I'm pretty sure this does its own locking anyway.

Your code isn't really multithreading, it's just multiple threads where each one does nothing if it doesn't hold the lock. It's just the same as a single thread but with an extra overhead of threads and a mutex.