3
\$\begingroup\$

I have to make a program which creates a number of threads that are waiting at the beginning, then the main process creates a doubly linked list where random numbers are put and then a signal tells the threads that a number has been put in the list, then the threads should "process" these numbers (is simply calculate the sum from 0 to the given number), graphically it looks like this:

enter image description here

And here is the code, I wonder if I'm doing what I'm doing fine...

Note: I'm working with 4 threads and the sleep(1) was to see the output...

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <signal.h>
#include <time.h>
#include <semaphore.h>

#include "double_linked_list.h"

void *sum(void *ptr);

sem_t semaphore;
pthread_mutex_t mutex;

int main() {
    sem_init(&semaphore, 0, 0);

    int threads = 4;
    int number, i;

    pthread_t *threads_tab;
    int *thread_args;

    threads_tab = malloc(threads * sizeof(pthread_t));
    thread_args = malloc(threads * sizeof(int));

    srand(time(NULL));

    head = NULL;
    tail = NULL;

    for (i = 0; i < threads; i++) {
        thread_args[i] = i;
        pthread_create(&threads_tab[i], NULL, sum, (void*) &thread_args[i]);
    }

    while (1) {
        number = rand() % 100;

        printf("Insertando: %d\n", number);
        InsertAtHead(number);

        sem_post(&semaphore);

        sleep(1);
    }

    return 0;
}

void *sum(void *ptr) {
    int i, acumulator, number;

    while (1) {
        sem_wait(&semaphore);

        acumulator = 0;

        pthread_mutex_lock(&mutex);
        number = tail->data;
        PopTail();
        pthread_mutex_unlock(&mutex);

        for (i = 0; i < number + 1; i++)
            acumulator += i;

        printf("The number is: %d and the result is: %d from thread: %d\n", number, acumulator, *(int *)ptr);
    }
}

Apparently it's working well, it doesn't generate any errors and the outputs are the expected ones, but I would like to hear more opinions, because I am new in this, perhaps I'm making some mistakes without realizing...

\$\endgroup\$
1
\$\begingroup\$

It's hard to tell without seeing the code for InsertHead and PopTail, but I would not be surprised if you've got a race condition between the main thread's InsertHead and the worker thread's PopTail. The workers take mutex in order to prevent data races among themselves; but the main thread blithely pushes new data onto the list without taking mutex, which seems like it would cause a race in the case that the list had only one element.


Stylistically, I'd replace

int threads = 4;
int number, i;

pthread_t *threads_tab;
int *thread_args;

threads_tab = malloc(threads * sizeof(pthread_t));
thread_args = malloc(threads * sizeof(int));

with

pthread_t threads_tab[4];
int thread_args[4];

to save those heap allocations (which, by the way, you never free).

The variable i should be declared within the for loop:

for (int i = 0; i < threads; i++) {

and the variable number should be declared where it's used:

int number = rand() % 100;

thread_args[i] = i;
pthread_create(&threads_tab[i], NULL, sum, (void*) &thread_args[i]);
[...]
printf(..., *(int *)ptr);

This is fine (and 100% correct, because it avoids implementation-defined casts!), but you should know that in practice most codebases will do simply

pthread_create(&threads_tab[i], NULL, sum, (void*) i);
[...]
printf(..., (int)ptr);

The latter is technically implementation-defined, but on common platforms it works, and it works without any memory loads or stores, which is nice, so that's why people end up doing it.


You fail to check the return value of pthread_create, which is a pretty big deal in real code; it's easy for a process to run out of threads. See man pthread_create.


In English, accumulator has two cs.

\$\endgroup\$
  • \$\begingroup\$ Although, seeing your answer more carefully, I don't agree with the for (int i = 0; i <threads; i ++) and int number = rand ()% 100. It's not necessary to do it that way, and BTW it's only allowed in the standard c99 and above \$\endgroup\$ – OiciTrap May 21 '16 at 1:43
  • \$\begingroup\$ Defining your variables at their point-of-use isn't necessary, but then neither is naming them i instead of, say, marypoppins. It's simply better to name the loop variable i and define it in the looping statement. \$\endgroup\$ – Quuxplusone May 22 '16 at 17:58
  • \$\begingroup\$ "BTW it's only allowed in the standard c99 and above" — True. However, note that 1999 was 17 years ago. There do still exist codebases which need to conform to the 1989 standard, but that's a very small niche occupied mainly by very long-bearded people. If you personally were in that niche, trust me, you wouldn't be asking this type of question on CodeReview. \$\endgroup\$ – Quuxplusone May 22 '16 at 18:02
  • \$\begingroup\$ At least in my case, to be able to compile for (int i = 0; i <threads; i ++) I have to put the flag -std=c99, so I prefer to do the definition of i firt and then initialize it in the loop for \$\endgroup\$ – OiciTrap May 23 '16 at 1:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.