Fix xTaskNotifyWait & ulTaskNotifyTake determinism. #625
Conversation
karver8
force-pushed
the
deterministic-task-notify
branch
from
5ff4250 to
eeed340
Compare
- Delayed task list is no longer walked while interrupts are disabled. - Fixed comment that could lead to similar mistakes in the future. - Minimize performance impact.
karver8
force-pushed
the
deterministic-task-notify
branch
from
eeed340 to
8daadaf
Compare
|
Let me know if there is anything I need to do to move this forward. It looks like unit tests need to be updated in the other repository, these should be pretty simple changes but I am not sure how that would link up to this PR. |
|
The code seems to work on an RP2040 but fails unit tests. These failures need to be checked and updated as required. |
|
@n9wxu Yes I have looked at the failures and I know how to fix them, but how do I synchronize changes to the unit test repo with changes in this repository? I can submit a PR in the FreeRTOS repo to fix the unit tests, but it looks like that will break the tests until the FreeRTOS-kernel submodule is updated? |
| @@ -3137,8 +3137,8 @@ void vTaskPlaceOnEventList( List_t * const pxEventList, | |||
| { | |||
| configASSERT( pxEventList ); | |||
|
|
|||
| /* THIS FUNCTION MUST BE CALLED WITH EITHER INTERRUPTS DISABLED OR THE | |||
| * SCHEDULER SUSPENDED AND THE QUEUE BEING ACCESSED LOCKED. */ | |||
| /* THIS FUNCTION MUST BE CALLED WITH THE SCHEDULER SUSPENDED AND THE QUEUE | |||
There was a problem hiding this comment.
Why was this comment changed? The original "Interrupts disabled" should probably be "inside a critical section", but I think it's still valid.
There was a problem hiding this comment.
I changed it because this function calls vListInsert(), which shouldn't be called while interrupts are disabled as far as I am aware. I noted this comment in my original issue. But I can change it back if you think this is not correct.
| /* Suspend the scheduler before enabling interrupts. */ | ||
| vTaskSuspendAll(); | ||
| taskEXIT_CRITICAL(); |
There was a problem hiding this comment.
Strictly speaking, the API doesn't allow calling vTaskSuspendAll() from inside a critical section. It probably works, but it would be the only place it happens in the FreeRTOS codebase.
The intent behind the larger construct being proposed here seems to be to avoid suspending/resuming the scheduler if possible, which in turn keeps ulTaskGenericNotifyTake() as lightweight as possible. Maybe a different construct could be used instead. For example, the idle task uses a different construct to avoid suspending/resuming the scheduler for tickless idle. A similar approach might work here -- specifically, checking the notification value while "unprotected" to determine whether to suspend the scheduler, and then verifying the notification value after suspending the scheduler if suspending is warranted.
This PR fixes the bug described in the following issue: #612. This was originally contributed in the following PR: #625. The implementation suspends the scheduler before exiting the critical section (i.e. before enabling interrupts). If we do not do so, a notification sent from an ISR, which happens after exiting the critical section and before suspending the scheduler, will get lost. The sequence of events will be: 1. Exit critical section. 2. Interrupt - ISR calls xTaskNotifyFromISR which adds the task to the Ready list. 3. Suspend scheduler. 4. prvAddCurrentTaskToDelayedList moves the task to the delayed or suspended list. 5. Resume scheduler does not touch the task (because it is not on the pendingReady list), effectively losing the notification from the ISR. The same does not happen when we suspend the scheduler before exiting the critical section. The sequence of events in this case will be: 1. Suspend scheduler. 2. Exit critical section. 3. Interrupt - ISR calls xTaskNotifyFromISR which adds the task to the pendingReady list as the scheduler is suspended. 4. prvAddCurrentTaskToDelayedList adds the task to delayed or suspended list. Note that this operation does not nullify the add to pendingReady list done in the above step because a different list item, namely xEventListItem, is used for adding the task to the pendingReady list. In other words, the task still remains on the pendingReady list. 5. Resume scheduler moves the task from pendingReady list to the Ready list. ------------ Co-authored-by: Jacob Carver <[email protected]>
|
Hi @karver8 |
|
Thanks! @kar-rahul-aws |
This PR fixes the bug described in the following issue: FreeRTOS#612. This was originally contributed in the following PR: FreeRTOS#625. The implementation suspends the scheduler before exiting the critical section (i.e. before enabling interrupts). If we do not do so, a notification sent from an ISR, which happens after exiting the critical section and before suspending the scheduler, will get lost. The sequence of events will be: 1. Exit critical section. 2. Interrupt - ISR calls xTaskNotifyFromISR which adds the task to the Ready list. 3. Suspend scheduler. 4. prvAddCurrentTaskToDelayedList moves the task to the delayed or suspended list. 5. Resume scheduler does not touch the task (because it is not on the pendingReady list), effectively losing the notification from the ISR. The same does not happen when we suspend the scheduler before exiting the critical section. The sequence of events in this case will be: 1. Suspend scheduler. 2. Exit critical section. 3. Interrupt - ISR calls xTaskNotifyFromISR which adds the task to the pendingReady list as the scheduler is suspended. 4. prvAddCurrentTaskToDelayedList adds the task to delayed or suspended list. Note that this operation does not nullify the add to pendingReady list done in the above step because a different list item, namely xEventListItem, is used for adding the task to the pendingReady list. In other words, the task still remains on the pendingReady list. 5. Resume scheduler moves the task from pendingReady list to the Ready list. ------------ Co-authored-by: Jacob Carver <[email protected]>
Description
Fixes the bug described in this issue:
#612
One unorthodox thing being done here, which I feel should be pointed out, is that I am calling
vTaskSuspendAll()from inside a critical section. This is done to minimize the performance impact of this change, since one of the main benefits of task notifications is that they are faster than the other primitives. The only worry I see here is that it may cause issues with certain ports if they do tricky things insideportSOFTWARE_BARRIER()orportMEMORY_BARRIER(), but I cannot think of any examples.Test Steps
As suggested by Richard Barry, adding
configASSERT( ulCriticalNesting == 0UL ), intovListInsert(), which is a condition that should always be met, will cause it to be triggered by a call toxTaskNotifyWait()orulTaskNotifyTake()if the task blocks. This of course only works on ports that have the variableulCriticalNesting.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.