Issue with os_eventq.c

[will sanfilippo <wi...@runtime.io>]

Hello:

It turns out the real issue I was having was a stack overflow but in any event it does look like it has uncovered an issue with os_eventq.c. This is the issue; note that tasks are numbered based on their priority (ie. task1 is priority 1, task2 is priority 2, etc).

* Task3 calls os_eventq_get() when there are no events on the event queue. evq->evq_task gets set to the current task.
* Task3 goes to sleep.
* Task 2 wake up and puts an event on this eventq. os_eventq_put() sees that evq->evq_task is set and calls os_sched_wakeup() which removes Task3 from sleep list and puts it on the run list. Task2 is still running.
* Before Task3 can wake up, Task1 preempts Task2 and puts an event on the event queue. Since evq->evq_task is set, os_sched_wakeup() is called but this time the task is already on the run list. This causes an error since os_sched_wakeup() presumes that the task is on the sleep list.

I am not sure the proper fix is to modify os_sched_wakeup() so that it checks the state of the task and only if the state is SLEEP does it remove the task from the sleep list. I think we lose something valuable by allowing os_sched_wakeup() to be called when the task is on the run list. I do agree that “randomly failing” is undesirable so this is what I would do:

* Put an assert() in os_sched_wakeup() and make sure the state is SLEEP.
* Modify the code such that we set evq->evq_task to NULL in os_eventq_put2(). This will prevent us from calling os_sched_wakeup() on future calls to os_eventq_put2().

We could also add a check in os_eventq_put2() so that we only call os_sched_wakeup() if the state is SLEEP.

Thoughts?

Will

Re: Issue with os_eventq.c

[will sanfilippo <wi...@runtime.io>]

Yeah,  I am fine with that. Not the most useful functionality if you ask me. Unless others have any strong objections I will remove those functions from the code.
 

> On Dec 4, 2015, at 2:59 PM, marko kiiskila <ma...@runtime.io> wrote:
> 
> Hi,
> 
> I don’t think we need/want os_sem_delete(), os_mutex_delete() functions.
> 
> I believe the only time you’d want to remove one of these is when you are
> also going to free the underlying memory it is located in. When you’re going to
> do that, then there has to be some other mechanism already in place which
> makes sure that this memory is not going to be touched by other tasks (e.g.
> ref counting).
> And if you have ref counting, you already know when memory for the lock
> can be freed.
> 
>> On Dec 3, 2015, at 11:37 PM, will sanfilippo <wi...@runtime.io> wrote:
>> 
>> I just committed the fixes to the OS. If someone wants to review them that would be great.
>> 
>> While looking at the changes, I noticed something about the os_sem_delete() and os_mutex_delete() code. For the semaphore code, if a task is waiting on a semaphore and that semaphore gets deleted, an error of OS_TIMEOUT will be returned to the task waiting on the semaphore. Not sure if that is the proper behavior. Actually, not sure what the proper behavior would be. Should we allow a semaphore to be deleted if tasks are waiting on it? If so, what should the returned error be? OS_OK? OS_TIMEOUT? So other error code? (OS_ERR_DELETED, or some such)?
>> 
>> Regarding mutexes, if a task is waiting on a mutex and the mutex gets deleted, the mutex is actually de-referenced (in os_mutex_pend). This is not good. Should we allow a mutex to be deleted if tasks are waiting on it? If yes, we will need to modify the code so that we dont de-reference the mutex.
>> 
>> Thanks!
>> 
>> PS If we allow semaphores/mutexes to be deleted when tasks are waiting on them, what should we return for OS error code? I would say OS_OK; if we return anything other than OS_OK or OS_TIMEOUT it gets a bit more complicated.
>> 
>> 
>>> On Dec 2, 2015, at 1:54 PM, marko kiiskila <ma...@runtime.io> wrote:
>>> 
>>> Hi Will,
>>> 
>>> e.g. separate flags for different types of locks would accomplish that.
>>> I’m thinking about the case of debugging a deadlocked system, and I want
>>> quickly figure out what tasks are waiting on, and the state of those locks.
>>> 
>>> Of course, I can dig through stacks of those tasks, and eventually figure
>>> that out. But it would be nicer to have an easier way.
>>> 
>>>> On Dec 2, 2015, at 1:32 PM, will sanfilippo <wi...@runtime.io> wrote:
>>>> 
>>>> Well, the code certainly wont know what is behind it (other than the list element). From a debugging perspective you might have to hunt around for it… not sure what you consider easy :-)
>>>> 
>>>>> On Dec 2, 2015, at 1:18 PM, marko kiiskila <ma...@runtime.io> wrote:
>>>>> 
>>>>> Sounds good, as long as it is easy to figure out what kind of structure is behind void *t_obj.
>>>>> 
>>>>>> On Dec 2, 2015, at 1:05 PM, Sterling Hughes <st...@apache.org> wrote:
>>>>>> 
>>>>>>> 
>>>>>>> Not quite sure the best fix but here is a proposal to consider. Change struct os_mutex *t_mutex to void *t_obj in struct os_task and modify all the code to use the common object. Not sure whether to have separate os task flags for each object or a common task flag when waiting on an object. This would depend, I guess, on whether we want to force some commonality among objects that we are waiting for. I dont see why that would be an issue; we can place the object list pointers at the start of the object. That is what I would do.
>>>>>>> 
>>>>>> 
>>>>>> +1
>>>>> 
>>>> 
>>> 
>> 
> 

