C/Multithreading /Segmentation fault / (May be) Issue with queue for the threads

Question

I am trying to create thread library.For this I am trying to implement queue to store the pending threads to be executed.

   #include <ucontext.h>
   #include <stdio.h>
   #include <stdlib.h>

   typedef struct {
       ucontext_t context;
   }MyThread;

   #define MAX 20
   MyThread queue[MAX];
   int rear=0,front=0;

   void addToQueue(MyThread t)
   {
       if(rear==MAX)
       {
           printf("Queue is full!");
           return;
       }        
       queue[front]=t;
       front+=1;
   }

   MyThread* removeFromQueue()
   {       
       if(front==rear)
       return NULL;        
       rear=rear+1;
       return &(queue[rear-1]);       
   }

   MyThread umain;

   void MyThreadInit (void(*start_funct)(void *), void *args)
   {
    getcontext(&(umain.context));
    char p[64000];
       umain.context.uc_stack.ss_sp =(char *)p;
       umain.context.uc_stack.ss_size = sizeof(p);
       umain.context.uc_link =NULL;
       makecontext(&umain.context,(void(*)(void))start_funct,1,args);
       setcontext(&(umain.context));

   }

    MyThread MyThreadCreate (void(*start_funct)(void *), void *arg)
   {
         MyThread newthread;
       char args[10000];
        getcontext(&(newthread.context));
        newthread.context.uc_stack.ss_sp =(char *)args;
        newthread.context.uc_stack.ss_size = sizeof(args);
        newthread.context.uc_link =NULL;
        makecontext(&newthread.context,(void(*)(void))start_funct,1,arg);
        addToQueue(newthread);

        return newthread;
    }         
    void MyThreadYield(void)
    {
        MyThread* a=removeFromQueue();
        MyThread save;
        if(a != NULL)
        {
         printf("Before yielding the context \n");

         getcontext(&(save.context));
         addToQueue(save);
         //swapcontext(&umain.context,&(a->context));
         setcontext(a);    
         printf("After the swapping the context \n");
        }
        else
        { printf("NULL!!! \n");
        }
    }

    void func1(void *arg)
    {
     printf("func1started \n");        
     MyThreadYield();
    }

    void func2(void *arg)
    {
     printf("func2started \n");
     MyThreadYield();         
    }
    void func12(void *arg)
    {
     printf("func12started \n");
     MyThreadCreate(func1,arg);
     MyThreadCreate(func2,arg);
     MyThreadYield();

    }

    int main(void)
    {
        int i=0;
        printf("inside the main function \n");
        MyThreadInit(func12,&i);

        return 0;
    }

     Output :
     inside the main function
     func12started
     Before yielding the context
     func1started
     Before yielding the context
     func2started
     Before yielding the context
     func1started
     Before yielding the context
     Segmentation fault

The reason I mentioned the queue because i tried experimenting by removing below code from 'MyThreadYield' function and it workes fine but doesnt do the intended functionality.
getcontext(&(save.context)); addToQueue(save);

Answer 1

For one, your queue implementation is not thread-safe at this point. Your question strongly suggests that this code will be used in a multi-threaded environment. Having a non thread-safe queue will give you wrong results, and weird things can happen (like removeFromQueue() returning the same thing to two different threads, or addToQueue() inserting two items in the same position).

Aside from that, your queue implementation would never work. You are not using front and rear correctly. Look carefully at the insert function:

void addToQueue(MyThread t)
{
    if (rear==MAX)
    {
        printf("Queue is full!");
        return;
    }        
    queue[front]=t;
    front+=1;
}

You check if rear is MAX, yet, you write into queue[front] and increment front. What if I just keep adding items to the queue, eventually reaching the buffer's limit? rear will always be 0, front will grow indefinitely, and your function will write beyond the limits of queue. That's probably the cause of your segmentation fault errors.

I think you wanted to check for front instead:

void addToQueue(MyThread t)
{
    if (front == MAX)
    {
        printf("Queue is full!");
        return;
    }        
    queue[front]=t;
    front+=1;
}

The code for removeFromQueue() looks superficially ok, as long as queue is a global array (since you're returning a pointer, and can't return pointers to local variables). However, the single, most important fact you have to take out of this answer, is that your queue implementation will not scale in the long-term. An array-based queue is a terrible choice. What do you do when you run out of space in the array? What if I insert MAX elements, then remove 2 or 3, and try to insert more? Your code will say the queue is full, because it only ever allows you to insert MAX elements in total. You could shift every element in the queue to the left when an element is removed, but that's crazy, and extremely inefficient. Or you could increment front modulo MAX, allowing rear to be ahead of front, as long as you know that no more than MAX elements can be inserted. That would be better, but it would break the logic in removeFromQueue(), since the pointers returned earlier will possibly point to a different thread struct as you manipulate the queue - total disaster. Definitely not what you want.

A much, much better approach would be to implement this with a linked list where you keep a pointer to the head and a pointer to the tail. Have a look at http://en.wikipedia.org/wiki/Queue_(abstract_data_type)#Queue_implementation