Remove run state assertion in prvCheckForRunStateChange

chinglee-iot commented 3 months ago

Description

Address the problem in this thread.

https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/76eb44382178237197a2c4982f9d99e8d64c9599/tasks.c#L846-L853 In prvCheckForRunStateChange(), enabling interrupts should cause this core to immediately service the pending interrupt and yield. Upon the next scheduling of the task, the assertion configASSERT(pxThisTCB->xTaskRunState != taskTASK_SCHEDULED_TO_YIELD); may not be true, as other cores could have requested a yield for this task before it evaluates its run state within the assertion. To address this, the task re-evaluates its run state in critical section within a loop until it is eligible for execution, which is the current implementation. Consequently, this assertion should be removed to ensure correct behavior.

In this PR:

Remove run state assertion in prvCheckForRunStateChange() as it doesn't account for the possibility that the task run state could be modified by another task after interrupt is enabled and checking the run state of a task should be performed in a critical section.

Test Steps

Forum user helps to test this on a Cortex A53 SMP port, with this PR. The main_full demo can run for 5 hours without problem.

Checklist:

[x] I have tested my changes. No regression in existing tests.
[x] I have modified and/or added unit-tests to cover the code changes in this Pull Request. - https://github.com/FreeRTOS/FreeRTOS/pull/1231

Related Issue

Forum thread.

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

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.31%. Comparing base (8c49c54) to head (62940d3). Report is 46 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1093 +/- ## ========================================== - Coverage 93.00% 92.31% -0.69% ========================================== Files 6 6 Lines 3200 3226 +26 Branches 879 885 +6 ========================================== + Hits 2976 2978 +2 - Misses 111 132 +21 - Partials 113 116 +3 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/1093/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/1093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `92.31% <ø> (-0.69%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Saiiijchan commented 3 months ago

Excuse me, I am confused with this patch.

Upon the next scheduling of the task, the assertion configASSERT(pxThisTCB->xTaskRunState != taskTASK_SCHEDULED_TO_YIELD); may not be true, as other cores could have requested a yield for this task before it evaluates its run state within the assertion.

Why not it may be true, if other cores request a yield, it will send a irq to this core firstly and change its task run state.

#define prvYieldCore( xCoreID ) \ else \ { \ /* Request other core to yield if it is not requested before. */ \ if( pxCurrentTCBs[ ( xCoreID ) ]->xTaskRunState != taskTASK_SCHEDULED_TO_YIELD ) \ { \ portYIELD_CORE( xCoreID ); \ pxCurrentTCBs[ ( xCoreID ) ]->xTaskRunState = taskTASK_SCHEDULED_TO_YIELD; \ } \ } \ } while( 0 )

Does it mean that the irq is not be responded if run state is taskTASK_SCHEDULED_TO_YIELD？

chinglee-iot commented 3 months ago

@Saiiijchan I think the assumption for this configASSERT is that the core which is requested to yield will yield immediately without any latency. However, on some platforms, the core can still execute some instructions before it jumps to the interrupt handler for the yield request. The proper way to check the run state should be performed in a critical section and in a loop.

FreeRTOS / FreeRTOS-Kernel