Circular buffer implementation in C testing issue

Luke, like EOF said, you're going to have race conditions in your code (probably several). Here are a few thoughts I had when I saw your code:

When threads share memory, you need to have protections on that memory so that only one thread can access it at a time. You can do this with a mutex (see pthread_mutex_lock and pthread_mutex_unlock).

Also, I noticed that your if statements in your read and write threads check the result of push and pop to see if a character was actually pushed or popped. While that might work, you should probably use semaphores to synchronize your threads instead. Here's what I mean by that: When your write thread writes to the buffer, you should have something like sem_post(&buff_count_sem);. Then, before your read thread reads a character, you should have sem_wait(&buff_count_sem); so that you know there is at least one character in the buffer.

The same principle applies to the read thread. I suggest the read thread having sem_post(&space_left_sem) just after popping a byte from the buffer and the write thread having sem_wait(&space_left_sem) just before pushing to the buffer to ensure that there is room in the buffer for the byte you are trying to write.

Here are my suggested changes to your code (not tested):

#include <stdio.h> #include <stdlib.h> #include <string.h> #include <pthread.h> #include <time.h> #include <unistd.h> #define MAX_NUM_ITEM 10000 #define HASH_SIZE 32 long write_count = 0; long read_count = 0; pthread_mutex_t buff_mutex; sem_t buff_count_sem; sem_t space_left_sem; struct cBuf { int first; int last; int max_items; int item_size; int valid_items; unsigned char *buffer; }; void init_cBuf(struct cBuf *buf, int max_items, int item_size) { buf -> first = 0; buf -> last = 0; buf -> max_items = max_items; buf -> item_size = item_size; buf -> valid_items = 0; buf -> buffer = calloc(max_items, item_size); return; } int isEmpty(struct cBuf *buf) { if (buf -> valid_items == 0) { return 1; } else { return 0; } } int push(struct cBuf *buf, unsigned char *data) { pthread_mutex_lock(&buff_mutex); if (buf -> valid_items >= buf -> max_items) { // buffer full pthread_mutex_unlock(&buff_mutex); return -1; } else { // push data into the buffer memcpy(buf -> buffer + (buf -> last) * (buf -> item_size), data, buf -> item_size); // update cBuf info buf -> valid_items++; buf -> last = (buf -> last + 1) % (buf -> max_items); pthread_mutex_unlock(&buff_mutex); return 0; } } int pop(struct cBuf *buf, unsigned char *new_buf) { phthread_mutex_lock(&buff_mutex); if (isEmpty(buf)) { // buffer empty pthread_mutex_unlock(&buff_mutex); return -1; } else { // read data memcpy(new_buf, buf -> buffer + (buf -> first) * (buf -> item_size), buf -> item_size); // update cBuf info buf -> first = (buf -> first + 1) % (buf -> max_items); buf -> valid_items--; pthread_mutex_unlock(&buff_mutex); return 0; } } void *write_hash(void *ptr) { struct cBuf *buf = (struct cBuf *)(ptr); while (1) { unsigned char *hash = malloc(HASH_SIZE); // for simplicity I just create some data with 32-byte. sem_wait(&space_left_sem); push(buf, hash); write_count++; sem_post(&buff_count_sem); free(hash); if (write_count == MAX_NUM_ITEM) { break; } } printf (" thread id = %lu\n", (long unsigned) (pthread_self ())); printf (" total write = %lu\n\n", write_count); return NULL; } void *read_hash(void *ptr) { struct cBuf *buf = (struct cBuf *)(ptr); unsigned char *new_buf = malloc(HASH_SIZE); while (1) { sem_wait(&buff_count_sem); pop(buf, new_buf); read_count++; sem_post(&space_left_sem); if (read_count == MAX_NUM_ITEM) { break; } } free(new_buf); printf (" thread id = %lu\n", (long unsigned) (pthread_self ())); printf (" total read = %lu\n\n", read_count); } int main(int argc, char const *argv[]) { struct cBuf buf; init_cBuf(&buf, 200, HASH_SIZE); pthread_t write_thd, read_thd; pthread_mutex_init(&buff_mutex); sem_init(&buff_count_sem, 0, 0); // the last parameter is the initial value - initially 0 if no data is in the buffer sem_init(&space_left_sem, 0, buf.max_items); double diff = 0.0, t1 = 0.0, t2 = 0.0; t1 = clock (); pthread_create(&read_thd, NULL, read_hash, &buf); pthread_create(&write_thd, NULL, write_hash, &buf); pthread_join(write_thd, NULL); pthread_join(read_thd, NULL); t2 = clock (); diff = (double)((t2 - t1) / CLOCKS_PER_SEC); printf ("----------------\nTotal time: %lf second\n", diff); printf ("Total write: %lu\n", write_count); printf ("write per-second: %lf\n\n", write_count / diff); return 0; } 

I suggest reading up on how to properly use threads.

UPDATE: 1. Fixed a typo. 2. You'll also want to have mutex_lock and mutex_unlock in isEmpty and any other functions that you write for your circular buffers where multiple threads might access the same buffer.