Skip to content

Commit d33a460

Browse files
sudeep-mohantysudeep-mohanty
committed
fix(freertos-smp): Fixed deferred action race condition
When TCB locks are used, we observe a race condition before taking a deferred action which can result in a task being orphaned and hence does not undergo the deferred action. THis can lead to memory leaks. This commit fixes the issue by ensuring that we do not allow a task to yield when a deferred action is pending.
1 parent 118bfde commit d33a460

File tree

1 file changed

+109
-34
lines changed

1 file changed

+109
-34
lines changed

‎tasks.c‎

Lines changed: 109 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,8 @@
370370
/* Acquire the target core's TCB spinlock to prevent race with vTaskPreemptionDisable. */ \
371371
portGET_SPINLOCK( xCurrentCoreID, &( pxCurrentTCBs[ xCoreToYield ]->xTCBSpinlock ) ); \
372372
{ \
373-
if( pxCurrentTCBs[ xCoreToYield ]->uxPreemptionDisable == 0U ) \
373+
if( ( pxCurrentTCBs[ xCoreToYield ]->uxPreemptionDisable == 0U ) && \
374+
( pxCurrentTCBs[ xCoreToYield ]->uxDeferredStateChange == 0U ) ) \
374375
{ \
375376
/* Request other core to yield if it is not requested before. */ \
376377
if( pxCurrentTCBs[ xCoreToYield ]->xTaskRunState != taskTASK_SCHEDULED_TO_YIELD ) \
@@ -397,7 +398,8 @@
397398
} \
398399
else \
399400
{ \
400-
if( pxCurrentTCBs[ ( xCoreID ) ]->uxPreemptionDisable == 0U ) \
401+
if( ( pxCurrentTCBs[ ( xCoreID ) ]->uxPreemptionDisable == 0U ) && \
402+
( pxCurrentTCBs[ ( xCoreID ) ]->uxDeferredStateChange == 0U ) ) \
401403
{ \
402404
/* Request other core to yield if it is not requested before. */ \
403405
if( pxCurrentTCBs[ ( xCoreID ) ]->xTaskRunState != taskTASK_SCHEDULED_TO_YIELD ) \
@@ -2428,18 +2430,39 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
24282430

24292431
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
24302432

2433+
/* When TCB data group lock is enabled, we must acquire the target TCB's spinlock
2434+
* to prevent a race condition with prvTaskPreemptionEnable(). */
2435+
#if ( configUSE_TCB_DATA_GROUP_LOCK == 1 )
2436+
{
2437+
const BaseType_t xCoreID = ( BaseType_t ) portGET_CORE_ID();
2438+
portGET_SPINLOCK( xCoreID, &pxTCB->xTCBSpinlock );
2439+
}
2440+
#endif /* configUSE_TCB_DATA_GROUP_LOCK */
2441+
24312442
/* If the task has disabled preemption, we need to defer the deletion until the
2432-
* task enables preemption. The deletion will be performed in vTaskPreemptionEnable(). */
2443+
* task enables preemption. The deletion will be performed in prvTaskPreemptionEnable(). */
24332444
if( pxTCB->uxPreemptionDisable > 0U )
24342445
{
2435-
pxTCB->uxDeferredStateChange |= tskDEFERRED_DELETION;
2446+
/* Deletion takes priority over suspension. Clear any pending suspension
2447+
* and set only the deletion flag. */
2448+
pxTCB->uxDeferredStateChange = tskDEFERRED_DELETION;
24362449
xDeferredDeletion = pdTRUE;
24372450
}
24382451
else
24392452
{
2440-
/* Reset the deferred state change flags */
2441-
pxTCB->uxDeferredStateChange &= ~tskDEFERRED_DELETION;
2453+
/* Reset all deferred state change flags since the task is being
2454+
* deleted. Any pending suspension is now irrelevant. Clearing all
2455+
* flags is necessary to allow the yield to happen in vTaskExitCritical
2456+
* which checks uxDeferredStateChange == 0. */
2457+
pxTCB->uxDeferredStateChange = 0U;
2458+
}
2459+
2460+
#if ( configUSE_TCB_DATA_GROUP_LOCK == 1 )
2461+
{
2462+
const BaseType_t xCoreID = ( BaseType_t ) portGET_CORE_ID();
2463+
portRELEASE_SPINLOCK( xCoreID, &pxTCB->xTCBSpinlock );
24422464
}
2465+
#endif /* configUSE_TCB_DATA_GROUP_LOCK */
24432466
#endif /* configUSE_TASK_PREEMPTION_DISABLE */
24442467

24452468
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
@@ -3449,7 +3472,8 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
34493472
BaseType_t xYieldCurrentTask = pdFALSE;
34503473

34513474
/* Get the xYieldPending stats inside the critical section. */
3452-
if( pxCurrentTCBs[ xCoreID ]->uxPreemptionDisable == 0U )
3475+
if( ( pxCurrentTCBs[ xCoreID ]->uxPreemptionDisable == 0U ) &&
3476+
( pxCurrentTCBs[ xCoreID ]->uxDeferredStateChange == 0U ) )
34533477
{
34543478
xYieldCurrentTask = xYieldPendings[ xCoreID ];
34553479
}
@@ -3518,7 +3542,7 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
35183542
BaseType_t prvTaskPreemptionEnable( const TaskHandle_t xTask )
35193543
{
35203544
TCB_t * pxTCB;
3521-
UBaseType_t uxDeferredAction = 0U;
3545+
BaseType_t xHasDeferredAction = pdFALSE;
35223546
BaseType_t xAlreadyYielded = pdFALSE;
35233547
BaseType_t xTaskRequestedToYield = pdFALSE;
35243548

@@ -3542,7 +3566,17 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
35423566
{
35433567
if( pxTCB->uxDeferredStateChange != 0U )
35443568
{
3545-
uxDeferredAction = pxTCB->uxDeferredStateChange;
3569+
/* A deferred state change is pending. Leave uxDeferredStateChange
3570+
* non-zero — it acts as a yield-suppression flag. All yield paths
3571+
* (vTaskTCBExitCritical, vTaskSwitchContext, prvYieldCore,
3572+
* xTaskIncrementTick, vTaskExitCritical) check this flag and
3573+
* suppress context switches when it is set.
3574+
*
3575+
* After exiting this critical section, we will call the
3576+
* appropriate deferred action function (vTaskDelete/vTaskSuspend)
3577+
* which will enter the kernel critical section, clear the flag,
3578+
* and execute the action atomically. */
3579+
xHasDeferredAction = pdTRUE;
35463580
}
35473581
else
35483582
{
@@ -3572,44 +3606,50 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
35723606
kernelEXIT_CRITICAL();
35733607
#endif
35743608

3575-
if( uxDeferredAction != 0U )
3609+
if( xHasDeferredAction != pdFALSE )
35763610
{
3577-
if( uxDeferredAction & tskDEFERRED_DELETION )
3611+
/* At this point:
3612+
* - uxPreemptionDisable == 0 (preemption re-enabled)
3613+
* - uxDeferredStateChange != 0 (suppresses all yield paths)
3614+
* - Interrupts are enabled but no context switch can occur
3615+
*
3616+
* vTaskDelete/vTaskSuspend will:
3617+
* 1. Enter kernel critical section (recursive spinlocks allow nesting)
3618+
* 2. Acquire TCB spinlock
3619+
* 3. See uxPreemptionDisable == 0 -> take immediate path
3620+
* 4. Clear uxDeferredStateChange
3621+
* 5. Execute the action (remove from lists, add to termination/suspended)
3622+
* 6. Call taskYIELD_WITHIN_API (pends yield since critical nesting > 0)
3623+
* 7. Exit kernel critical section -> yield occurs */
3624+
if( pxTCB->uxDeferredStateChange & tskDEFERRED_DELETION )
35783625
{
35793626
vTaskDelete( xTask );
35803627
}
3581-
else if( uxDeferredAction & tskDEFERRED_SUSPENSION )
3628+
else if( pxTCB->uxDeferredStateChange & tskDEFERRED_SUSPENSION )
35823629
{
35833630
vTaskSuspend( xTask );
35843631
}
3585-
else
3586-
{
3587-
mtCOVERAGE_TEST_MARKER();
3588-
}
35893632

35903633
/* Any deferred action on the task would result in a context switch. */
35913634
xAlreadyYielded = pdTRUE;
35923635
}
3593-
else
3636+
else if( xTaskRequestedToYield != pdFALSE )
35943637
{
3595-
if( xTaskRequestedToYield != pdFALSE )
3638+
/* prvYieldCore must be called in critical section. */
3639+
kernelENTER_CRITICAL();
35963640
{
3597-
/* prvYieldCore must be called in critical section. */
3598-
kernelENTER_CRITICAL();
3599-
{
3600-
pxTCB = prvGetTCBFromHandle( xTask );
3641+
pxTCB = prvGetTCBFromHandle( xTask );
36013642

3602-
/* There is gap between TCB critical section and kernel critical section.
3603-
* Checking the yield pending again to prevent that the current task
3604-
* already handle the yield request. */
3605-
if( ( xYieldPendings[ pxTCB->xTaskRunState ] != pdFALSE ) && ( taskTASK_IS_RUNNING( pxTCB ) != pdFALSE ) )
3606-
{
3607-
prvYieldCore( pxTCB->xTaskRunState );
3608-
}
3643+
/* There is gap between TCB critical section and kernel critical section.
3644+
* Checking the yield pending again to prevent that the current task
3645+
* already handle the yield request. */
3646+
if( ( xYieldPendings[ pxTCB->xTaskRunState ] != pdFALSE ) && ( taskTASK_IS_RUNNING( pxTCB ) != pdFALSE ) )
3647+
{
3648+
prvYieldCore( pxTCB->xTaskRunState );
36093649
}
3610-
kernelEXIT_CRITICAL();
3611-
xAlreadyYielded = pdTRUE;
36123650
}
3651+
kernelEXIT_CRITICAL();
3652+
xAlreadyYielded = pdTRUE;
36133653
}
36143654

36153655
return xAlreadyYielded;
@@ -3652,18 +3692,41 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
36523692

36533693
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
36543694

3695+
/* When TCB data group lock is enabled, we must acquire the target TCB's spinlock
3696+
* to prevent a race condition with prvTaskPreemptionEnable(). */
3697+
#if ( configUSE_TCB_DATA_GROUP_LOCK == 1 )
3698+
{
3699+
const BaseType_t xCoreID = ( BaseType_t ) portGET_CORE_ID();
3700+
portGET_SPINLOCK( xCoreID, &pxTCB->xTCBSpinlock );
3701+
}
3702+
#endif /* configUSE_TCB_DATA_GROUP_LOCK */
3703+
36553704
/* If the task has disabled preemption, we need to defer the suspension until the
3656-
* task enables preemption. The suspension will be performed in vTaskPreemptionEnable(). */
3705+
* task enables preemption. The suspension will be performed in prvTaskPreemptionEnable(). */
36573706
if( pxTCB->uxPreemptionDisable > 0U )
36583707
{
3659-
pxTCB->uxDeferredStateChange |= tskDEFERRED_SUSPENSION;
3708+
/* Only set deferred suspension if deletion is not already deferred.
3709+
* Deletion takes priority over suspension - if both are requested,
3710+
* only deletion will be processed. */
3711+
if( ( pxTCB->uxDeferredStateChange & tskDEFERRED_DELETION ) == 0U )
3712+
{
3713+
pxTCB->uxDeferredStateChange |= tskDEFERRED_SUSPENSION;
3714+
}
3715+
36603716
xDeferredSuspension = pdTRUE;
36613717
}
36623718
else
36633719
{
36643720
/* Reset the deferred state change flags */
36653721
pxTCB->uxDeferredStateChange &= ~tskDEFERRED_SUSPENSION;
36663722
}
3723+
3724+
#if ( configUSE_TCB_DATA_GROUP_LOCK == 1 )
3725+
{
3726+
const BaseType_t xCoreID = ( BaseType_t ) portGET_CORE_ID();
3727+
portRELEASE_SPINLOCK( xCoreID, &pxTCB->xTCBSpinlock );
3728+
}
3729+
#endif /* configUSE_TCB_DATA_GROUP_LOCK */
36673730
#endif /* configUSE_TASK_PREEMPTION_DISABLE */
36683731

36693732
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
@@ -5488,7 +5551,8 @@ BaseType_t xTaskIncrementTick( void )
54885551
for( xCoreID = 0; xCoreID < ( BaseType_t ) configNUMBER_OF_CORES; xCoreID++ )
54895552
{
54905553
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
5491-
if( pxCurrentTCBs[ xCoreID ]->uxPreemptionDisable == 0U )
5554+
if( ( pxCurrentTCBs[ xCoreID ]->uxPreemptionDisable == 0U ) &&
5555+
( pxCurrentTCBs[ xCoreID ]->uxDeferredStateChange == 0U ) )
54925556
#endif
54935557
{
54945558
if( xYieldPendings[ xCoreID ] != pdFALSE )
@@ -5788,6 +5852,17 @@ BaseType_t xTaskIncrementTick( void )
57885852
* a context switch. */
57895853
xYieldPendings[ xCoreID ] = pdTRUE;
57905854
}
5855+
5856+
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
5857+
else if( pxCurrentTCBs[ xCoreID ]->uxDeferredStateChange != 0U )
5858+
{
5859+
/* A deferred state change (deletion or suspension) is pending
5860+
* for the current task. The task must execute this deferred
5861+
* action before being switched out. Pend the yield so it will
5862+
* be processed after the deferred action completes. */
5863+
xYieldPendings[ xCoreID ] = pdTRUE;
5864+
}
5865+
#endif /* configUSE_TASK_PREEMPTION_DISABLE */
57915866
else
57925867
{
57935868
xYieldPendings[ xCoreID ] = pdFALSE;

0 commit comments

Comments
 (0)