Improve POSIX port functionality #914
Conversation
|
|
||
| if( iRet != 0 ) | ||
| { | ||
| fprintf( stderr, "[WARN] pthread_attr_setstack failed with return value: %d. Default stack will be used.\n", iRet ); | ||
| fprintf( stderr, "[WARN] Increase the stack size to PTHREAD_STACK_MIN.\n" ); | ||
| fprintf( stderr, "[WARN] pthread_attr_setstacksize failed with return value: %d. Default stack size will be used.\n", iRet ); |
There was a problem hiding this comment.
Isn't this log message unnecessary now that the stack size is set to be PTHREAD_STACK_MIN if ulStackSize is less than PTHREAD_STACK_SIZE?
There was a problem hiding this comment.
pthreaad_attr_setstacksize() could still fail and in those cases I thought it was useful to report it to the user. It should be very rare but there are a number of conditions that could result in it failing and given that this is an OS it seemed like we should handle all error conditions in some manner, even if that's a warning printout since the consequences are usually minimal if this does fail.
cmorganBE
force-pushed
the
cmm_posix
branch
2 times, most recently
from
844cd8a
to
c4ffc40
Compare
|
@Skptak can you re-initiate the workflows? I was waiting for them to complete and just realized they don't run without approval.... |
|
Hey @cmorganBE, thanks for moving that idle hook call to work the way you have it set up now! There are a few FreeRTOS Style Guide guide differences in your PR than what we use. For example the Where I was wondering if you'd be fine with me adding a few commits to your PR to address these problems? If you want to do it yourself that's fine as well! I appreciate the effort you've put into this PR so far, and thanks again for your contribution! |
|
Hi @Skptak . It's no trouble. We've got a ton of code that would be helped by having this working better and its been interesting learning how the port works and more about freertos internals. If you want to add commits and feel they align with the spirit of the PR feel free to do so. On the timer tick thread, this isn't a FreeRTOS managed thread per-se, that's why I didn't use Thread_t there, there isn't much use for most of the stuff in that struct. Renaming it to pxTimerTask might make sense but again, it's not a freeRTOS task at the moment. I think it wouldn't make sense to make it a FreeRTOS task either, in that case the task would end up being suspended and I think the signals that serve as interrupts in this approach wouldn't be generated correctly.
|
|
@cmorganBE |
cmorganBE
force-pushed
the
cmm_posix
branch
2 times, most recently
from
fc37d5f
to
45a6885
Compare
Yes. I edited the reply cause this information is wrong and no need to be discussed. In this PR, SIGALARM will only be sent to the thread which is running the FreeRTOS current task. |
|
@cmorganBE
Yes, this is one of the way how you can use vApplicationIdleHook. From your previous description. The idle hook introduced in this PR tries to create a cancellation point for the idle task. Otherwise, the idle task will be running in a loop and can't be cancelled by main thread. I would like to discuss the following scenarios where the vTaskEndScheduler is called:
Since vTaskEndScheduler is only called from a FreeRTOS task, I don't think portHAS_IDLE_HOOK is necessary to cancel the idle task. |
|
@chinglee-iot I also don't see any test failures if I disable the cancellation point. I'll drop the port idle change off in the next push of this branch. I can't help but think that if the idle thread isn't waiting on an event that we'd have an issue where it never hit a cancellation point. Perhaps that behavior was changed by the directed signals. In any case I'll drop it and keep a copy of it here locally just in case. I'll also update the comments to use c style comments, and adjust various names as you've mentioned and across the changes made. |
…ling with non-FreeRTOS pthreads Improve upon the elegant approach of using signals to cause task/pthreads suspension and scheduler execution by using directed signals. This fixes: - Deadlocks in non-FreeRTOS pthreads - Multiple FreeRTOS tasks(pthreads) incorrectly running at the same time By directing the signals using pthread_kill() the signal handler in the presently running FreeRTOS task/pthread will be called, ensuring that the scheduler runs both in the context of a FreeRTOS task/pthread and from the presently executing FreeRTOS task/pthread. Details ============== The POSIX port uses signals to preempt FreeRTOS tasks (implemented as pthreads), a very neat and elegant approach to forcing tasks/pthreads to suspend and run the scheduler. Signal handlers are process global. Posix timers generate signals when the timer expires, and the signal is sent to the currently running pthread. In systems where there are pthreads that are NOT a result of creating FreeRTOS tasks, such as the entry point thread that calls main(), or user created pthreads, this poses a serious issue. While the POSIX port only allows a single FreeRTOS pthread to run at once, by causing all suspended threads to not be scheduled due to their waiting on a pthread condition variable, this isn't the case with non-FreeRTOS pthreads. Thus it is possible that a non-FreeRTOS pthread is running when the timer expires and the signal is generated. This results in the signal handler running in the non-FreeRTOS thread. The sequence of events results in these events from signal handler context: - vPortSystemTickHandler() being called - The scheduler running - Selecting another FreeRTOS task to run and switching the active task - The newly selected task released from suspension by pthread_cond_signal() - The presently active thread calling event_wait() - The pthread calling pthread_cond_wait(), suspending the thread and allowing the host OS scheduler to schedule another thread to run. If this occurs from a non-FreeRTOS thread this results in: - The active FreeRTOS pthread (Task A/Thread A) continuing to run (as the signal handler that calls event_wait() ran instead in a non-FreeRTOS pthread. - The pthread where the signal handler did run (Thread B) will call event_wait() and pthread_cond_wait(), but on the condition variable of the previously active FreeRTOS task, oops. This causes the non-FreeRTOS pthread to block unexpectedly relative to what the developer might have expected. - The newly selected FreeRTOS Task (Task C/Thread C) will resume and start running. At this point Task A/Thread A is running concurrently with Task C/Thread C. While this may not necessarily be an issue, it does not replicate the expected behavior of a single Task running at once. Note that Thread B will resume if/when Task A/ThreadA is switched to. However, this could be delayed by an arbitrary amount of time, or could never occur. Also note that if there are multiple non-FreeRTOS pthreads that Thread D, E, F...etc could suffer the same fate as Thread B, if the scheduler were to suspend Task C/Thread C and resume Task E/Thread E. Implementation ============== Timer details ------------- A standalone pthread for the signal generation thread was chosen, rather than using a posix timer_settime() handler function because the latter creates a temporary pthread for each handler callback. This makes debugging much more difficult due to gdb detecting the creation and destruction of these temporary threads. Signal delivery -------------- While signal handlers are per-thread, it is possible for pthreads to selectively block signals, rather than using thread directed signals. However, the approach of blocking signals in non-FreeRTOS pthreads adds complexity to each of these non-FreeRTOS pthreads including ensuring that these signals are blocked at thread creation, prior to the thread starting up. Directed signals removes the requirement for non-FreeRTOS pthreads to be aware of and take action to protect against these signals, reducing complexity.
For a clean shutdown where memory is freed, it is necessary for all pthreads to be joined at shutdown. Previously there was explicit cancellation of the idle task and timer daemon task, however there may be a number of other tasks in the system, both system created and user created, and those tasks/threads were being left at shutdown. This change calls pthread_cancel()/pthread_join() on all FreeRTOS managed pthreads upon shutdown.
|
@chinglee-iot addressed naming, the port idle change, comments, and a few other minor formatting things with this latest push. |
|
@cmorganBE
Please help to take a look. I will also revert the posix change in #944. Some update about re-initialize PR. Pseudo example code : /* First time start scheduler. */
vTaskStartScheduler();
/* End scheduler in a FreeRTOS task. */
vTaskEndScheduler();
/* Calling FreeRTOS reset state API before next time restart scheduler. */
#if ( configUSE_CO_ROUTINES == 1 )
{
vCoRoutineResetState();
}
#endif
#if ( configUSE_TIMERS == 1 )
{
vTimerResetState();
}
#endif /* #if ( configUSE_TIMERS == 1 ) */
#if ( configSUPPORT_DYNAMIC_ALLOCATION == 1 )
vPortHeapResetState();
#endif
vTaskResetState();
/* Restart scheduler. */
vTaskStartScheduler();
/* End scheduler in a FreeRTOS task. */
vTaskEndScheduler(); |
|
@chinglee-iot only had one comment on the pthread stack stuff on the other PR. |
…y itself, to allowing them to specify the stack size Change from pthread_attr_setstack() to pthread_attr_setstacksize(), and automatically adjust the stack size to be at least PTHREAD_STACK_MIN if it wasn't already, removing the size warning. This permits the user to increase the pthread stack size beyond the PTHREAD_STACK_MIN default of 16384 if desired, without producing a warning in the typical case where stacks are minimized for RAM limited targets. Continue to store thread paramters on the provided stack, for consistency with the MCU targets. Previously pthread_attr_setstack() was used to enable user defined stacks. Note that: 1. The stack size can still be specified by the user. 2. pxPortInitialiseStack(), and pthread_addr_setstack() was failing on stacks of typical size, as these are smaller than PTHREAD_STACK_MIN (16384) bytes, and printing out a series of warnings. Improve usability by having the posix port automatically increase the stack size to be at least PTHREAD_STACK_MIN as posix platforms have enough memory for this not to be a concern. 3. Reuse of stack memory will also result in valgrind 'invalid write' errors to what is demonstrably valid memory. Root cause is that Valgrind is tracking a stack pointer as the stack is used. Reuse of a stack buffer results in the stack being used at its start, in an area that Valgrind thinks is far away from the start of the stack. There are ways to notify Valgrind of these changes however this would require linking against and calling Valgrind functions from the FreeRTOS application using the posix port, https://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.clientreq. Also, apparently it isn't permitted by posix to reuse stack memory once its been used in a pthread via pthread_attr_setstack(), see https://stackoverflow.com/a/5422134
|
@chinglee-iot merged and re-added the stack size commit, let me know if it looks ok. Rewrote the commit log there to improve the arguments for the change. |
|
@chinglee-iot all tests except the restart ones pass from git@github.com:cmorganBE/freertos_posix.git |
|
PR #914 caused Posix Port to fail to build on MacOS. This PR fixes teh build failure. This PR also adds a Matrix configuration to the GitHub kernel-demo workflow to build the Posix Demos on MacOS. --------- Co-authored-by: chinglee-iot <61685396+chinglee-iot@users.noreply.github.com> Co-authored-by: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com>
Posix port improvements.
I'm interested in feedback on these changes. You can see the test applications here, https://github.com/cmorganBE/freertos_posix
Description
The posix port had a handful of issues that were making it difficult to use, including:
These commits build upon the posix port to resolve these issues.
Test Steps
Checklist:
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.