Re: Issue with os_eventq.c

[marko kiiskila <ma...@runtime.io>]

Hi,

I don’t think we need/want os_sem_delete(), os_mutex_delete() functions.

I believe the only time you’d want to remove one of these is when you are
also going to free the underlying memory it is located in. When you’re going to
do that, then there has to be some other mechanism already in place which
makes sure that this memory is not going to be touched by other tasks (e.g.
ref counting).
And if you have ref counting, you already know when memory for the lock
can be freed.

> On Dec 3, 2015, at 11:37 PM, will sanfilippo <wi...@runtime.io> wrote:
> 
> I just committed the fixes to the OS. If someone wants to review them that would be great.
> 
> While looking at the changes, I noticed something about the os_sem_delete() and os_mutex_delete() code. For the semaphore code, if a task is waiting on a semaphore and that semaphore gets deleted, an error of OS_TIMEOUT will be returned to the task waiting on the semaphore. Not sure if that is the proper behavior. Actually, not sure what the proper behavior would be. Should we allow a semaphore to be deleted if tasks are waiting on it? If so, what should the returned error be? OS_OK? OS_TIMEOUT? So other error code? (OS_ERR_DELETED, or some such)?
> 
> Regarding mutexes, if a task is waiting on a mutex and the mutex gets deleted, the mutex is actually de-referenced (in os_mutex_pend). This is not good. Should we allow a mutex to be deleted if tasks are waiting on it? If yes, we will need to modify the code so that we dont de-reference the mutex.
> 
> Thanks!
> 
> PS If we allow semaphores/mutexes to be deleted when tasks are waiting on them, what should we return for OS error code? I would say OS_OK; if we return anything other than OS_OK or OS_TIMEOUT it gets a bit more complicated.
> 
> 
>> On Dec 2, 2015, at 1:54 PM, marko kiiskila <ma...@runtime.io> wrote:
>> 
>> Hi Will,
>> 
>> e.g. separate flags for different types of locks would accomplish that.
>> I’m thinking about the case of debugging a deadlocked system, and I want
>> quickly figure out what tasks are waiting on, and the state of those locks.
>> 
>> Of course, I can dig through stacks of those tasks, and eventually figure
>> that out. But it would be nicer to have an easier way.
>> 
>>> On Dec 2, 2015, at 1:32 PM, will sanfilippo <wi...@runtime.io> wrote:
>>> 
>>> Well, the code certainly wont know what is behind it (other than the list element). From a debugging perspective you might have to hunt around for it… not sure what you consider easy :-)
>>> 
>>>> On Dec 2, 2015, at 1:18 PM, marko kiiskila <ma...@runtime.io> wrote:
>>>> 
>>>> Sounds good, as long as it is easy to figure out what kind of structure is behind void *t_obj.
>>>> 
>>>>> On Dec 2, 2015, at 1:05 PM, Sterling Hughes <st...@apache.org> wrote:
>>>>> 
>>>>>> 
>>>>>> Not quite sure the best fix but here is a proposal to consider. Change struct os_mutex *t_mutex to void *t_obj in struct os_task and modify all the code to use the common object. Not sure whether to have separate os task flags for each object or a common task flag when waiting on an object. This would depend, I guess, on whether we want to force some commonality among objects that we are waiting for. I dont see why that would be an issue; we can place the object list pointers at the start of the object. That is what I would do.
>>>>>> 
>>>>> 
>>>>> +1
>>>> 
>>> 
>> 
> 

