"c pthreads - issues with thread-safe queue implementation" Code Answer

i found a few more bugs.

i've annotated your code. i took a bit from your first posting and your second. i've fixed the code, showing before and after [please pardon the gratuitous style cleanup]:

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

static pthread_mutex_t task_mutex = pthread_mutex_initializer;

typedef struct task {
    int number;
} task;

typedef struct cell {
// note/bug: this should be a pointer to the task. otherwise, dequeue gets
// messy
#if 0
    task t;
#else
    task *t;
#endif
    struct cell *next;
} cell;

typedef struct tqueue {
    struct cell *head;
    struct cell *tail;
} tqueue;

void
startqueue(tqueue *queue)
{

#if 0
    queue->head = malloc(sizeof(cell));
#else
    queue->head = null;
#endif
    queue->tail = null;
}

int
empty(tqueue *queue)
{

    // note/bug: dequeue never touches tail, so this test is incorrect
#if 0
    return (queue->head == queue->tail);
#else
    return (queue->head == null);
#endif
}

void
enqueue(tqueue *queue, task *t)
{
    cell *p;

    pthread_mutex_lock(&task_mutex);

    p = malloc(sizeof(cell));
    p->next = null;
    p->t = t;

    if (queue->tail == null) {
        queue->tail = p;
        queue->head = p;
    }
    else {
        queue->tail->next = p;
        queue->tail = p;
    }

    pthread_mutex_unlock(&task_mutex);
}

task *
dequeue(tqueue *queue)
{
    task *t;

    pthread_mutex_lock(&task_mutex);

    if (empty(queue))
        t = null;

    else {
        cell *p = queue->head;

        if (p == queue->tail)
            queue->tail = null;

        queue->head = p->next;

        // note/bug: this is setting t to the second element in the list,
        // not the first
        // note/bug: this is also undefined behavior, in original code (with
        // original struct definition), because what t points to _does_ get
        // freed before return
#if 0
        t = &queue->head->t;
#else
        t = p->t;
#endif

        free(p);
    }

    pthread_mutex_unlock(&task_mutex);

    return t;
}

void *
work(void *arg)
{

    tqueue *queue = (tqueue *) arg;

    // note/bug: this gets orphaned on the first call to dequeue
#if 0
    task *t = malloc(sizeof(task));
#else
    task *t;
#endif

    for (t = dequeue(queue); t != null; t = dequeue(queue))
        printf("%d ", t->number);

    // note/bug: this frees some cell allocated in main -- not what we want
#if 0
    free(t);
#endif

    pthread_exit(null);
    return 0;
}

// for a simple test i runned this on main:

int
main()
{

    tqueue *queue = malloc(sizeof(tqueue));

    startqueue(queue);

    pthread_t threads[3];
    task t[3];

    for (int i = 0; i < 3; i++) {
        t[i].number = i + 1;
#if 0
        enqueue(queue, t);
#else
        enqueue(queue, &t[i]);
#endif
    }

    for (int i = 0; i < 3; i++)
        pthread_create(&threads[i], null, work, (void *) queue);

    for (int i = 0; i < 3; i++)
        pthread_join(threads[i], null);

    return 0;
}

update:

are the threads executing the tasks concurrently ? i've been testing the cpu usage with htop and i can only max the usage of a single core out of four.

a few things to keep in mind. htop probably won't show much on programs that have such a short running time. even with 10,000 queue entries this program executes in 20ms.

it's better to have the program itself print the information [see below]. note that printf does thread locking on stdin so it may contribute to the "serial" nature of the program. it also contributes a significant amount to the execution time of the program (i.e. the printf is much slower than the dequeue)

also, one thread (i.e. the first one) could monopolize the queue and drain all entries before the others have a chance to run.

the os may [is at liberty to] schedule all threads on a single core. it may then "migrate" them later (e.g. within a second or so).

i've enhanced the program to include some timing information in the output print that may help show more of what you'd like to see. also, i've added command line options to control the number of threads and number of items queued. this is similar to what i do for some of my own programs. divert the program output to a log file and examine it. play around with the options on multiple runs

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <malloc.h>
#include <time.h>

int opt_n;                              // suppress thread output
int opt_t;                              // number of threads
int opt_q;                              // number of queue items

static pthread_mutex_t task_mutex = pthread_mutex_initializer;

