Remove run state assertion in prvCheckForRunStateChange

[chinglee-iot]

Description

Address the problem in this thread.

FreeRTOS-Kernel/tasks.c

Lines 846 to 853 in 76eb443

	portENABLE_INTERRUPTS();
	/* Enabling interrupts should cause this core to immediately
	* service the pending interrupt and yield. If the run state is still
	* yielding here then that is a problem. */
	configASSERT( pxThisTCB->xTaskRunState != taskTASK_SCHEDULED_TO_YIELD );
	portDISABLE_INTERRUPTS();

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:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request. - Remove the configASSERT test for prvCheckForRunStateChange FreeRTOS#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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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	Coverage Δ
unittests	92.31% <ø> (-0.69%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Saiiijchan]

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]

@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.