Re: Issue with os_eventq.c

[will sanfilippo <wi...@runtime.io>]

I just committed the fixes to the OS. If someone wants to review them that would be great.

While looking at the changes, I noticed something about the os_sem_delete() and os_mutex_delete() code. For the semaphore code, if a task is waiting on a semaphore and that semaphore gets deleted, an error of OS_TIMEOUT will be returned to the task waiting on the semaphore. Not sure if that is the proper behavior. Actually, not sure what the proper behavior would be. Should we allow a semaphore to be deleted if tasks are waiting on it? If so, what should the returned error be? OS_OK? OS_TIMEOUT? So other error code? (OS_ERR_DELETED, or some such)?

Regarding mutexes, if a task is waiting on a mutex and the mutex gets deleted, the mutex is actually de-referenced (in os_mutex_pend). This is not good. Should we allow a mutex to be deleted if tasks are waiting on it? If yes, we will need to modify the code so that we dont de-reference the mutex.

Thanks!

PS If we allow semaphores/mutexes to be deleted when tasks are waiting on them, what should we return for OS error code? I would say OS_OK; if we return anything other than OS_OK or OS_TIMEOUT it gets a bit more complicated.


> On Dec 2, 2015, at 1:54 PM, marko kiiskila <ma...@runtime.io> wrote:
> 
> Hi Will,
> 
> e.g. separate flags for different types of locks would accomplish that.
> I’m thinking about the case of debugging a deadlocked system, and I want
> quickly figure out what tasks are waiting on, and the state of those locks.
> 
> Of course, I can dig through stacks of those tasks, and eventually figure
> that out. But it would be nicer to have an easier way.
> 
>> On Dec 2, 2015, at 1:32 PM, will sanfilippo <wi...@runtime.io> wrote:
>> 
>> Well, the code certainly wont know what is behind it (other than the list element). From a debugging perspective you might have to hunt around for it… not sure what you consider easy :-)
>> 
>>> On Dec 2, 2015, at 1:18 PM, marko kiiskila <ma...@runtime.io> wrote:
>>> 
>>> Sounds good, as long as it is easy to figure out what kind of structure is behind void *t_obj.
>>> 
>>>> On Dec 2, 2015, at 1:05 PM, Sterling Hughes <st...@apache.org> wrote:
>>>> 
>>>>> 
>>>>> Not quite sure the best fix but here is a proposal to consider. Change struct os_mutex *t_mutex to void *t_obj in struct os_task and modify all the code to use the common object. Not sure whether to have separate os task flags for each object or a common task flag when waiting on an object. This would depend, I guess, on whether we want to force some commonality among objects that we are waiting for. I dont see why that would be an issue; we can place the object list pointers at the start of the object. That is what I would do.
>>>>> 
>>>> 
>>>> +1
>>> 
>> 
> 

Re: Issue with os_eventq.c

[marko kiiskila <ma...@runtime.io>]

Hi Will,

e.g. separate flags for different types of locks would accomplish that.
I’m thinking about the case of debugging a deadlocked system, and I want
quickly figure out what tasks are waiting on, and the state of those locks.

Of course, I can dig through stacks of those tasks, and eventually figure
that out. But it would be nicer to have an easier way.

> On Dec 2, 2015, at 1:32 PM, will sanfilippo <wi...@runtime.io> wrote:
> 
> Well, the code certainly wont know what is behind it (other than the list element). From a debugging perspective you might have to hunt around for it… not sure what you consider easy :-)
> 
>> On Dec 2, 2015, at 1:18 PM, marko kiiskila <ma...@runtime.io> wrote:
>> 
>> Sounds good, as long as it is easy to figure out what kind of structure is behind void *t_obj.
>> 
>>> On Dec 2, 2015, at 1:05 PM, Sterling Hughes <st...@apache.org> wrote:
>>> 
>>>> 
>>>> Not quite sure the best fix but here is a proposal to consider. Change struct os_mutex *t_mutex to void *t_obj in struct os_task and modify all the code to use the common object. Not sure whether to have separate os task flags for each object or a common task flag when waiting on an object. This would depend, I guess, on whether we want to force some commonality among objects that we are waiting for. I dont see why that would be an issue; we can place the object list pointers at the start of the object. That is what I would do.
>>>> 
>>> 
>>> +1
>> 
> 