double tvzero;

typedef struct task {
    int number;
} task;

typedef struct cell {
    task *t;
    struct cell *next;
} cell;

typedef struct tqueue {
    struct cell *head;
    struct cell *tail;
} tqueue;

typedef struct thread {
    pthread_t tid;
    int xid;
    tqueue *queue;
} thread;

double
tvgetf(void)
{
    struct timespec ts;
    double sec;

    clock_gettime(clock_realtime,&ts);
    sec = ts.tv_nsec;
    sec /= 1e9;
    sec += ts.tv_sec;

    sec -= tvzero;

    return sec;
}

void
startqueue(tqueue *queue)
{

    queue->head = null;
    queue->tail = null;
}

int
empty(tqueue *queue)
{

    return (queue->head == null);
}

void
enqueue(tqueue *queue, task *t)
{
    cell *p;

    pthread_mutex_lock(&task_mutex);

    p = malloc(sizeof(cell));
    p->next = null;
    p->t = t;

    if (queue->tail == null) {
        queue->tail = p;
        queue->head = p;
    }
    else {
        queue->tail->next = p;
        queue->tail = p;
    }

    pthread_mutex_unlock(&task_mutex);
}

task *
dequeue(tqueue *queue)
{
    task *t;

    pthread_mutex_lock(&task_mutex);

    if (empty(queue))
        t = null;

    else {
        cell *p = queue->head;

        if (p == queue->tail)
            queue->tail = null;

        queue->head = p->next;

        t = p->t;

        free(p);
    }

    pthread_mutex_unlock(&task_mutex);

    return t;
}

void *
work(void *arg)
{
    thread *tskcur = arg;
    tqueue *queue = tskcur->queue;
    task *t;
    double tvbef;
    double tvaft;

    while (1) {
        tvbef = tvgetf();
        t = dequeue(queue);
        tvaft = tvgetf();

        if (t == null)
            break;

        if (! opt_n)
            printf("[%.9f/%.9f %5.5d] %dn",
                tvbef,tvaft - tvbef,tskcur->xid,t->number);
    }

    return (void *) 0;
}

// for a simple test i runned this on main:

int
main(int argc,char **argv)
{
    char *cp;
    tqueue *queue;
    task *t;
    thread *tsk;

    --argc;
    ++argv;

    for (;  argc > 0;  --argc, ++argv) {
        cp = *argv;
        if (*cp != '-')
            break;

        switch (cp[1]) {
        case 'n':  // suppress thread output
            opt_n = 1;
            break;

        case 'q':  // number of queue items
            opt_q = atoi(cp + 2);
            break;

        case 't':  // number of threads
            opt_t = atoi(cp + 2);
            break;

        default:
            break;
        }
    }

    tvzero = tvgetf();

    queue = malloc(sizeof(tqueue));
    startqueue(queue);

    if (opt_t == 0)
        opt_t = 16;
    thread threads[opt_t];

    if (opt_q == 0)
        opt_q = 10000;
    t = malloc(sizeof(task) * opt_q);

    for (int i = 0; i < opt_q; i++) {
        t[i].number = i + 1;
        enqueue(queue, &t[i]);
    }

    for (int i = 0; i < opt_t; i++) {
        tsk = &threads[i];
        tsk->xid = i + 1;
        tsk->queue = queue;
        pthread_create(&tsk->tid, null, work, tsk);
    }

    for (int i = 0; i < opt_t; i++) {
        tsk = &threads[i];
        pthread_join(tsk->tid, null);
    }

    printf("total: %.9fn",tvgetf());

    free(t);

    return 0;
}

update #2:

also, one thread (i.e. the first one) could monopolize the queue and drain all entries before the others have a chance to run." what can be done in that case ?

a few things.

pthread_create takes a bit of time, allowing thread 1 to go while the others are still being created. a way to ameliorate this is to create all threads, each thread sets an "i am running" flag (in its thread control block). the main thread waits for all threads to set this flag. then, the main thread sets a global volatile "you_may_now_all_run" flag that each thread spins on before entering its primary thread loop. in my experience, they all start running within microseconds of each other [or better].

i didn't implement this in the updated code below, so you can experiment with it yourself [along with the nanosleep].

