OSDev.org

Posted: **Fri Apr 04, 2008 11:04 am**

I am adding thread sleeping facility to my os. So i am having a default idle process started up by the scheduler which manages sleeping processes. It wakes up sleeping processes upon an event that was specified by the process. Currently I only have an irq event. I am using it for waiting for a key input in which the thread sleeps waiting for IRQ1 and when theres an IRQ1 then idle process which is constantly running starts up the sleeping process. Heres the code:

Code: Select all


struct sleep_event_struct	{
	unsigned long sleeping_pid;
	char thread_event_flags;	//Defines actions
	char thread_event_state;	//Current state
	char event_type;
	void *arg;
	struct sleep_event_struct *next,*prev;
};

volatile struct sleep_event_struct *head_sev,*temp_sev;

void _idle()
{
	struct sleep_event_struct *x,*y;
	while(1)	{
		for(x=head_sev;x;x=x->next)	{ //Not entered!!
			switch(x->event_type)	{
				case EVENT_IRQ:	if(got_irq(x->arg))	{
									wake_thread(x->sleeping_pid);
								}
								break;
			}
		}
		for(x=head_sev;x;x=x->next)	{
			if(x->thread_event_state==PESTATE_EXPIRED)	{
				if((x->thread_event_flags)&PEFLAG_EXPIRED_DELETE)	{
					(x->prev)->next=x->next;
					(x->next)->prev=x->prev;
					y=x->next;
					free(x);
					x=y;
				}
			}
		}
		asm("hlt");
	}
}

When a thread sleeps on an event then it is added in the linked list with the head 'head_sev'.
Uptill this part everything goes fine. But when an IRQ goes then the process is not woken up.
When i tried to debug the code i saw that the first for loop never got executed because the value of x was already null since head_sev was read as null (i guess).
head_sev is properly updated by the main thread when an event sleeps (i am only adding one sleep_event_struct currently. So either head_sev is null or it has the first entry). So now idle thread should read head_sev and enter the for loop to check whether the specified event for head_sev has taken place...

But head_sev is not updated in the idle thread inspite of a volatile declaration.

So what else do i do?

My compilation options simply include gcc -c -O2

Posted: **Fri Apr 04, 2008 11:11 am**

Hi,

Are those structs declared volatile in the main thread as well as this one? it's possible they're not actually being written (as opposed to not being read).

Cheers,

James

Posted: **Fri Apr 04, 2008 11:16 am**

The main thread accesses those structs in the same file and hence uses the same declaration... That is the main thread calls the functions like create_sleep_on_event which are defined just below the idle thread. So for them too its been declared as volatile...

Posted: **Fri Apr 04, 2008 12:32 pm**

The pointers are not declared volatile, they are declared to be pointing to a struct that is volatile which is probably not what you wanted. Instead try (not verified here):

Code: Select all

struct sleep_event_struct * volatile head_sev,* volatile temp_sev;

Posted: **Fri Apr 04, 2008 10:20 pm**

Just curious, but this seems like a bad design from the start, and rather than fixing this problem, then having others, why does your idle thread handle waking up threads? I mean, if a keyboard interrupt happens, and you have a very high priority task running, or a few of them, how long will it take for your idle thread to get time to run? Is it moved up to high priority to process messages first? I'm just thinking the response time of using an idle thread, unless it's priority is increased is going to be very sluggish.

That said, why are you running 2 for loops that do the same thing? Wouldn't it be easier to do something like this:

Code: Select all

void _idle() 
{ 
   struct sleep_event_struct *x,*y; 
   while(1)
   { 
      for(x=head_sev;x;x=x->next)
      { //Not entered!! 
         if(x->thread_event_state==PESTATE_EXPIRED)
         { 
            if((x->thread_event_flags)&PEFLAG_EXPIRED_DELETE)
            {
               (x->prev)->next=x->next; 
               (x->next)->prev=x->prev; 
               y=x->next; 
               free(x); 
               x=y; 
            } 
         }
         else
         {
            switch(x->event_type)
            { 
               case EVENT_IRQ:   if(got_irq(x->arg))
               { 
                  wake_thread(x->sleeping_pid); 
               } 
               break; 
            }
         }
      } 
      asm("hlt"); 
   } 
}

Posted: **Sat Apr 05, 2008 10:48 am**

Awesome!! Thanks a lot skyking.. It worked. Its just that the only source of my C basics is my OS experience. So i never knew the difference between volatile pointers and structures of the pointers declared as volatile... Now life seems simpler...

Ready4Dis, I know completely and understand that this is not a right design.. But its just a simple workaround over the tight useless looping of waiting for keystrokes from the keyboard driver... What i could do to improve this could be simply to schedule the idle thread as soon as an irq is fired for user response.

Yes, the for loops can be optimized the way you say.. But i just designed them in order to have one event check and one cleanup loop...

I got this problem as soon i started implementing sleeping threads.. So nothings really optimized...

OSDev.org

Volatile for what?

Volatile for what?