Re: Issue with os_eventq.c

[will sanfilippo <wi...@runtime.io>]

Well, the code certainly wont know what is behind it (other than the list element). From a debugging perspective you might have to hunt around for it… not sure what you consider easy :-)

> On Dec 2, 2015, at 1:18 PM, marko kiiskila <ma...@runtime.io> wrote:
> 
> Sounds good, as long as it is easy to figure out what kind of structure is behind void *t_obj.
> 
>> On Dec 2, 2015, at 1:05 PM, Sterling Hughes <st...@apache.org> wrote:
>> 
>>> 
>>> Not quite sure the best fix but here is a proposal to consider. Change struct os_mutex *t_mutex to void *t_obj in struct os_task and modify all the code to use the common object. Not sure whether to have separate os task flags for each object or a common task flag when waiting on an object. This would depend, I guess, on whether we want to force some commonality among objects that we are waiting for. I dont see why that would be an issue; we can place the object list pointers at the start of the object. That is what I would do.
>>> 
>> 
>> +1
> 

Re: Issue with os_eventq.c

[marko kiiskila <ma...@runtime.io>]

Sounds good, as long as it is easy to figure out what kind of structure is behind void *t_obj.

> On Dec 2, 2015, at 1:05 PM, Sterling Hughes <st...@apache.org> wrote:
> 
>> 
>> Not quite sure the best fix but here is a proposal to consider. Change struct os_mutex *t_mutex to void *t_obj in struct os_task and modify all the code to use the common object. Not sure whether to have separate os task flags for each object or a common task flag when waiting on an object. This would depend, I guess, on whether we want to force some commonality among objects that we are waiting for. I dont see why that would be an issue; we can place the object list pointers at the start of the object. That is what I would do.
>> 
> 
> +1

Re: Issue with os_eventq.c

[Sterling Hughes <st...@apache.org>]

>
> Not quite sure the best fix but here is a proposal to consider. Change struct os_mutex *t_mutex to void *t_obj in struct os_task and modify all the code to use the common object. Not sure whether to have separate os task flags for each object or a common task flag when waiting on an object. This would depend, I guess, on whether we want to force some commonality among objects that we are waiting for. I dont see why that would be an issue; we can place the object list pointers at the start of the object. That is what I would do.
>

+1

Re: Issue with os_eventq.c

[will sanfilippo <wi...@runtime.io>]

All:

As some of you noticed, I decided to commit the code with the assert since there was a 2-2 tie :-) And, as some of you noticed, the regression tests failed. My bad for not running them first!

Interestingly enough, the assert uncovered a bug in the OS. If a task is waiting for a semaphore with a timeout and the timeout occurs, the task is moved from the sleep list to the run list but not removed from the list of tasks waiting for the semaphore (the task needs to be removed from the semaphore object). When the semaphore was released, the task was scheduled for wakeup since the os sem code thought the task was still waiting on the semaphore.

Not quite sure the best fix but here is a proposal to consider. Change struct os_mutex *t_mutex to void *t_obj in struct os_task and modify all the code to use the common object. Not sure whether to have separate os task flags for each object or a common task flag when waiting on an object. This would depend, I guess, on whether we want to force some commonality among objects that we are waiting for. I dont see why that would be an issue; we can place the object list pointers at the start of the object. That is what I would do.

Thanks,
Will

