2
\$\begingroup\$

We're preparing for an exam at the moment, and our lecturer has given us a sample problem to work on. I have it completed, but would like to know a) If it is actually doing what it's supposed to, and b) if it could be done more efficiently or there are any questionable coding aspects. It certainly seems to be working, and GDB tells me the extra threads are definitely bring created.

The question:

Implement a multithreaded car park simulator in C. One thread should move cars into the car park and another thread should take cars out of the car park (these steps can be simulated by simply inserting integers into/removing integers from a buffer).

The capacity of the car park should be supplied as a command line parameter to your program.

A monitor thread should periodically print out the number of cars currently in the car park.

Requirements:

  • Implement mutual exclusion where appropriate
  • Do not remove cars from an empty car park
  • Do not add cars to a full car park
  • Avoid busy-waiting
  • Have each producer/consumer thread pause for a random period (up to 1s) between inserting/removing a car
  • Have the monitor thread periodically print out the number of cars currently in the car park

Part 2 - Add one additional producer thread and one additional consumer thread to your solution to Q1. Use pthread_barrier_init and pthread_barrier_wait to ensure that all producer/consumer threads begin producing/consuming at the same time.

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

#define ONE_SECOND 1000000
#define RANGE 10
#define PERIOD 2
#define NUM_THREADS 4 

typedef struct {
  int *carpark;
  int capacity;
  int occupied;
  int nextin;
  int nextout;
  int cars_in;
  int cars_out;
  pthread_mutex_t lock;
  pthread_cond_t space;
  pthread_cond_t car;
  pthread_barrier_t bar;
} cp_t;

static void * car_in_handler(void *cp_in);
static void * car_out_handler(void *cp_in);

static void * monitor(void *cp_in);
static void initialise(cp_t *cp, int size);


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

    if (argc != 2) { 
        printf("Usage: %s carparksize\n", argv[0]);
        exit(1);
    }

    cp_t ourpark;

    initialise(&ourpark, atoi(argv[1]));

    pthread_t car_in, car_out, m;
    pthread_t car_in2, car_out2;

    pthread_create(&car_in, NULL, car_in_handler, (void *) &ourpark);
    pthread_create(&car_out, NULL, car_out_handler, (void *) &ourpark);
    pthread_create(&car_in2, NULL, car_in_handler, (void *) &ourpark);
    pthread_create(&car_out2, NULL, car_out_handler, (void *) &ourpark);
    pthread_create(&m, NULL, monitor, (void *) &ourpark);

    pthread_join(car_in, NULL);
    pthread_join(car_out, NULL);
    pthread_join(car_in2, NULL);
    pthread_join(car_out2, NULL);
    pthread_join(m, NULL);

    exit(0);


} 

static void initialise(cp_t *cp, int size) { 

    cp->occupied = cp->nextin = cp->nextout = cp->cars_in = cp->cars_out = 0;
    cp->capacity = size;

    cp->carpark = (int *)malloc(cp->capacity * sizeof(*cp->carpark));

    pthread_barrier_init(&cp->bar, NULL, NUM_THREADS);


    if(cp->carpark == NULL) {
        perror("malloc()");
        exit(1);
    }

    srand((unsigned int)getpid());

    pthread_mutex_init(&cp->lock, NULL);
    pthread_cond_init(&cp->space, NULL);
    pthread_cond_init(&cp->car, NULL);
} 

static void* car_in_handler(void *carpark_in) {

    cp_t *temp;
    unsigned int seed;
    temp = (cp_t *)carpark_in;

    pthread_barrier_wait(&temp->bar);
    while(1) { 

        usleep(rand_r(&seed) % ONE_SECOND);

        pthread_mutex_lock(&temp->lock);

        //while full wait until there is room available
        while (temp->occupied == temp->capacity)
            pthread_cond_wait(&temp->space, &temp->lock);

        //insert an item
        temp->carpark[temp->nextin] = rand_r(&seed) % RANGE;

        //increment counters

        temp->occupied++;
        temp->nextin++;
        temp->nextin %= temp->capacity; //circular buffer here then
        temp->cars_in++;

         //someone may be waiting on data to become available
                pthread_cond_signal(&temp->car);

          //release the lock
                pthread_mutex_unlock(&temp->lock);

    }
    return ((void *)NULL);

}

static void* car_out_handler(void *carpark_out) {

    cp_t *temp;
    unsigned int seed;
    temp = (cp_t *)carpark_out;
    pthread_barrier_wait(&temp->bar);
    for(; ;) { 

    usleep(rand_r(&seed) % ONE_SECOND);

        //acquire the lock
        pthread_mutex_lock(&temp->lock);

    while(temp->occupied == 0) 
            pthread_cond_wait(&temp->car, &temp->lock);

    //increment counters
    temp->occupied--;
    temp->nextout++;
        temp->nextout %= temp->capacity;
        temp->cars_out++;


        //somebody may be waiting on toom to become available
                pthread_cond_signal(&temp->space);

         //release the locl
                pthread_mutex_unlock(&temp->lock);


    }
    return ((void *)NULL);

}