mutexes are pretty fair overall [under linux, at least] because a blocked thread will get queued, waiting on the mutex. as i mentioned in the comments, a nanosleep can also be used, but this [somewhat] defeats the purpose as the threads will slow down.

the antidote to thread starvation is "fairness". as i mentioned, there is an elaborate algorithm for fairness without waiting. it is the kogan/petrank algorithm: http://www.cs.technion.ac.il/~erez/papers/wf-methodology-ppopp12.pdf this is really a bit involved/advanced, so caveat emptor ...

however, a compromise may be a ticket lock: https://en.wikipedia.org/wiki/ticket_lock

i've reworked the program again. it has options for pooled allocation, ticket vs. mutex lock, and deferred printing of log entries. it also cross-checks the results between threads to ensure none of them got duplicate entries.

of course, the key to all this is accurate, high precision logging (i.e. if you can't measure it, you can't tune it).

for example, one would think that doing free inside dequeue would be slower than simply releasing the cell to a resuable pool (similar to a slab allocator), but, the performance boost wasn't as great as expected. this could be that glibc's malloc/free is just blazing fast [which is what they claim].

these various versions should give you some ideas of how to build up your own performance measurement suite.

anyway, here's the code:

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <stdatomic.h>
#include <malloc.h>
#include <errno.h>
#include <string.h>
#include <time.h>

int opt_p;                              // print thread output immediately
int opt_t;                              // number of threads
int opt_q;                              // number of queue items
int opt_l;                              // use ticket lock
int opt_m;                              // use fast cell alloc/free

typedef unsigned char byte;
typedef unsigned int u32;

#define sysfault(_fmt...) 
    do { 
        fprintf(stderr,_fmt); 
        exit(1); 
    } while (0)

// lock control
typedef struct anylock {
    pthread_mutex_t mutex;              // standard mutex
    volatile u32 seqreq;                // ticket lock request
    volatile u32 seqacq;                // ticket lock grant
} anylock;

// work value
typedef struct task {
    union {
        struct task *next;
        int number;
    };
} task;

// queue item
typedef struct cell {
    struct cell *next;
    task *t;
} cell;

// queue control
typedef struct tqueue {
    struct cell *head;
    struct cell *tail;
} tqueue;

// thread log entry
typedef struct log {
    double tvbef;
    double tvaft;
    int number;
} log;

#define btvoff(_off) 
    ((_off) >> 3)
#define btvmsk(_off) 
    (1u << ((_off) & 0x07))

#define btvlen(_len) 
    ((_len) + 7) >> 3

// thread control
typedef struct thread {
    pthread_t tid;
    int xid;
    tqueue *queue;
    log *log;
    byte *bitv;
} thread;

static inline byte
btvset(byte *bitv,long off)
{
    u32 msk;
    byte oval;

    bitv += btvoff(off);
    msk = btvmsk(off);

    oval = *bitv & msk;

    *bitv |= msk;

    return oval;
}

anylock task_mutex;
anylock print_mutex;
double tvzero;
cell *cellpool;                         // free pool of cells
long bitvlen;

#define barrier 
    __asm__ __volatile__("" ::: "memory")

// virtual function pointers
cell *(*cellnew)(void);
void (*cellfree)(cell *);
void (*lock_acquire)(anylock *lock);
void (*lock_release)(anylock *lock);

double
tvgetf(void)
{
    struct timespec ts;
    double sec;

    clock_gettime(clock_realtime,&ts);
    sec = ts.tv_nsec;
    sec /= 1e9;
    sec += ts.tv_sec;

    sec -= tvzero;

    return sec;
}

void *
xalloc(size_t cnt,size_t siz)
{
    void *ptr;

    ptr = calloc(cnt,siz);
    if (ptr == null)
        sysfault("xalloc: calloc failure -- %sn",strerror(errno));

    return ptr;
}

void
lock_wait_ticket(anylock *lock,u32 newval)
{
    u32 oldval;

    // wait for our ticket to come up
    // note: atomic_load is [probably] overkill here
    while (1) {
#if 0
        oldval = atomic_load(&lock->seqacq);
#else
        oldval = lock->seqacq;
#endif
        if (oldval == newval)
            break;
    }
}