> On Dec 1, 2015, at 11:01 AM, marko kiiskila <ma...@runtime.io> wrote:
> 
> I agree with Sterling on this one; I don’t think there is much to be gained by
> asserting on task state in os_sched_wakeup(). I’d rather have it return
> if the task is already running.
> 
> However, I also would make the change you suggest to os_event_put().
> 
>> On Dec 1, 2015, at 10:24 AM, Sterling Hughes <st...@gmail.com> wrote:
>> 
>> Yeah, but whether or not we get a check - how should the function operate.  I'm not sure there was anything wrong in OS eventq for calling this twice, it's just more efficient to not do so.
>> 
>> I think it's perfectly valid to wake up a task twice, so it doesn't make sense to me to have the code crash when we do that. 
>> 
>> Sterling 
>> 
>> 
>> 
>>> On Dec 1, 2015, at 10:02 AM, will sanfilippo <wi...@runtime.io> wrote:
>>> 
>>> While there may not be anything “wrong” with adding a check in os_sched_wakeup(), I think we lose something by adding it. If the OS was written in a way that everywhere os_sched_wakeup() was called (or in many places) we checked to see if the task was on the sleep queue, I would agree with you. However, there are no places where this is required (I dont think the code in os_eventq() qualifies; two wrongs dont make a right). In fact, if the task was not on the sleep queue, it would indicate something wrong with the code or a memory overwrite.
>>> 
>>> I think having an API like os_sched_wakeup() which checks to see if the task is on the sleep queue and asserts if not is a good check on the logic of the rest of the code and will help us find bugs.
>>> 
>>> Will
>>> 
>>> 
>>>> On Dec 1, 2015, at 9:42 AM, Sterling Hughes <st...@apache.org> wrote:
>>>> 
>>>> On Mon, Nov 30, 2015 at 9:49 PM, will sanfilippo <wi...@runtime.io> wrote:
>>>>> Hello:
>>>>> 
>>>>> It turns out the real issue I was having was a stack overflow but in any event it does look like it has uncovered an issue with os_eventq.c. This is the issue; note that tasks are numbered based on their priority (ie. task1 is priority 1, task2 is priority 2, etc).
>>>>> 
>>>>> * Task3 calls os_eventq_get() when there are no events on the event queue. evq->evq_task gets set to the current task.
>>>>> * Task3 goes to sleep.
>>>>> * Task 2 wake up and puts an event on this eventq. os_eventq_put() sees that evq->evq_task is set and calls os_sched_wakeup() which removes Task3 from sleep list and puts it on the run list. Task2 is still running.
>>>>> * Before Task3 can wake up, Task1 preempts Task2 and puts an event on the event queue. Since evq->evq_task is set, os_sched_wakeup() is called but this time the task is already on the run list. This causes an error since os_sched_wakeup() presumes that the task is on the sleep list.
>>>>> 
>>>>> I am not sure the proper fix is to modify os_sched_wakeup() so that it checks the state of the task and only if the state is SLEEP does it remove the task from the sleep list. I think we lose something valuable by allowing os_sched_wakeup() to be called when the task is on the run list. I do agree that “randomly failing” is undesirable so this is what I would do:
>>>>> 
>>>>> * Put an assert() in os_sched_wakeup() and make sure the state is SLEEP.
>>>> 
>>>> I don't like this, I think its fine to wakeup a task that's already
>>>> woken up.  There is nothing wrong with doing so.
>>>> 
>>>>> * Modify the code such that we set evq->evq_task to NULL in os_eventq_put2(). This will prevent us from calling os_sched_wakeup() on future calls to os_eventq_put2().
>>>> 
>>>> +1 - I think we should do this.
>>>> 
>>>>> We could also add a check in os_eventq_put2() so that we only call os_sched_wakeup() if the state is SLEEP.
>>>> 
>>>> I think we should just make os_sched_wakeup fine if we call it on a
>>>> task that's already woken up.
>>>> 
>>>> Sterling
>>> 
> 

Re: Issue with os_eventq.c

[marko kiiskila <ma...@runtime.io>]

I agree with Sterling on this one; I don’t think there is much to be gained by
asserting on task state in os_sched_wakeup(). I’d rather have it return
if the task is already running.

However, I also would make the change you suggest to os_event_put().

