Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FWS-1372 Fix os_eventq_put() to wake up task only if EVQ_WAIT flag is set #3117

Closed
wants to merge 1 commit into from
Closed

FWS-1372 Fix os_eventq_put() to wake up task only if EVQ_WAIT flag is set #3117

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion kernel/os/src/os_eventq.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ os_eventq_put(struct os_eventq *evq, struct os_event *ev)
* Check if task is sleeping, because another event
* queue may have woken this task up beforehand.
*/
if (evq->evq_task->t_state == OS_TASK_SLEEP) {
if (evq->evq_task->t_state == OS_TASK_SLEEP &&
evq->evq_task->t_flags & OS_TASK_FLAG_EVQ_WAIT) {
Comment on lines +72 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this does not break anything I don't think it solves the problem.
evq->evq_task is only set when task calls os_eventq_get() this is blocking call so it's strange that same task can later call os_sem_pend().
For me it more likely looks like memory corruption of task struct.

Copy link
Contributor Author

@vrahane vrahane Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I did not do a good job at explaining this. So, what is happening is that we have the default task waiting on a semaphore. The task actually wakes up when os_eventq_put() is called which should not happen, it should only wakeup when os_sem_release() is called on the pending semaphore. So, what this change does is it forces the scheduler to be blocked on the semaphore.
The above code did the right thing back in 2015-2016 before we added os_eventq_poll(). You will have to go back into incubator-mynewt-core to look at the exact code. The functionality broke when OS_TASK_SLEEP flag was added I believe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what this change does is it forces the scheduler to be blocked on the semaphore even though an event is scheduled if thats what the task has been waiting for.

This is exactly what's impossible here: task cannot be blocked on semaphore and wait for an event at the same time.

The condition you modify is only executed if evq->evq_task != NULL and this is only valid if task is blocked on os_eventq_get. If you have task blocked on a semaphore which is also set as evq_task in some eventq then something is broken elsewhere.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the code in more detail (which I should have done before approving admittedly) and yes you are correct. Checking evq_task first will guarantee the above situation does not occur. This is good :-) Thanks guys

Copy link
Contributor Author

@vrahane vrahane Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, makes complete sense. We got a bit carried away by the semaphore behavior which contributed to the issue.

os_sched_wakeup(evq->evq_task);
resched = 1;
}
Expand Down