C Thread pool - passing argument duplicate

Question

Having the source code below:

#define THREAD 32
#define QUEUE 300
#include <stdio.h>
#include <pthread.h>
#include <unistd.h>
#include <assert.h>
#include "threadpool.h"


struct fparam {
int no;
};

int tasks = 0, done = 0;
pthread_mutex_t lock;

int exit_me(){
    pthread_mutex_lock(&lock);
    tasks--;
    pthread_mutex_unlock(&lock);
    return 0;
}

void dummy_task(void *arg) {
    struct fparam *args = arg;
    pthread_mutex_lock(&lock);
    done++;
    pthread_mutex_unlock(&lock);
    printf("Thread INDEX: %d started.\n",args->no);
    exit_me();
}

int main()
{
    int t, result;
    threadpool_t *pool;
    struct fparam push_args;
    pthread_mutex_init(&lock, NULL);
    pool = threadpool_create(THREAD, QUEUE, 0);
    fprintf(stderr, "Pool started with %d threads and "
            "queue size of %d\n", THREAD, QUEUE);

    for (t = 0;t < 2000; t++){
        push_args.no = t;
        result = threadpool_add(pool, &dummy_task, (void *)&push_args, 0);
        if (result == 0){
            pthread_mutex_lock(&lock);
            tasks++;
            pthread_mutex_unlock(&lock);
        } else {
             printf("Something went wrong with thread: %d\n", t);
        }
        while(tasks  >= QUEUE); // do nothing until tasks running is less than max queue.
    }

    while(tasks >= 1);
    return 0;
}

I'm using https://github.com/mbrossard/threadpool pool implementation.

Everything looks alright, but while checking the t parameter passed to the dummy function, I can see duplicates:

Thread INDEX: 1998 started.
Thread INDEX: 1999 started.
Thread INDEX: 1999 started.
Thread INDEX: 1974 started.
Thread INDEX: 1979 started.
Thread INDEX: 1979 started.
Thread INDEX: 1978 started.
Thread INDEX: 1979 started.
Thread INDEX: 1979 started.

I suppose given the code that there is not any race condition as the fparam struct is declared inside the functions. Any ideas or suggestions?

Run the code through msan and tsan. You should spot the mistake pretty quickly. Remember: Sharing is the root of all evil. — Kerrek SB, Jan 23 '15 at 00:29

paxdiablo · Answer 1 · 2015-01-23T01:26:35.507

Yes, your push_args is suffering from a race condition. While each thread gets it's own copy of the parameter you pass in, the way you're doing it means every thread gets the same value (the pointer).

And the main thread is continuously modifying the data behind that pointer wwhile starting new threads but the threads themselves are starting up and using it.

Think in terms of the following sequence as an example:

main sets ID to 1 and creates thread A.
main sets ID to 2 and creates thread B.
thread A starts and reads ID of 2.
thread B starts and reads ID of 2.

Now both threads are using the ID of 2.

If you want to do it that way, you will probably need to wait until each thread has made a local copy of this ID value before modifying it in main.

Another way is to "automatically" give the thread a local copy by passing it as the parameter itself (not a pointer to it), something like:

result = threadpool_add(pool, &dummy_task, (void *)t, 0);

This ensures that each thread receives a localised copy of t as it was when the call was made. You'll just have to cast it from void* back to int.

If you cannot use a simple variable that's able to be cast to and from a pointer, and you don't want to wait until each thread has made a local copy before starting the next, you need to separate the items being passed.

One way to do this is to have an array of items (structures in your case) and pass them to the equivalent threads. For example, you could do something like:

static struct payload items[100];
for (int i = 0; i < 100; i++) {
    items[i].t = i;
    result = threadpool_add(pool, &dummy_task, (void *)(&items[i]), 0);
    // check result.
}

That's a bit more expensive on memory but it solves the problems of race conditions without having to serialise thread creation.

@LiviuValentin: an easy fix is to create an array of `struct fparam` and give a different one to each thread. Another more brutish one is to cast the thread number to `void *` and pass that to the thread function (which has to be changed to convert the `void *` back to an `int`). Or you can use synchronization primitives (a mutex, for example). On the whole, though, passing independent values to the thread is easier and better. — Jonathan Leffler, Jan 23 '15 at 00:35
Casting the number and pass it to the dummy function, actually worked out. But i have to use a `struct` because I'm going to pass more things through that `struct`. — Liviu Valentin, Jan 23 '15 at 00:48

score 1 · Accepted Answer · answered Jan 23 '15 at 01:17

I actually made it. Please check bellow and suggest if else or wrong.

int main()
{
    int t, result;
    threadpool_t *pool;
    struct fparam *push_args = NULL;

    pthread_mutex_init(&lock, NULL);
    pool = threadpool_create(THREAD, QUEUE, 0);
    fprintf(stderr, "Pool started with %d threads and "
                    "queue size of %d\n", THREAD, QUEUE);
    for (t = 0;t < 2000; t++){
            push_args = (struct fparam*)malloc(sizeof *push_args);
            push_args->no = t;
            result = threadpool_add(pool, &dummy_task, push_args, 0);
            if (result == 0){
                    pthread_mutex_lock(&lock);
                    tasks++;
                    pthread_mutex_unlock(&lock);
            } else {
                    printf("Something went wrong with thread: %d\n", t);
            }
            while(tasks  >= QUEUE); // do nothing until tasks running is less than max queue.
    }

    while(tasks >= 1);
    free(push_args);
    return 0;
}

Liviu, you're probably better off having an _array_ of structures rather than polluting the memory arena with lots of small allocations. — paxdiablo, Jan 23 '15 at 01:27
@paxdiablo, I've seen that you've edited your answer. Your solution sounds pretty much legit. I will further try both of them and i will come back with vote. Thanks a lot meanwhile. — Liviu Valentin, Jan 23 '15 at 01:31

C Thread pool - passing argument duplicate

2 Answers2