> On Dec 1, 2015, at 10:24 AM, Sterling Hughes <st...@gmail.com> wrote:
> 
> Yeah, but whether or not we get a check - how should the function operate.  I'm not sure there was anything wrong in OS eventq for calling this twice, it's just more efficient to not do so.
> 
> I think it's perfectly valid to wake up a task twice, so it doesn't make sense to me to have the code crash when we do that. 
> 
> Sterling 
> 
> 
> 
>> On Dec 1, 2015, at 10:02 AM, will sanfilippo <wi...@runtime.io> wrote:
>> 
>> While there may not be anything “wrong” with adding a check in os_sched_wakeup(), I think we lose something by adding it. If the OS was written in a way that everywhere os_sched_wakeup() was called (or in many places) we checked to see if the task was on the sleep queue, I would agree with you. However, there are no places where this is required (I dont think the code in os_eventq() qualifies; two wrongs dont make a right). In fact, if the task was not on the sleep queue, it would indicate something wrong with the code or a memory overwrite.
>> 
>> I think having an API like os_sched_wakeup() which checks to see if the task is on the sleep queue and asserts if not is a good check on the logic of the rest of the code and will help us find bugs.
>> 
>> Will
>> 
>> 
>>> On Dec 1, 2015, at 9:42 AM, Sterling Hughes <st...@apache.org> wrote:
>>> 
>>> On Mon, Nov 30, 2015 at 9:49 PM, will sanfilippo <wi...@runtime.io> wrote:
>>>> Hello:
>>>> 
>>>> It turns out the real issue I was having was a stack overflow but in any event it does look like it has uncovered an issue with os_eventq.c. This is the issue; note that tasks are numbered based on their priority (ie. task1 is priority 1, task2 is priority 2, etc).
>>>> 
>>>> * Task3 calls os_eventq_get() when there are no events on the event queue. evq->evq_task gets set to the current task.
>>>> * Task3 goes to sleep.
>>>> * Task 2 wake up and puts an event on this eventq. os_eventq_put() sees that evq->evq_task is set and calls os_sched_wakeup() which removes Task3 from sleep list and puts it on the run list. Task2 is still running.
>>>> * Before Task3 can wake up, Task1 preempts Task2 and puts an event on the event queue. Since evq->evq_task is set, os_sched_wakeup() is called but this time the task is already on the run list. This causes an error since os_sched_wakeup() presumes that the task is on the sleep list.
>>>> 
>>>> I am not sure the proper fix is to modify os_sched_wakeup() so that it checks the state of the task and only if the state is SLEEP does it remove the task from the sleep list. I think we lose something valuable by allowing os_sched_wakeup() to be called when the task is on the run list. I do agree that “randomly failing” is undesirable so this is what I would do:
>>>> 
>>>> * Put an assert() in os_sched_wakeup() and make sure the state is SLEEP.
>>> 
>>> I don't like this, I think its fine to wakeup a task that's already
>>> woken up.  There is nothing wrong with doing so.
>>> 
>>>> * Modify the code such that we set evq->evq_task to NULL in os_eventq_put2(). This will prevent us from calling os_sched_wakeup() on future calls to os_eventq_put2().
>>> 
>>> +1 - I think we should do this.
>>> 
>>>> We could also add a check in os_eventq_put2() so that we only call os_sched_wakeup() if the state is SLEEP.
>>> 
>>> I think we should just make os_sched_wakeup fine if we call it on a
>>> task that's already woken up.
>>> 
>>> Sterling
>> 

Re: Issue with os_eventq.c

[Sterling Hughes <st...@gmail.com>]

Yeah, but whether or not we get a check - how should the function operate.  I'm not sure there was anything wrong in OS eventq for calling this twice, it's just more efficient to not do so.

I think it's perfectly valid to wake up a task twice, so it doesn't make sense to me to have the code crash when we do that. 

Sterling 



> On Dec 1, 2015, at 10:02 AM, will sanfilippo <wi...@runtime.io> wrote:
> 
> While there may not be anything “wrong” with adding a check in os_sched_wakeup(), I think we lose something by adding it. If the OS was written in a way that everywhere os_sched_wakeup() was called (or in many places) we checked to see if the task was on the sleep queue, I would agree with you. However, there are no places where this is required (I dont think the code in os_eventq() qualifies; two wrongs dont make a right). In fact, if the task was not on the sleep queue, it would indicate something wrong with the code or a memory overwrite.
> 
> I think having an API like os_sched_wakeup() which checks to see if the task is on the sleep queue and asserts if not is a good check on the logic of the rest of the code and will help us find bugs.
> 
> Will
> 
> 
>> On Dec 1, 2015, at 9:42 AM, Sterling Hughes <st...@apache.org> wrote:
>> 
>> On Mon, Nov 30, 2015 at 9:49 PM, will sanfilippo <wi...@runtime.io> wrote:
>>> Hello:
>>> 
>>> It turns out the real issue I was having was a stack overflow but in any event it does look like it has uncovered an issue with os_eventq.c. This is the issue; note that tasks are numbered based on their priority (ie. task1 is priority 1, task2 is priority 2, etc).
>>> 
>>> * Task3 calls os_eventq_get() when there are no events on the event queue. evq->evq_task gets set to the current task.
>>> * Task3 goes to sleep.
>>> * Task 2 wake up and puts an event on this eventq. os_eventq_put() sees that evq->evq_task is set and calls os_sched_wakeup() which removes Task3 from sleep list and puts it on the run list. Task2 is still running.
>>> * Before Task3 can wake up, Task1 preempts Task2 and puts an event on the event queue. Since evq->evq_task is set, os_sched_wakeup() is called but this time the task is already on the run list. This causes an error since os_sched_wakeup() presumes that the task is on the sleep list.
>>> 
>>> I am not sure the proper fix is to modify os_sched_wakeup() so that it checks the state of the task and only if the state is SLEEP does it remove the task from the sleep list. I think we lose something valuable by allowing os_sched_wakeup() to be called when the task is on the run list. I do agree that “randomly failing” is undesirable so this is what I would do:
>>> 
>>> * Put an assert() in os_sched_wakeup() and make sure the state is SLEEP.
>> 
>> I don't like this, I think its fine to wakeup a task that's already
>> woken up.  There is nothing wrong with doing so.
>> 
>>> * Modify the code such that we set evq->evq_task to NULL in os_eventq_put2(). This will prevent us from calling os_sched_wakeup() on future calls to os_eventq_put2().
>> 
>> +1 - I think we should do this.
>> 
>>> We could also add a check in os_eventq_put2() so that we only call os_sched_wakeup() if the state is SLEEP.
>> 
>> I think we should just make os_sched_wakeup fine if we call it on a
>> task that's already woken up.
>> 
>> Sterling
> 

