Dining Philosophers variation in C

Question

This is a variation of the Dining Philosophers Problem. The task is to coordinate several students inside a gym. All students try to obtain their desired training weights from a shared weight rack. During runtime, the user can issue commands to:

• block a student (b + student id)
• unblock a student (u + student id)
• proceed (p + student id) (ends a student's workout/rest loop)
• end the program (q or Q)

I'm a beginner in C and would appreciate opinions and pointers on how to improve my code.

main.c

#include <pthread.h>
#include "main.h"
#include "gym_monitor.h"
/*
*
*   Main module of the Thread-Coordination Exercise. This modul contains
*   the main function which creates the threads. After creation all threads
*   enter the gym_routine function.
*
*/
#define REST_LOOP     1000000000
#define WORKOUT_LOOP  500000000

#define WEIGHTS_ANNA        6
#define WEIGHTS_BERND       8
#define WEIGHTS_CLARA_DIRK 12
#define WEIGHTS_EMMA       14

#define MAX_INPUT_SIZE 3

#define WEIGHT_RACK_DEF {4,4,5}

static pthread_barrier_t gym_routine_barrier;

static void workout(Student* student) {
  for( int i = 0; i < WORKOUT_LOOP; i++ ) {
    if(student->status == BLOCKED) {
      rest_student(student);
    }else if(student->status == PROCEED) {
      student->status = NORMAL;
      break;
    }
  }
}

static void rest(Student* student) {
  for( int i = 0; i < REST_LOOP; i++ ) {
    if(student->status == BLOCKED) {
      rest_student(student);
    }else if(student->status == PROCEED) {
      student->status = NORMAL;
      break;
    }
  }
}

static void* gym_routine(void* stud) {
  pthread_barrier_wait(&gym_routine_barrier);
  Student* student = (Student*) stud;
  while(student->status != QUIT) {
    get_weights(student);
    workout(student);
    put_weights(student);
    rest(student);
  }
  return NULL;
}

int main(void) {

  char available_weights[] = WEIGHT_RACK_DEF;

  int students_weights[] = {WEIGHTS_ANNA,WEIGHTS_BERND,WEIGHTS_CLARA_DIRK,
                            WEIGHTS_CLARA_DIRK,WEIGHTS_EMMA};

  Student students[NR_STUDENTS];
  monitor_vars* monitor = init_monitor();

  pthread_barrier_init(&gym_routine_barrier, NULL, NR_STUDENTS);

  int res;
  for( int i = 0; i < NR_STUDENTS; i++ ) {
    students[i].thread_id = i;
    students[i].weight_plan = students_weights[i];
    students[i].status = NORMAL;

    for(int j = 0; j < NR_WEIGHTS; j++) {
      students[i].current_weight[j] = 0;
    }
    students[i].mon = monitor;
    students[i].sem_student = init_sem_student();
    students[i].other_students = students;
    students[i].weight_rack = available_weights;


    res = pthread_create(&students[i].thread, NULL, gym_routine,
                         (void*) &students[i]);
    if(res != 0) {
      perror("Thread creation failed");
      exit(EXIT_FAILURE);
    }
  }

  /*Handling user input*/
  char input[MAX_INPUT_SIZE] = {0};
  while(strncasecmp(fgets(input, MAX_INPUT_SIZE, stdin),"q", 1))  {

    /* trying to get rid of newline from input
      this is the only 'solution' that works so far*/
    if(input[0] == '\n' || input[0] == '\0') {
      continue;
    }

    fflush(stdout);
    if((input[0] - '0') >= 0 && (input[0] - '0') < NR_STUDENTS
      && strchr("bpu", input[1]) && strlen(input) == 2 ) {
        int student_id = input[0] - '0';

        students[student_id].status = input[1];
        if(students[student_id].status == UNBLOCK) {
          wake_student(&(students[student_id]));
          students[student_id].status = NORMAL;      
        }
      }else {
        printf("Not a valid instruction\n");
        fflush(stdout);
      }
    }

  /*updating student status*/
  for(int i = 0; i < NR_STUDENTS; i++) {
    wake_student(&students[i]);     //students can only quit if they are not asleep
    students[i].status = QUIT;
  }

  for(int i = 0; i < NR_STUDENTS; i++) {
      pthread_join(students[i].thread,NULL);
      destroy_sem_student(&students[i]);
      free(students[i].sem_student);
  }
  destroy_monitor(monitor);
  exit(EXIT_SUCCESS);
}

main.h

#ifndef MAIN_H
#define MAIN_H
#include <stdlib.h>
#include <pthread.h>
#include <string.h>
#include <strings.h>
#include "gym_monitor.h"

enum weight_names{
  KG_2,
  KG_3,
  KG_5
};

#define BLOCKED 'b'
#define PROCEED 'p'
#define NORMAL  'n'
#define UNBLOCK 'u'
#define QUIT    'q'

#define NR_WEIGHTS  3
#define NR_STUDENTS 5

typedef struct Student Student;

struct Student  {
  pthread_t thread;
  int thread_id;
  int weight_plan;
  char status;
  char training_state;
  char current_weight[NR_WEIGHTS];
  monitor_vars* mon;
  sem_t* sem_student;
  Student* other_students;
  char* weight_rack;
};

#endif

gym_Monitor.c

#include "gym_monitor.h"
#include "main.h"

/*
*   Monitor module - Encapsulates all objects and functions that manage
*   thread-coordination.
*
*/

#define MAX_2KG_3KG 4
#define MAX_5KG 5
#define RED   "\x1B[31m"
#define RESET "\x1B[0m"

const int weight_arr[] = {
  [KG_2] = 2,
  [KG_3] = 3,
  [KG_5] = 5
};


static int calculate_weight(Student* student, int weight) {
  if(weight == 0) {
    return 1;
  }

  if(weight >= weight_arr[KG_2] && student->weight_rack[KG_2] > 0) {
    student->weight_rack[KG_2] -= 1;
    student->current_weight[KG_2] += 1;

    if(!calculate_weight(student, weight - weight_arr[KG_2])) {
      student->weight_rack[KG_2] += 1;
      student->current_weight[KG_2] -= 1;
    }else {
      return 1;
    }

    if(weight >= weight_arr[KG_3] && student->weight_rack[KG_3] > 0) {
      student->weight_rack[KG_3] -= 1;
      student->current_weight[KG_3] += 1;

      if(!calculate_weight(student, weight - weight_arr[KG_3])) {
        student->weight_rack[KG_3] += 1;
        student->current_weight[KG_3] -= 1;
      }else {
        return 1;
      }
    }

    if(weight >= weight_arr[KG_5] && student->weight_rack[KG_5] > 0) {
      student->weight_rack[KG_5] -= 1;
      student->current_weight[KG_5] += 1;

      if(!calculate_weight(student, weight - weight_arr[KG_5])) {
        student->weight_rack[KG_5] += 1;
        student->current_weight[KG_5] -= 1;
      }else {
        return 1;
      }
    }
  }

  return 0;
}

static void display__status(Student* student) {
  int consistency_check[] = {0,0,0};

  for(int i = 0; i < NR_STUDENTS; i++) {
    printf("%d(%d)%c:%c:[%d, %d, %d]  ",student->other_students[i].thread_id,
                                        student->other_students[i].weight_plan,
                                        student->other_students[i].status,
                                        student->other_students[i].training_state,
                                        student->other_students[i].current_weight[KG_2],
                                        student->other_students[i].current_weight[KG_3],
                                        student->other_students[i].current_weight[KG_5]);

    consistency_check[KG_2] += student->other_students[i].current_weight[KG_2];
    consistency_check[KG_3] += student->other_students[i].current_weight[KG_3];
    consistency_check[KG_5] += student->other_students[i].current_weight[KG_5];
  }
  if(  consistency_check[KG_2] > MAX_2KG_3KG
    || consistency_check[KG_3] > MAX_2KG_3KG
    || consistency_check[KG_5] > MAX_5KG ) {
      printf(RED "Inconsistent State\n" );
      printf("[%d, %d, %d]\n" RESET,consistency_check[KG_2],
                              consistency_check[KG_3],
                              consistency_check[KG_5]);
  }else {
      printf("Supply: [%d, %d, %d]\n", student->weight_rack[KG_2],
                                       student->weight_rack[KG_3],
                                       student->weight_rack[KG_5]);
  }
  fflush(stdout);
}


void get_weights(Student* student) {
  pthread_mutex_lock(&student->mon->lock);
  student->training_state = GET_WEIGHTS;
  while(!calculate_weight(student, student->weight_plan)) {
    student->training_state = BLOCKED;
    display__status(student);
    pthread_cond_wait(&student->mon->insufficient_weight, &student->mon->lock);
    if(student->status != QUIT) {
      student->status = NORMAL;
    }
  }

  display__status(student);
  student->training_state = WORKOUT;
  pthread_mutex_unlock(&student->mon->lock);
}

void put_weights(Student* student) {
  pthread_mutex_lock(&student->mon->lock);
  student->training_state = PUT_WEIGHTS;

  student->weight_rack[KG_2] += student->current_weight[KG_2];
  student->weight_rack[KG_3] += student->current_weight[KG_3];
  student->weight_rack[KG_5] += student->current_weight[KG_5];

  for(int i = 0; i < NR_WEIGHTS; i++) {
    student->current_weight[i] = 0;
  }

  display__status(student);
  pthread_cond_signal(&student->mon->insufficient_weight);
  student->training_state = REST;
  pthread_mutex_unlock(&student->mon->lock);
}

void rest_student(Student* student) {
  sem_wait(student->sem_student);
}

void wake_student(Student* student) {
  sem_post(student->sem_student);
}

sem_t* init_sem_student() {
  int res = 0;
  sem_t* sem_student = malloc(sizeof(sem_t));
  if(sem_student == NULL) {
    perror("malloc failed, exiting...");
    exit(EXIT_FAILURE);
  }
  res = sem_init(sem_student,0,1);
  if(res != 0) {
    perror("Semaphore creation failed");
    exit(EXIT_FAILURE);
  }
  return sem_student;
}

void destroy_sem_student(Student* student) {
  int res = 0;

  res = sem_destroy(student->sem_student);
  if(res != 0) {
    perror("Destroying semaphore failed");
    exit(EXIT_FAILURE);
  }
}

monitor_vars* init_monitor() {
  int res = 0;
  monitor_vars* monitor_vars_ptr = malloc(sizeof(monitor_vars));
  if(monitor_vars_ptr == NULL) {
    perror("malloc failed, exiting...");
    exit(EXIT_FAILURE);
  }

  res = pthread_mutex_init(&monitor_vars_ptr->lock, NULL);
  if(res != 0) {
    perror("Mutex creation failed");
    exit(EXIT_FAILURE);
  }

  res = pthread_cond_init(&monitor_vars_ptr->insufficient_weight, NULL);
  if(res != 0) {
    perror("Condition Variable creation failed");
    exit(EXIT_FAILURE);
  }
  return monitor_vars_ptr;
}

void destroy_monitor(monitor_vars* monitor_vars_ptr) {
  int res = 0;
  res = pthread_mutex_destroy(&monitor_vars_ptr->lock);
  if(res != 0) {
    perror("Destroying mutex failed");
    exit(EXIT_FAILURE);
  }
  res = pthread_cond_destroy(&monitor_vars_ptr->insufficient_weight);
  if(res != 0) {
    perror("Destroying condition variable failed");
    exit(EXIT_FAILURE);
  }
  free(monitor_vars_ptr);
}

gym_monitor.h

#ifndef MONITOR_H
#define MONITOR_H

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

#define REST        'R'
#define WORKOUT     'W'
#define GET_WEIGHTS 'G'
#define PUT_WEIGHTS 'P'

struct Student;

typedef struct {
  pthread_mutex_t lock;
  pthread_cond_t insufficient_weight;
}monitor_vars;

/*
*   Manages the weight distribution among the students.
*   Students enter the function, lock the mutex and try to obtain
*   their corresponding weight. If the weight is not obtainable
*   the student is waiting on a condition variable. Otherwise the
*   student picks the weight and unlocks the mutex.
*
*   parameter - the student
*   return    - none
*/
void get_weights(struct Student* stud);

/*
*   Coordinates the transfer of weights from students back to the
*   weight rack inside a monitor.
*
*   parameter - the student
*   return    - none
*/
void put_weights(struct Student* stud);

/*
*   Student waits on a semaphore
*
*   parameter - the student
*   return    - none
*/
void rest_student(struct Student* stud);

/*
*   Student waiting on a semaphore is unblocked
*
*   parameter - the student
*   return    - none
*/
void wake_student(struct Student* stud);

/*
*   Creates, initializes and returns a struct of monitor_vars.
*   The struct contains a mutex and a condition variable.
*   Exits the program if either memory allocation or initializing fails.
*
*   parameter - none
*   return    - the mutex and condition variable inside a monitor_vars struct
*/
monitor_vars* init_monitor();

/*
*   Destroys the supplied monitor struct.
*   Exits the program if destroying the monitor fails.
*
*   parameter - the monitor struct to be destroyed
*   return    - none
*/
void destroy_monitor(monitor_vars* mon);

/*
*   Creates, initializes and returns a semaphore.
*   Exits the program if either memory allocation or initializing fails.
*
*   parameter - none
*   return    - the initialized semaphore
*/
sem_t* init_sem_student();

/*
*   Destroys the semaphore of the supplied student.
*   Exits the program if destroying the semaphore fails.
*
*   parameter - the student
*   return    - none
*/
void destroy_sem_student(struct Student* student);

#endif

Answer 1

Before I go into details, I feel obliged to say that your project is fairly ambitious for a C beginner. I have several criticisms, which I hope you will receive as constructive, but overall, your code makes me suspect that although you are new to C, you are not new to programming in general or to multi-threaded programming in particular. If I am mistaken about that then please take it as a complement.

Data races

Your code contains some data races involving the manipulation and testing of students' status variables. The main thread modifies these as a result of command input and at shutdown, and each student's thread both reads and writes that variable for its own student. Some of the student threads' accesses are performed under protection of the mutex (those in functions from gym_routine.c), but others and the main thread's are not.

Since these variables are written to by at least one thread each and read by multiple threads, every access must be appropriately protected once the per-student threads are started. You've apparently chosen to use a mutex for that, which is fine; you just need to be sure to protect all accesses.

Busy loops

You use high-iteration-count busy loops for making the workout() and rest() functions consume non-trivial time. At minimum, you'll need to greatly reduce the iteration counts when you correct the data races there, as locking and unlocking mutexes is costly.

Really, however, you ought to choose a delay mechanism that doesn't consume CPU. pthread_cond_timedwait() provides one such mechanism, with the advantage that another thread (e.g. the main one) can interrupt the wait if needed. That could be made to work in concert with resolving your data races, by giving each student its own mutex and condition variable to protect access to the student status. That would also allow you to set the durations of the workout and rest times in terms of machine-independent time units.

Unnecessary dynamic allocation

There are many good uses for dynamic allocation, but it's complicated enough and easy enough to mismanage that you should not use it where you don't actually need it. In particular, just because you need a pointer to something does not necessarily mean that that thing needs to be dynamically allocated. It's not uncommon to use a pointer to an ordinary local or file-scope variable, obtained via the & operator.

In your code, this applies to most (but, oddly, not all) of your synchronization objects. For example, I recommend changing the sem_student member of struct Student from a pointer to a plain sem_t. (It will then need to be handled slightly differently, but mainly the dynamic allocation will go away.) Similarly, there is no need to dynamically allocate your monitor_vars object. Just declare an instance.

Input handling

In your main input loop, you should account for the possibility that fgets() returns NULL (indicating end-of-file before any input is read, or error). On the other hand, you probably do not have to account for input[0] == '\0' because fgets() always copies at least one character from input to buffer upon success (provided you specify a buffer size of at least 2). Literal '\0' characters in the input could conceivably trip you up, but if you need to accommodate those then you need to handle input altogether differently.

If you mean to accept only one command per input line, then I'd recommend consuming the balance of the line, up to the next newline, at the bottom of each iteration of the input loop. The one thing to watch out for there would be input lines containing a newline at index 1, which your current code will reject as invalid, but for which the trailing newline will already have been read.

Issues with headers and #includes

As a matter mostly of style, each of your headers should #include those headers defining constants and identifiers used directly by that header, if any, but not any other headers. (Each C source file should do the same.) Do not otherwise have your headers include other headers; it is unnecessary, and under some circumstances it can be harmful.

For example, given it's current contents, your gym_monitor.h is right to include pthread.h and semaphore.h, but there appears to be no reason for it to include stdio.h. On the other hand, I would encourage having it include main.h for the definition of struct Student, or else to combine those two headers into one.

As a separate matter, it is a good idea to ensure that each source file and header that #includes headers includes them in the same relative order. This is less important for standard library headers, but there's no good reason to distinguish. It can be the case that changing the order of headers changes their interpretation (which would be a weakness of one or more of the headers involved, but sometimes that happens). In your case, your files differ on the order of gym_monitor.h and main.h.

Answer 2

1. Avoid stinginess with user input buffers. Better to balance the idea of a "right-sized" buffer (3) with the idea users from time-to-time will enter excessive text. Better to read the line than leave extra text in stdin. IN general, suggest 2x the expected maximum need or create code that will consume excessive text from the line of user input..

   // #define MAX_INPUT_SIZE 3
   #define MAX_INPUT_SIZE (3*2)
   ...
   char input[MAX_INPUT_SIZE] = {0};
   while(strncasecmp(fgets(input, MAX_INPUT_SIZE, stdin),"q", 1))  {
   

2. A simple way to lop off the potential trailing '\n' is:

   input[strcspn(input, "\n")] = '\0';
   

3. Unclear of the value of fflush(stdout); right after fgets(). If anything, coding that before the fgets() may add value.

4. Consider coding a variable to help detail what the NULL is about.

   // pthread_barrier_init(&gym_routine_barrier, NULL, NR_STUDENTS);
   const pthread_barrierattr_t *attr = NULL;
   pthread_barrier_init(&gym_routine_barrier, attr, NR_STUDENTS);
   

5. Robust code would check the result of pthread_barrier_init().

6. When objects are constant, using const helps to convey that to reviewers and helps with some compiler optimizations.

   // int students_weights[] = {WEIGHTS_ANNA,WEIGHTS_BERND, ...
   const int students_weights[] = {WEIGHTS_ANNA,WEIGHTS_BERND, ...
   

7. For array indexing and sizing size_t is the Goldilocks type, neither too narrow or too wide, just the right size for all indexing.

   // for( int i = 0; i < NR_STUDENTS; i++ ) {
   for( size_t i = 0; i < NR_STUDENTS; i++ ) {
     students[i].thread_id = i;
   

8. Name space organization: Inside gym_monitor.h, IDs like REST, Student, monitor_vars occur. In *.c files that use these identifiers it is not clear where they originated. A define like REST can certainly collide with other code. Perhaps using gym_REST, gym_Student or some other scheme would help.

9. Flexible code: The size of consistency_check[] varies with enum weight_names. So using a fixed int consistency_check[] = {0,0,0} is a problem should the number of weight names change. suggest:

   enum weight_names{
     KG_2,
     KG_3,
     KG_5,
     weight_names_N 
   };
   int consistency_check[weight_names_N] = {0};
   

GTG