static void *monitor(void *carpark_in) {

    cp_t *temp;
    temp = (cp_t *)carpark_in;

    for(; ;) { 
    sleep(PERIOD);

    //acquire the lock
    pthread_mutex_lock(&temp->lock);
    printf("Delta: %d\n", temp->cars_in - temp->cars_out - temp->occupied);
    printf("Number of cars in carpark: %d\n", temp->occupied);

    //release the lock
pthread_mutex_unlock(&temp->lock);

    }

    return ((void *)NULL);
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Looks good. Only issues is your indentation is not consistent (probably because you are mixing tabs a char most companies will tell you to pick one or the other but not to mix). Also be consistent. Here you choose two different methods for infinite loop (choose one and use consistently). Also because the child threads never end rather than try and cleanup in main (added a big error message saying threads have unexpectedly ended). \$\endgroup\$ Commented Aug 12, 2012 at 15:09
  • \$\begingroup\$ Thank you for the feedback. The indentation I think was me getting the four spaces bit wrong in parts. \$\endgroup\$
    – Saf
    Commented Aug 13, 2012 at 15:48
  • \$\begingroup\$ The code above has changed a lot due to the feedback, the original is here is anyone wants to see it - pastebin.com/rbnA2QHK \$\endgroup\$
    – Saf
    Commented Aug 14, 2012 at 12:57

2 Answers 2

5
\$\begingroup\$

I have a few minor points:

  • put main() last to avoid the need for prototypes.

  • any reason for exit(0) instead of return 0 (or EXIT_SUCCESS) in main() ?

  • in initialise() don't cast the return from malloc()

  • create the barrier after checking carpark for NULL ?

  • use EXIT_FAILURE instead of 1

  • in threads, the name temp is badly chosen. I'd prefer cp or carpark.

  • I'd prefer to see temp initialised immediately, as in cp_t *temp = carpark_in; and note that no cast is needed.

  • is your usleep really random? (note that although the number N returned by rand_r() may be random, it may be unsafe to believe that N%100000 is random)

  • I would put the random sleep into a spearate function

  • what is the purpose of the random value in the carpark[] buffer?

  • cars_in/out seem to be redundant

  • no need to cast return values: ((void*) NULL) should be just NULL

  • toom and locl typos in car_out_handler

  • in monitor, 'Delta' is not in the spec

  • various spurious/inconsistent blank lines make code look a little sloppy

  • is it necessary to declare variables away from start of functions? I know you can, but does it help at all?

  • some comments seem like 'noise', eg. 'release the lock'

  • I'd like to see a statement of the problem at the top, along with a statement of any assumptions you have made in the solution

\$\endgroup\$
8
  • \$\begingroup\$ Thank you so much for such a detailed response. I'll work on it and see what I can come up with. Should I edit the code above or just post the newer version in a comment? \$\endgroup\$
    – Saf
    Commented Aug 13, 2012 at 15:49
  • \$\begingroup\$ @Saf - probably best to edit it. \$\endgroup\$ Commented Aug 14, 2012 at 3:20
  • \$\begingroup\$ OK. I've edited the code and made most of the changes you mentioned, it's a hell of a lot cleaner now, thank you! Perhaps you could have another quick look at it. In response to some of your points - I changed exit(0) to return(EXIT_SUCCESS) - I'm assuming this point was about readability and not actual code effect (I did some reading after you mentioned it, return only makes a difference in main in C++ ad far as I can tell) \$\endgroup\$
    – Saf
    Commented Aug 14, 2012 at 12:42
  • \$\begingroup\$ On the whole sleep randomness here, it's not a major issue if it not truly random, it's just so that the program doesn't run one car in, one car out. \$\endgroup\$
    – Saf
    Commented Aug 14, 2012 at 12:43
  • 1
    \$\begingroup\$ Looks a lot nicer now :-) As you say, exit/return from main are equivalent. Regarding comments, your statement of the task would be best included in the C file when you submit the work. By 'assumptions' I meant specifying anything that was not explicitly stated in the task (eg. that you put a random number in the next consecutive position in the buffer for each new car; removing a car does not affect the buffer). One minor point is to validate the value passed in argv[1] in some arbitrary way (again, stated in your assumptions). Also minor, return (NULL); is better as return NULL; \$\endgroup\$ Commented Aug 14, 2012 at 20:02
2
\$\begingroup\$

In addition to @William Morris' great list of points:

  • For clarity and maintainability, I'd rename ONE_SECOND to specify the applicable unit of time. Depending on the particular time function to be used, you may likely need microseconds or nanoseconds or possibly both. If needed, have a macro for both cases.

  • usleep() has already been declared obsolete and should be replaced with nanosleep(). Compilation under GNU99 mode may also be necessary.

  • Your loops, mostly the for loops, are lacking indentation for some reason. Since curly braces are still in use, indentation is still necessary. Also, be sure to keep it all consistent. There are some lines in these loops that are misaligned with the others.

    In addition, consider using curly braces for single-line loops as well. This can help improve maintainability and avoid some bugs associated with this.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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