Re: Issue with os_eventq.c

[will sanfilippo <wi...@runtime.io>]

While there may not be anything “wrong” with adding a check in os_sched_wakeup(), I think we lose something by adding it. If the OS was written in a way that everywhere os_sched_wakeup() was called (or in many places) we checked to see if the task was on the sleep queue, I would agree with you. However, there are no places where this is required (I dont think the code in os_eventq() qualifies; two wrongs dont make a right). In fact, if the task was not on the sleep queue, it would indicate something wrong with the code or a memory overwrite.

I think having an API like os_sched_wakeup() which checks to see if the task is on the sleep queue and asserts if not is a good check on the logic of the rest of the code and will help us find bugs.

Will


> On Dec 1, 2015, at 9:42 AM, Sterling Hughes <st...@apache.org> wrote:
> 
> On Mon, Nov 30, 2015 at 9:49 PM, will sanfilippo <wi...@runtime.io> wrote:
>> Hello:
>> 
>> It turns out the real issue I was having was a stack overflow but in any event it does look like it has uncovered an issue with os_eventq.c. This is the issue; note that tasks are numbered based on their priority (ie. task1 is priority 1, task2 is priority 2, etc).
>> 
>> * Task3 calls os_eventq_get() when there are no events on the event queue. evq->evq_task gets set to the current task.
>> * Task3 goes to sleep.
>> * Task 2 wake up and puts an event on this eventq. os_eventq_put() sees that evq->evq_task is set and calls os_sched_wakeup() which removes Task3 from sleep list and puts it on the run list. Task2 is still running.
>> * Before Task3 can wake up, Task1 preempts Task2 and puts an event on the event queue. Since evq->evq_task is set, os_sched_wakeup() is called but this time the task is already on the run list. This causes an error since os_sched_wakeup() presumes that the task is on the sleep list.
>> 
>> I am not sure the proper fix is to modify os_sched_wakeup() so that it checks the state of the task and only if the state is SLEEP does it remove the task from the sleep list. I think we lose something valuable by allowing os_sched_wakeup() to be called when the task is on the run list. I do agree that “randomly failing” is undesirable so this is what I would do:
>> 
>> * Put an assert() in os_sched_wakeup() and make sure the state is SLEEP.
> 
> I don't like this, I think its fine to wakeup a task that's already
> woken up.  There is nothing wrong with doing so.
> 
>> * Modify the code such that we set evq->evq_task to NULL in os_eventq_put2(). This will prevent us from calling os_sched_wakeup() on future calls to os_eventq_put2().
>> 
> 
> +1 - I think we should do this.
> 
>> We could also add a check in os_eventq_put2() so that we only call os_sched_wakeup() if the state is SLEEP.
>> 
> 
> I think we should just make os_sched_wakeup fine if we call it on a
> task that's already woken up.
> 
> Sterling

Re: Issue with os_eventq.c

[Sterling Hughes <st...@apache.org>]

