2
\$\begingroup\$

I am not wondering about error checking (I will add that soon). I would instead like to know about correctness, efficiency and simplicity.

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

#define  NTHREADS    7
#define  RUN_N_TIMES 37

pthread_cond_t      new_task_available  = PTHREAD_COND_INITIALIZER;
pthread_cond_t      task_complete       = PTHREAD_COND_INITIALIZER;
pthread_mutex_t     mutex               = PTHREAD_MUTEX_INITIALIZER;

volatile int producerCond           = 0;
volatile int consumerCond[NTHREADS] = {};    
volatile int no_more_tasks          = 0;
volatile int mini_tasks_complete    = - NTHREADS;

void do_work(unsigned int* current_level){
  /* Some work done */
  ++*current_level;
}

void *threadfunc(void *parm){
  volatile int* thread_cond;
  unsigned int level = 0;
  thread_cond = (volatile int*)parm;
  while(!no_more_tasks){
    pthread_mutex_lock(&mutex);
    mini_tasks_complete++;
    if(mini_tasks_complete > 0)
      printf("Thread %u has completed %u units of work.\nIt was #%d to complete it's task this round..\n", \
        (unsigned int)pthread_self(), level, mini_tasks_complete);
    if(mini_tasks_complete == NTHREADS){
      producerCond = 1;
      pthread_cond_signal(&task_complete);
    }
    *thread_cond = 0;
    while (!*thread_cond) {
      pthread_cond_wait(&new_task_available, &mutex);
    }
    pthread_mutex_unlock(&mutex);
    if(no_more_tasks){
      return NULL;
    } else {
      do_work(&level);
    }
  }
  return NULL;
}

void reset_cond(int val){
  int i;
  for (i=0; i<NTHREADS; ++i)
    consumerCond[i] = val;
}

int main(int argc, char **argv)
{
  int                   i;
  pthread_t             threadid[NTHREADS];

  for(i=0; i < NTHREADS; ++i) {
    pthread_create(&threadid[i], NULL, threadfunc, (void*)&(consumerCond[i]));
  }

  while(mini_tasks_complete < 0){
    /* Do nothing */
  }

  printf("Waking up all waiting threads " "#RUN_N_TIMES" " times...\n");
  for(i = 0; i < RUN_N_TIMES; i++){
    pthread_mutex_lock(&mutex);
    mini_tasks_complete = 0;
    printf("New tasks available.\n");
    reset_cond(1);
    /* Wake them up */
    pthread_cond_broadcast(&new_task_available);
    producerCond = 0;
    while (!producerCond) {
      printf("Main waiting\n");
      /* Go to sleep */
      pthread_cond_wait(&task_complete, &mutex);
    }
    pthread_mutex_unlock(&mutex);
  }
  pthread_mutex_lock(&mutex);
  reset_cond(1);
  no_more_tasks = 1;
  printf("Go home everybody!\n");
  pthread_cond_broadcast(&new_task_available);
  printf("Wait for threads and cleanup...\n");
  pthread_mutex_unlock(&mutex);

  for (i=0; i<NTHREADS; ++i) {
    pthread_join(threadid[i], NULL);
  }

  pthread_cond_destroy(&new_task_available);
  pthread_cond_destroy(&task_complete);
  pthread_mutex_destroy(&mutex);

  printf("Main done\n");
  return 0;
}
\$\endgroup\$
2
\$\begingroup\$

Here are some comments on your program. Note that I am not a threads expert.

Firstly, the code runs nicely. I'm running OSX and it compiles cleanly and executes correctly (as far as I can tell). I was a little surprised that threads wake up in the same order each time - in other words if thread 123456 is the first to complete the first job, then it seems to be the first to complete each subsequent job. This says more about the thread implementation than your code I guess.

The problem I have with the code is that although it seems to work, it is coded very specifically for the problem in hand. The barriers ensuring that all threads come together at intervals are not separately identifiable but are entwined in the control code. The variable mini_tasks_complete, used in creating the barrier is initialized to -NTHREADS, allowed to run up to zero during the initial barrier and then cycles from 0 to NTHREADS. This clearly works but is inelegant (I think you will find that the initial barrier can be omitted and worked into the loop with a bit of thought).

I would prefer to see the thread function have the simple form:

while (more_tasks) {
    barrier();
    work();
}

and the main thread have the general form:

for (...) {
    prepare_work();
    barrier();
}

Written like this one can clearly identify the synchronisation. The barrier function here would also preferably take a parameter identifying the barrier and not use globals.

I assume your platform, like mine, lacks the pthread barrier. You might write your own barrier function. From what I have read, barriers are normally based upon a pair of condition variables and a pair of mutexes. The Little Book of Semaphores has a discussion of this and there is a useful Solaris implementation that could be used as a template.

Some trivia:

  • I would find while (more_tasks) more logical than while (!no_more_tasks)

  • inconsistent use of camelCase

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.