I've been working on a multithreading exercise in C using POSIX threads that implements a thread pool design. My strategy was to embed the required and shared concurrency primitives in a struct instance then pass it to every pthread.

I think my approach is flawed. It would appear do_work() is not dishing out jobs optimally and my book doesn't really explain a method for debugging these types of problems.

#include <stdlib.h>
#include <stdio.h>
#include <pthread.h>
#include "list.h"
#include "cserver.h"

#define MAX_THREADS 5

struct thread_argvs {
    struct list *jobs_to_do;
    pthread_mutex_t job_lock;
    pthread_cond_t job_signal;
    int active;
}

struct job {
    int data; // arbitrary for example.
}

void * do_work(void *argv_values);

main(){
    
    // Stores jobs collected from client.
    struct job_list;
    list_init(&job_list);
    
    struct thread_argvs argvs;
    argvs.jobs_to_do = &job_list;
    argvs.job_lock = (pthread_mutex_t) PTHREAD_MUTEX_INITIALIZER;
    argvs.job_signal = (pthread_cond_t) PTHREAD_COND_INITIALIZER;
    argvs.active = 1;
    
    pthread_t threads[MAX_THREADS];
    for (int i = 0; i < MAX_THREADS; i  ){
         pthread_create(&thread_list[i], NULL, do_work, (void *) argvs);
    }
    
    /* connect to a client in-between here */
        
    for (;;){
        
        struct job *new_job = malloc(sizeof(struct job));
        new_job = get_job_from_client();  
        list_push(&job_list, &new_job);
        pthread_cond_signal(&argv.job_signal);
        
        if (new_job.data == -1){ // final job
            argv.active = 0;
            break;
        }
    }
    
    for (int i = 0; i < MAX_THREADS; i  ) {
        pthread_cond_signal(&argv.job_signal);
    }

    for (int i = 0; i < MAX_THREADS; i  ) {
        pthread_join(threads[i], NULL);
    }
    
    return 0;
}

void * do_work(void *argv_values){

    struct thread_argvs *argvs = (struct thread_argvs *) argv_values;
    struct job *next_job;
    
    while(argvs.active == 1){
    
        next_job = NULL;
        
        pthread_mutex_lock(&(argvs->job_lock));
        pthread_cond_wait(&(argvs->job_signal), &(argvs->job_lock));
        
        if (list_size(argvs->jobs_to_do) > 0) {
            next_job = list_pop(&jobs_to_do);
        }
        
        pthread_mutex_unlock(&(argvs->job_lock));

        if (new_job != NULL) {
            do_something_with_job(next_job->data);
            free(next_job);
            next_job = NULL;
        }
    }
    
    pthread_exit(0);
}

CodePudding user response：

You code can easily deadlock. All the calls to pthread_cond_signal can occur while no thread is blocked on the condition variable. Then the first thread to call pthread_cond_wait can just block forever.

You must never call pthread_cond_wait without first confirming (while holding the mutex!) that the thing you are going to wait for hasn't already happened.

CodePudding user response：

From the top:

    while(argvs.active == 1){

Every bit of storage that is shared between threads must have a synchronization guarantee. Where is the guarnatee for the line quoted above? Next up; Condition variables have no memory. There is no doubt, if somebody sees something like:

        pthread_mutex_lock(&(argvs->job_lock));
        pthread_cond_wait(&(argvs->job_signal), &(argvs->job_lock));

That they missed the message about how condition variables work. It is in the name condition variables, which means the before waiting upon them, there is some condition that you must check upon, and failing that condition being true, choose to wait upon it.

Next: a supplicant of the last - when you awake on a condition variable, you need to check that the condition is satisfied before continuing. An example follows:

    Lock(&VeryImportantThing);
    while (SomethingVeryImportant is not Set) {
        CondWait(&VeryImportantCond, &VeryImportantLock);
    }
    SomethingVeryImportant = Vanilla;