On Mon, Nov 30, 2015 at 9:49 PM, will sanfilippo <wi...@runtime.io> wrote:
> Hello:
>
> It turns out the real issue I was having was a stack overflow but in any event it does look like it has uncovered an issue with os_eventq.c. This is the issue; note that tasks are numbered based on their priority (ie. task1 is priority 1, task2 is priority 2, etc).
>
> * Task3 calls os_eventq_get() when there are no events on the event queue. evq->evq_task gets set to the current task.
> * Task3 goes to sleep.
> * Task 2 wake up and puts an event on this eventq. os_eventq_put() sees that evq->evq_task is set and calls os_sched_wakeup() which removes Task3 from sleep list and puts it on the run list. Task2 is still running.
> * Before Task3 can wake up, Task1 preempts Task2 and puts an event on the event queue. Since evq->evq_task is set, os_sched_wakeup() is called but this time the task is already on the run list. This causes an error since os_sched_wakeup() presumes that the task is on the sleep list.
>
> I am not sure the proper fix is to modify os_sched_wakeup() so that it checks the state of the task and only if the state is SLEEP does it remove the task from the sleep list. I think we lose something valuable by allowing os_sched_wakeup() to be called when the task is on the run list. I do agree that “randomly failing” is undesirable so this is what I would do:
>
> * Put an assert() in os_sched_wakeup() and make sure the state is SLEEP.

I don't like this, I think its fine to wakeup a task that's already
woken up.  There is nothing wrong with doing so.

> * Modify the code such that we set evq->evq_task to NULL in os_eventq_put2(). This will prevent us from calling os_sched_wakeup() on future calls to os_eventq_put2().
>

+1 - I think we should do this.

> We could also add a check in os_eventq_put2() so that we only call os_sched_wakeup() if the state is SLEEP.
>

I think we should just make os_sched_wakeup fine if we call it on a
task that's already woken up.

Sterling

Re: Issue with os_eventq.c

[marko kiiskila <ma...@runtime.io>]

> On Nov 30, 2015, at 9:49 PM, will sanfilippo <wi...@runtime.io> wrote:
> 
...
> I am not sure the proper fix is to modify os_sched_wakeup() so that it checks the state of the task and only if the state is SLEEP does it remove the task from the sleep list. I think we lose something valuable by allowing os_sched_wakeup() to be called when the task is on the run list. I do agree that “randomly failing” is undesirable so this is what I would do:
> 
> * Put an assert() in os_sched_wakeup() and make sure the state is SLEEP.
> * Modify the code such that we set evq->evq_task to NULL in os_eventq_put2(). This will prevent us from calling os_sched_wakeup() on future calls to os_eventq_put2().
> 
> We could also add a check in os_eventq_put2() so that we only call os_sched_wakeup() if the state is SLEEP.
> 
> Thoughts?

Fix sounds fine. That would make that wakeup look the same as the others (mutex/semaphore etc).
These all take the task out of lock’s sleep list when it’s time to go.
—
M

Re: Issue with os_eventq.c

[Christopher Collins <ch...@runtime.io>]

On Mon, Nov 30, 2015 at 09:49:25PM -0800, will sanfilippo wrote:
> I am not sure the proper fix is to modify os_sched_wakeup() so that it
> checks the state of the task and only if the state is SLEEP does it
> remove the task from the sleep list. I think we lose something
> valuable by allowing os_sched_wakeup() to be called when the task is
> on the run list. I do agree that “randomly failing” is undesirable so
> this is what I would do:
> 
> * Put an assert() in os_sched_wakeup() and make sure the state is
> SLEEP.
> * Modify the code such that we set evq->evq_task to NULL in
> os_eventq_put2(). This will prevent us from calling os_sched_wakeup()
> on future calls to os_eventq_put2().
> 
> We could also add a check in os_eventq_put2() so that we only call
> os_sched_wakeup() if the state is SLEEP.
> 
> Thoughts?
> 
> Will

I think both solutions are acceptable, but I have a slight preference
for adding the assert() call.  I am overgeneralizing here, but if you
allow a procedure to be applied redundantly (e.g., os_sched_assert()), I
think that can pave the way for code that is difficult to understand.
With the assert(), you have some assurance that you understand the state
of the system whenever os_sched_wakeup() is called.  Of course, if you
have to jump through a lot of hoops to arrange this, it probably isn't
worth it.

Chris