14

I'm trying to use threads (for the first time!) in a GCC C application which works fine in non-threaded mode. When I run it some threads give results which are all zero instead of the required answers (which I know for checking purposes), but the threads giving zeroes are not the same each time I run it. The ones which give non-zero answers are correct, so the code appears to run ok as such. I wonder if anyone can point out areas where I might have something which is not thread-safe.

My own thoughts are it may be due to how I collect results or maybe memory allocation - I use malloc and free but elsewhere in StackOverflow I see that GCC malloc is considered thread-safe if linked with -lpthread (which I am doing). Nothing uses global/static variables - everything is passed as function arguments.

In order to pass results back to main, my threaded routine uses an array of structures. Each thread writes to a distinct element of this array, so they are not trying to write to the same memory. Maybe I need to use some form of locking when writing results even though they don't go to the same element of the structure array?

I followed the recipe for threaded code here: https://computing.llnl.gov/tutorials/pthreads/#Abstract

I attach (simplified) code extracts in case this gives any clues (I may have omitted/modified something incorrectly but I am not asking for anyone to spot bugs, just the general methodology).

typedef struct p_struct { /* used for communicating results back to main */
    int given[CELLS];
    int type;
    int status;
    /*... etc */
} puzstru;

typedef struct params_struct { /* used for calling generate function using threads */
    long seed;
    char *text;
    puzzle *puzzp;
    bool unique;
    int required;
} paramstru;
/* ========================================================================================== */
void *myfunc(void *spv) /* calling routine for use by threads */
{
    paramstru *sp=(paramstru *)spv;
    generate(sp->seed, sp->text, sp->puzzp, sp->unique, sp->required);
    pthread_exit((void*) spv);
}
/* ========================================================================================== */
int generate(long seed, char *text, puzstru *puzzp, bool unique, int required)
{
/* working code , also uses malloc and free,
    puts results in the element of a structure array pointed to by "puzzp", 
    which is different for each thread
    (see calling routine below :        params->puzzp=puz+thr; )
    extract as follows: */
            puzzp->given[ix]=calcgiven[ix];
            puzzp->type=1; 
            puzzp->status=1;
            /* ... etc */
}
/* ========================================================================================== */


int main(int argc, char* argv[])
{
    pthread_t thread[NUM_THREADS];
    pthread_attr_t threadattr;
    int thr,threadretcode;
    void *threadstatus;
    paramstru params[1];

    /* ....... ETC */

/* set up params structure for function calling parameters */
    params->text=mytext;
    params->unique=TRUE;
    params->required=1;

    /* Initialize and set thread detached attribute */
    pthread_attr_init(&threadattr);
    pthread_attr_setdetachstate(&threadattr, PTHREAD_CREATE_JOINABLE);

    for(thr=0; thr<NUM_THREADS; thr++)
    {
        printf("Main: creating thread %d\n", thr);
        params->seed=ran_arr_next(startingseeds); 
        params->puzzp=puz+thr;
        threadretcode = pthread_create(&thread[thr], &threadattr, myfunc, (void *)params); 
        if (threadretcode)
        {
            printf("ERROR; return code from pthread_create() is %d\n", threadretcode);
            exit(-1);
        }
    }

    /* Free thread attribute and wait for the other threads */
    pthread_attr_destroy(&threadattr);
    for(thr=0; thr<NUM_THREADS; thr++)
    {
        threadretcode = pthread_join(thread[thr], &threadstatus);
        if (threadretcode)
        {
            printf("ERROR; return code from pthread_join() is %d\n", threadretcode);
            exit(-1);
        }
        printf("Main: completed join with thread %d having a status of %ld\n",thr,(long)threadstatus);
    }

/* non-threaded code, print results etc ............. */

    free(startingseeds);
    free(puz);
    printf("Main: program completed. Exiting.\n");
    pthread_exit(NULL);
}

For the benefit of others reading this - all the answers were correct, and the answer to the question in the heading is YES, threads can write safely to different elements of the same array of structures, my problem was in the calling routine - the following is the amended code snippet (now works fine):

    paramstru params[NUM_THREADS];

    for(thr=0; thr<NUM_THREADS; thr++)
    {
        printf("Main: creating thread %d\n", thr);
    /* set up params structure for function calling parameters */
        params[thr].text=mytext;
        params[thr].unique=TRUE;
        params[thr].required=1;
        params[thr].seed=ran_arr_next(startingseeds); 
        params[thr].puzzp=puz+thr;
        threadretcode = pthread_create(&thread[thr], &threadattr, myfunc, (void *)&params[thr]); 
        if (threadretcode)
        {
            printf("ERROR; return code from pthread_create() is %d\n", threadretcode);
            exit(-1);
        }
    }
1
  • Hmm. Where is variable puz declared? Is it of type puzzle *? How is ix calculated? I am guessing your problem is in the block you marked as "working code." ;) Mar 15, 2011 at 18:45

3 Answers 3

9

To answer your question, it is perfectly fine to write to different elements of the same array from different threads without locking. There will only ever be a data race if two threads write to the same byte without synchronizing (e.g., locking).

As other answers point out, the reason your code as-written breaks is because you pass a pointer to the same params object to each of your threads, an then you modify that object. You probably want to create a new param for each thread.

5
  • 5
    While it is safe, if not done carefully, it can lead to bad performance. If several threads keep accessing elements of the array on the same cache line, you'll have heavy cacheline-bouncing, which is expensive.
    – ninjalj
    Mar 15, 2011 at 19:26
  • Thanks for the info regarding different bytes. I was looking in the wrong place for my problem.
    – RussellG
    Mar 15, 2011 at 19:55
  • 1
    Are you sure it's guaranteed to be safe? What about machines which cannot perform smaller-than-word writes, for which writing a byte is a read-modify-write operation? I agree such machines are pathological crap that should not be used for any real-world purposes, but strictly speaking I believe they need to be considered if you're claiming full portability... Mar 15, 2011 at 20:17
  • @R: you're right, the smallest atomic memory unit does depend on the architecture.
    – Karmastan
    Mar 16, 2011 at 1:37
  • @ninjalj And how can you be careful about this?
    – IceFire
    Jan 26, 2018 at 16:39
3
paramstru params[1];

The code is passing the same structure to all threads. Just the thread initialization loop is overwriting the data that a thread should work on:

for(thr=0; thr<NUM_THREADS; thr++)
    {
        printf("Main: creating thread %d\n", thr);
        params->seed=ran_arr_next(startingseeds); /* OVERWRITE  */
        params->puzzp=puz+thr; /* OVERWRITE  */

The reason the non-threaded code works is because each call to myfunc() terminates before the params structure is changed.

2
  • 1
    Thanks, I guess there was a race between a thread reading the params and the next iteration of the setup loop.
    – RussellG
    Mar 15, 2011 at 19:57
  • You could fix this race with a barrier (mildly slow) or by using a separate struct for each thread (mildly wasteful of memory). Mar 15, 2011 at 20:15
1

You only created one copy of your parameter structure and are overwriting it and passing the same address to each thread. Dont' you want paramstru params[NUM_THREADS];?

0

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.