Posix: Fix no task switching issue if a task ended its main function #184

RedaMaher · 2020-09-24T11:39:17Z

Description

When the main function of a task exits, no task switching happened.
This is because all the remaining tasks are waiting on the condition
variable. The fix is to trigger a task switch and mark the exiting
task as "Dying" to be suspened and exited properly from the scheduler.

Test Steps

Create an application with multi tasks. Make one task to exit normally from its main function. Then no task switching will happen.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

alfred2g · 2020-09-28T23:37:13Z

portable/ThirdParty/GCC/Posix/utils/wait_for_event.c

+    if ( ev )
+    {
+        pthread_mutex_destroy( &ev->mutex );
+        pthread_cond_destroy( &ev->cond );
+        free( ev );
+        *ev_ptr = NULL;
+    }


This would hide the doube deletion of an event if called twice.
We would like it to fail as soon as possible, as it could make a design issue not apparent by trying to delete a task twice.
This is similar to how free works, if you call free twice on a pointer the second time it would crash instead of checking the validity of that pointer

Yes, you are correct. This will hide the double deletion. I added this check to be able to work with the fix in PR #181 when event_delete where called from two places. But now this is not needed after refining the fix of PR #181 by deleting the condition variable in the correct place (When a thread is canceled).

When the main function of a task exits, no task switching happened. This is because all the remaining tasks are waiting on the condition variable. The fix is to trigger a task switch and mark the exiting task as "Dying" to be suspened and exited properly from the scheduler.

cobusve · 2020-09-28T07:12:59Z

portable/ThirdParty/GCC/Posix/port.c

+	 * with any value to trigger a task switch where the task will
+	 * be suspended and exited. */
+	pxThread->xDying = pdTRUE;
+	vTaskDelay( portTASK_ENDED_DELAY );


I think it is better to call taskYIELD() here ?

It will return to the same task when there are no other tasks at a higher or equal priority. So I get stuck again.

The main issue is the delay
Why was it set to 500 ? would it work with 200? what about 100? etc
Can we make it so it doesn't wait at all?

The delay can be with any value as I described it in the comment. The task actually will not wait at all. Once the task switching is in place, the task will exit as it is marked as "Dying". The purpose of the delay is to trigger an actual task switching and call prvSwitchThread with different task handles.

Hmmm, I am still confused here. It seems like we are talking about the case where a FreeRTOS task function returns. By design when this happens the stack will be corrupted and the OS will crash, so what are we trying to do here?

I am trying to understand under which circumstances this code is helping us.

In the case above, after 500 ticks the scheduler will schedule your task again, and then you will be in exactly the same place where you would have been had you just called taskYIELD().

A task should call vTaskDelete( NULL ) at its end.

@RichardBarry The task resources will be freed in the idle task when the task deletes itself. This does not handle the following case:

Task (x) allocated memory to be used for a new higher priority static task (y).

Task (x) created a static task (y) and used the allocated memory from the previous step.

Task (y) scheduled then finished and deleted itself.

Scheduler returns to Task (x) which was waiting for Task (y) to finish.

Task (x) frees the allocated resources.

When The Idle task runs, it will try to free the resources of Task (y) and access a freed memory (and causes a Memory Corruption)

So I proposed to delete Task (y) from the Task(x) (to trigger immediate resources freeing) and handle the task exit properly.
There is a workaround for this issue by adding vTaskDelay in Task (x) (before step 5) to give a chance for the idle task to run and free the resources of Task (y).

@cobusve Yes, I was trying to handle the case where the task function finishes. And It looks like it is not the proper way by design. I got confused because the task is exiting silently in Posix port without assertion or crashes. I will update the PR to just assert in this case to be easy to catch.

@RichardBarry @cobusve any suggestion on how to handle the above scenario?

cobusve · 2020-10-01T03:08:20Z

portable/ThirdParty/GCC/Posix/port.c

 	pxThread->pxCode( pxThread->pvParams );

+	/* Task function should not return */
+	prvTaskExitError();


I think the normal way to handle asserts is like this https://github.com/FreeRTOS/FreeRTOS/blob/6cc5310f380524c7885181a3be0c60ead4b59a22/FreeRTOS/Demo/Posix_GCC/main.c#L283