void
lock_acquire_ticket(anylock *lock)
{
    u32 oldval;
    u32 newval;
    int ok;

    // acquire our ticket value
    // note: just use a garbage value for oldval -- the exchange will
    // update it with the correct/latest value -- this saves a separate
    // refetch within the loop
    oldval = 0;
    while (1) {
#if 0
        barrier;
        oldval = lock->seqreq;
#endif
        newval = oldval + 1;
        ok = atomic_compare_exchange_strong(&lock->seqreq,&oldval,newval);
        if (ok)
            break;
    }

    lock_wait_ticket(lock,newval);
}

void
lock_release_ticket(anylock *lock)
{

    // note: atomic_fetch_add is [probably] overkill, but leave it for now
#if 1
    atomic_fetch_add(&lock->seqacq,1);
#else
    lock->seqacq += 1;
#endif
}

void
lock_acquire_mutex(anylock *lock)
{

    pthread_mutex_lock(&lock->mutex);
}

void
lock_release_mutex(anylock *lock)
{

    pthread_mutex_unlock(&lock->mutex);
}

void
lock_init(anylock *lock)
{

    switch (opt_l) {
    case 1:
        lock->seqreq = 0;
        lock->seqacq = 1;
        lock_acquire = lock_acquire_ticket;
        lock_release = lock_release_ticket;
        break;

    default:
        pthread_mutex_init(&lock->mutex,null);
        lock_acquire = lock_acquire_mutex;
        lock_release = lock_release_mutex;
        break;
    }
}

void
startqueue(tqueue *queue)
{

    queue->head = null;
    queue->tail = null;
}

int
empty(tqueue *queue)
{

    return (queue->head == null);
}

// cellnew_pool -- allocate a queue entry
cell *
cellnew_pool(void)
{
    int cnt;
    cell *p;
    cell *pool;

    while (1) {
        // try for quick allocation
        p = cellpool;

        // bug out if we got it
        if (p != null) {
            cellpool = p->next;
            break;
        }

        // go to the heap to replenish the pool
        cnt = 1000;
        p = xalloc(cnt,sizeof(cell));

        // link up the entries
        pool = null;
        for (;  cnt > 0;  --cnt, ++p) {
            p->next = pool;
            pool = p;
        }

        // put this "online"
        cellpool = pool;
    }

    return p;
}

// cellfree_pool -- release a queue entry
void
cellfree_pool(cell *p)
{

    p->next = cellpool;
    cellpool = p;
}

// cellnew_std -- allocate a queue entry
cell *
cellnew_std(void)
{
    cell *p;

    p = xalloc(1,sizeof(cell));

    return p;
}

// cellfree_std -- release a queue entry
void
cellfree_std(cell *p)
{

    free(p);
}

void
enqueue(tqueue *queue, task *t)
{
    cell *p;

    lock_acquire(&task_mutex);

    p = cellnew();
    p->next = null;
    p->t = t;

    if (queue->tail == null) {
        queue->tail = p;
        queue->head = p;
    }
    else {
        queue->tail->next = p;
        queue->tail = p;
    }

    lock_release(&task_mutex);
}

task *
dequeue(tqueue *queue)
{
    task *t;

    lock_acquire(&task_mutex);

    if (empty(queue))
        t = null;

    else {
        cell *p = queue->head;

        if (p == queue->tail)
            queue->tail = null;

        queue->head = p->next;

        t = p->t;

        cellfree(p);
    }

    lock_release(&task_mutex);

    return t;
}

void *
work(void *arg)
{
    thread *tskcur = arg;
    tqueue *queue = tskcur->queue;
    task *t;
    log *log;
    long cnt;
    int tprev;
    byte *bitv;
    double tvbeg;
    double tvbef;
    double tvaft;

    log = tskcur->log;
    bitv = tskcur->bitv;
    tvbeg = tvgetf();

    tprev = 0;
    while (1) {
        tvbef = tvgetf();
        t = dequeue(queue);
        tvaft = tvgetf();

        if (t == null)
            break;

        // abort if we get a double entry
        if (btvset(bitv,t->number))
            sysfault("work: duplicaten");

        if (opt_p) {
            printf("[%.9f/%.9f %5.5d] %d [%d]n",
                tvbef,tvaft - tvbef,tskcur->xid,t->number,t->number - tprev);
            tprev = t->number;
            continue;
        }

        log->tvbef = tvbef;
        log->tvaft = tvaft;
        log->number = t->number;
        ++log;
    }

    if (! opt_p) {
        tvaft = tvgetf();

        cnt = log - tskcur->log;
        log = tskcur->log;

        lock_acquire(&print_mutex);

        printf("n");
        printf("thread=%5.5d start=%.9f stop=%.9f elap=%.9f total=%ldn",
            tskcur->xid,tvbeg,tvaft,tvaft - tvbeg,cnt);

        tprev = 0;
        for (;  cnt > 0;  --cnt, ++log) {
            printf("[%.9f/%.9f %5.5d] %d [%d]n",
                log->tvbef,log->tvaft - log->tvbef,tskcur->xid,
                log->number,log->number - tprev);
            tprev = log->number;
        }

        lock_release(&print_mutex);
    }

    return (void *) 0;
}

