HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Lock free MPMC Ring buffer implementation in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mpmcfreeimplementationringbufferlock

Problem

I have written a lock free MPMC FIFO in C based on a ring buffer. It uses gcc's atomic built-ins to achieve thread safety. The queue is designed to return -1 if it's full on enqueue or empty on dequeue.

After some feedback, I've changed by design. I have tested this code to work, but testing multithreaded code is hardly enough to prove it's correct.

struct queue
{
    void** buf;
    size_t num;
    uint64_t writePos;
    uint64_t readPos;
};

queue_t* createQueue(size_t num)
{
    struct queue* new = xmalloc(sizeof(*new));
    new->buf = xmalloc(sizeof(void*) * num);
    memset(new->buf, 0, sizeof(void*)*num);
    new->readPos = 0;
    new->writePos = 0;
    new->num = num;
    return new;
}

void destroyQueue(queue_t* queue)
{
    if(queue)
    {
        xfree(queue->buf);
        xfree(queue);
    }
}

int enqueue(queue_t* queue, void* item)
{
    for(int i = 0; i buf[queue->writePos % queue->num], NULL, item))
        {
            __sync_fetch_and_add(&queue->writePos, 1);
            return 0;
        }
    }
    return -1;
}

void* dequeue(queue_t* queue)
{
    for(int i = 0; i buf[queue->readPos % queue->num];
        if(value && __sync_bool_compare_and_swap(&queue->buf[queue->readPos % queue->num], value, NULL))
        {
            __sync_fetch_and_add(&queue->readPos, 1);
            return value;
        }
    }
    return NULL;
}


(link to the previous iteration)

Solution

Did you test it?

I think you have a problem if a thread calls enqueue and is interrupted
just before the memcpy, having calculated pos to be slot-1. If another
thread now calls enqueue and completes it will write to slot-2 and increment the pendingRead counter, leaving
slot-1 still unwritten. If a third thread now reads before the first thread
can complete its write, the reader will read from the unwritten slot-1. This
is becase the second thread correctly set the queue to say there is a pending
read (slot-2) but dequeue assumes the data must be in the next available
slot according to readPos.

A few other comments:

-
queue_t is defined (elsewhere) as a pointer type. I prefer pointers to be
explicit.

-
You have mixed use of struct queue * and queue_t

-
Return from malloc is not checked.

-
buf in struct queue should be void* not char **

-
Why are writePos and readPos of type uint64_t and not size_t?
After all, num is size_t. Why the difference?

-
Loss of precision assigning num in createQueue

Context

StackExchange Code Review Q#22915, answer score: 4

Revisions (0)

No revisions yet.