That means you probably just have to call configASSERT( pdFALSE ); over here and be done.

@cobusve This is exactly what prvTaskExitError(); is doing in addition to stop if configASSERT() is not actually configured. This is useful to cover the issue of task ending in FreeRTOS and be easy to catch.
The same function is used in many ports (For example https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/master/portable/GCC/ARM_CM3/port.c#L194)

In some ports it is defined by the demo/application, and in some ports it is defined by the port
in this case this function is defined by the demo application in: https://github.com/FreeRTOS/FreeRTOS/blob/master/FreeRTOS/Demo/Posix_GCC/main.c#L283

Sorry I did not get what is the issue here. If Posix port can be used without defining configASSERT then the stop loop is needed. If Posix port can not be used without defining configASSERT then no need for the stop loop but we need to write this down in some documentation.

https://www.freertos.org/a00110.html#configASSERT
As you can see in the doc, config assert is an application side setting not a port side setting, as the user has the option of turning it on or off depending on the maturity of the application in order to save space...

This is exactly what I am talking about. So that the stop loop in prvTaskExitError after config assert is important to avoid dead silent stop when config assert is not set in the application side

If the user is confident about his application, and is sure that they called vTaskDelete at the end of every task, extra code and code size is not desired, so vConfigAssert will rightfully be disabled to remove the extra code. By adding the prvTaskExitError function this would not be possible

Worth mentioning, this is a linux port and extra code is not as important, but to stay consistent, with other new ports, be as true as possible to an embedded environment, and provide a good example.

alfred2g reviewed Sep 28, 2020

View reviewed changes

RedaMaher force-pushed the fix_posix_no_switching_after_task_ended branch from f76b9fa to a4e007e Compare September 29, 2020 09:16

Merge branch 'master' into fix_posix_no_switching_after_task_ended

61ac0e4

alfred2g previously approved these changes Sep 29, 2020

View reviewed changes

Merge branch 'master' into fix_posix_no_switching_after_task_ended

dbae076

cobusve reviewed Sep 29, 2020

View reviewed changes

alfred2g self-requested a review September 30, 2020 05:49

Posix: Assert and stop if the Task function returned

905bd47

RedaMaher dismissed alfred2g’s stale review via 905bd47 September 30, 2020 10:13

alfred2g self-assigned this Sep 30, 2020

alfred2g previously approved these changes Oct 1, 2020

View reviewed changes

Merge branch 'master' into fix_posix_no_switching_after_task_ended

0764b3a

cobusve reviewed Oct 1, 2020

View reviewed changes

alfred2g self-requested a review October 1, 2020 03:14

Merge branch 'master' into fix_posix_no_switching_after_task_ended

30d9517

RedaMaher dismissed alfred2g’s stale review via 909eee6 October 4, 2020 21:35

Posix: just assert if a task returned from its main function

7f0cad3

RedaMaher force-pushed the fix_posix_no_switching_after_task_ended branch from 909eee6 to 7f0cad3 Compare October 4, 2020 21:40

Merge branch 'master' into fix_posix_no_switching_after_task_ended

ed40292

alfred2g approved these changes Oct 5, 2020

View reviewed changes

alfred2g requested a review from cobusve October 5, 2020 23:13

cobusve approved these changes Oct 6, 2020

View reviewed changes

alfred2g merged commit 77ad717 into FreeRTOS:master Oct 6, 2020

Posix: Fix no task switching issue if a task ended its main function #184

Posix: Fix no task switching issue if a task ended its main function #184

Uh oh!

Conversation

RedaMaher commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Steps

Related Issue

Uh oh!

alfred2g Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cobusve Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RedaMaher Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfred2g Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfred2g Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RedaMaher commented Sep 24, 2020 •

edited

Loading

alfred2g Sep 28, 2020 •

edited

Loading

cobusve Sep 29, 2020 •

edited

Loading

RedaMaher Sep 30, 2020 •

edited

Loading

alfred2g Oct 1, 2020 •

edited

Loading

alfred2g Oct 2, 2020 •

edited

Loading