void
btvchk(thread *tska,thread *tskb)
{
    byte *btva;
    byte *btvb;
    byte aval;
    byte bval;
    int idx;

    printf("btvchk: %d ??? %dn",tska->xid,tskb->xid);

    btva = tska->bitv;
    btvb = tskb->bitv;

    // abort if we get overlapping entries between two threads
    for (idx = 0;  idx < bitvlen;  ++idx) {
        aval = btva[idx];
        bval = btvb[idx];
        if (aval & bval)
            sysfault("btvchk: duplicaten");
    }
}

// for a simple test i runned this on main:

int
main(int argc,char **argv)
{
    char *cp;
    tqueue *queue;
    task *t;
    thread *tsk;

    --argc;
    ++argv;

    for (;  argc > 0;  --argc, ++argv) {
        cp = *argv;
        if (*cp != '-')
            break;

        switch (cp[1]) {
        case 'p':  // print immediately
            opt_p = 1;
            break;

        case 'q':  // number of queue items
            opt_q = atoi(cp + 2);
            break;

        case 't':  // number of threads
            opt_t = atoi(cp + 2);
            break;

        case 'l':
            opt_l = 1;
            break;

        case 'm':
            opt_m = 1;
            break;

        default:
            break;
        }
    }

    printf("p=%d -- thread log is %sn",opt_p,opt_p ? "immediate" : "deferred");

    if (opt_t == 0)
        opt_t = 16;
    printf("t=%d (number of threads)n",opt_t);

    if (opt_q == 0)
        opt_q = 1000000;
    printf("q=%d (number of items to enqueue)n",opt_q);

    printf("l=%d -- lock is %sn",opt_l,opt_l ? "ticket" : "mutex");
    printf("m=%d -- queue item allocation is %sn",
        opt_m,opt_m ? "pooled" : "malloc/free");

    tvzero = tvgetf();

    lock_init(&task_mutex);
    lock_init(&print_mutex);

    // select queue item allocation strategy
    switch (opt_m) {
    case 1:
        cellnew = cellnew_pool;
        cellfree = cellfree_pool;
        break;

    default:
        cellnew = cellnew_std;
        cellfree = cellfree_std;
        break;
    }

    queue = xalloc(1,sizeof(tqueue));
    startqueue(queue);

    thread threads[opt_t];

    // get byte length of bit vectors
    bitvlen = btvlen(opt_q + 1);

    // allocate per-thread log buffers
    for (int i = 0; i < opt_t; i++) {
        tsk = &threads[i];
        if (! opt_p)
            tsk->log = xalloc(opt_q,sizeof(log));
        tsk->bitv = xalloc(bitvlen,sizeof(byte));
    }

    // allocate "work to do"
    t = xalloc(opt_q,sizeof(task));

    // add to master queue
    for (int i = 0; i < opt_q; i++) {
        t[i].number = i + 1;
        enqueue(queue, &t[i]);
    }

    // fire up the threads
    for (int i = 0; i < opt_t; i++) {
        tsk = &threads[i];
        tsk->xid = i + 1;
        tsk->queue = queue;
        pthread_create(&tsk->tid, null, work, tsk);
    }

    // wait for threads to complete
    for (int i = 0; i < opt_t; i++) {
        tsk = &threads[i];
        pthread_join(tsk->tid, null);
    }

    // wait for threads to complete
    for (int i = 0; i < opt_t; i++) {
        for (int j = i + 1; j < opt_t; j++)
            btvchk(&threads[i],&threads[j]);
    }

    printf("total: %.9fn",tvgetf());

    free(t);

    return 0;
}