From ba47502c1477dd8e5387e67c7eb5f30f3e8d6144 Mon Sep 17 00:00:00 2001 From: Ching-Hsin Lee Date: Tue, 6 Feb 2024 17:12:15 +0800 Subject: [PATCH 1/5] Request task to yield after been suspended or deleted * Request a task to yield after been suspended or deleted to prevent this task puts itself back to another list --- tasks.c | 163 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 88 insertions(+), 75 deletions(-) diff --git a/tasks.c b/tasks.c index b0af5ffbe62..2731c95fb6e 100644 --- a/tasks.c +++ b/tasks.c @@ -2271,6 +2271,32 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, * the task that has just been deleted. */ prvResetNextTaskUnblockTime(); } + + /* It is important that to request the deleted task to yield before leaving + * the critical section. The deleted task may be blocked at the entry + * of critical section or scheduler suspension. Without requesting this + * task to yield, this task may void the task deletion by putting itself + * back to another list. */ + #if ( configNUMBER_OF_CORES > 1 ) + { + if( xSchedulerRunning != pdFALSE ) + { + if( taskTASK_IS_RUNNING( pxTCB ) == pdTRUE ) + { + if( pxTCB->xTaskRunState == ( BaseType_t ) portGET_CORE_ID() ) + { + configASSERT( uxSchedulerSuspended == 0 ); + taskYIELD_WITHIN_API(); + } + else + { + prvYieldCore( pxTCB->xTaskRunState ); + } + } + } + } + #endif /* #if ( configNUMBER_OF_CORES > 1 ) */ + } taskEXIT_CRITICAL(); @@ -2284,9 +2310,9 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, /* Force a reschedule if it is the currently running task that has just * been deleted. */ - if( xSchedulerRunning != pdFALSE ) + #if ( configNUMBER_OF_CORES == 1 ) { - #if ( configNUMBER_OF_CORES == 1 ) + if( xSchedulerRunning != pdFALSE ) { if( pxTCB == pxCurrentTCB ) { @@ -2298,30 +2324,8 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, mtCOVERAGE_TEST_MARKER(); } } - #else /* #if ( configNUMBER_OF_CORES == 1 ) */ - { - /* It is important to use critical section here because - * checking run state of a task must be done inside a - * critical section. */ - taskENTER_CRITICAL(); - { - if( taskTASK_IS_RUNNING( pxTCB ) == pdTRUE ) - { - if( pxTCB->xTaskRunState == ( BaseType_t ) portGET_CORE_ID() ) - { - configASSERT( uxSchedulerSuspended == 0 ); - taskYIELD_WITHIN_API(); - } - else - { - prvYieldCore( pxTCB->xTaskRunState ); - } - } - } - taskEXIT_CRITICAL(); - } - #endif /* #if ( configNUMBER_OF_CORES == 1 ) */ } + #endif /* #if ( configNUMBER_OF_CORES == 1 ) */ traceRETURN_vTaskDelete(); } @@ -3155,26 +3159,72 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, } } #endif /* if ( configUSE_TASK_NOTIFICATIONS == 1 ) */ - } - taskEXIT_CRITICAL(); - if( xSchedulerRunning != pdFALSE ) - { - /* Reset the next expected unblock time in case it referred to the - * task that is now in the Suspended state. */ - taskENTER_CRITICAL(); + /* It is important that to request the suspended task yield before leaving + * the critical section. The suspended task may be blocked at the entry + * of critical section or scheduler suspension. Without requesting this + * task to yield, this task may void the task suspension by putting itself + * back to another list. */ + #if ( configNUMBER_OF_CORES > 1 ) { - prvResetNextTaskUnblockTime(); + if( xSchedulerRunning != pdFALSE ) + { + /* Reset the next expected unblock time in case it referred to the + * task that is now in the Suspended state. */ + prvResetNextTaskUnblockTime(); + } + + if( taskTASK_IS_RUNNING( pxTCB ) == pdTRUE ) + { + if( xSchedulerRunning != pdFALSE ) + { + if( pxTCB->xTaskRunState == ( BaseType_t ) portGET_CORE_ID() ) + { + /* The current task has just been suspended. */ + configASSERT( uxSchedulerSuspended == 0 ); + vTaskYieldWithinAPI(); + } + else + { + prvYieldCore( pxTCB->xTaskRunState ); + } + } + else + { + /* This code path is not possible because only Idle tasks are + * assigned a core before the scheduler is started ( i.e. + * taskTASK_IS_RUNNING is only true for idle tasks before + * the scheduler is started ) and idle tasks cannot be + * suspended. */ + mtCOVERAGE_TEST_MARKER(); + } + } + else + { + mtCOVERAGE_TEST_MARKER(); + } } - taskEXIT_CRITICAL(); - } - else - { - mtCOVERAGE_TEST_MARKER(); + #endif /* #if ( configNUMBER_OF_CORES > 1 ) */ } + taskEXIT_CRITICAL(); #if ( configNUMBER_OF_CORES == 1 ) { + if( xSchedulerRunning != pdFALSE ) + { + /* Reset the next expected unblock time in case it referred to the + * task that is now in the Suspended state. */ + taskENTER_CRITICAL(); + { + prvResetNextTaskUnblockTime(); + } + taskEXIT_CRITICAL(); + } + else + { + mtCOVERAGE_TEST_MARKER(); + } + if( pxTCB == pxCurrentTCB ) { if( xSchedulerRunning != pdFALSE ) @@ -3207,43 +3257,6 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, mtCOVERAGE_TEST_MARKER(); } } - #else /* #if ( configNUMBER_OF_CORES == 1 ) */ - { - /* Enter critical section here to check run state of a task. */ - taskENTER_CRITICAL(); - { - if( taskTASK_IS_RUNNING( pxTCB ) == pdTRUE ) - { - if( xSchedulerRunning != pdFALSE ) - { - if( pxTCB->xTaskRunState == ( BaseType_t ) portGET_CORE_ID() ) - { - /* The current task has just been suspended. */ - configASSERT( uxSchedulerSuspended == 0 ); - vTaskYieldWithinAPI(); - } - else - { - prvYieldCore( pxTCB->xTaskRunState ); - } - } - else - { - /* This code path is not possible because only Idle tasks are - * assigned a core before the scheduler is started ( i.e. - * taskTASK_IS_RUNNING is only true for idle tasks before - * the scheduler is started ) and idle tasks cannot be - * suspended. */ - mtCOVERAGE_TEST_MARKER(); - } - } - else - { - mtCOVERAGE_TEST_MARKER(); - } - } - taskEXIT_CRITICAL(); - } #endif /* #if ( configNUMBER_OF_CORES == 1 ) */ traceRETURN_vTaskSuspend(); From da4f2a335d41461f37bb8219f8adae76e340fd27 Mon Sep 17 00:00:00 2001 From: Ching-Hsin Lee Date: Tue, 6 Feb 2024 17:57:37 +0800 Subject: [PATCH 2/5] Fix volatile variable access order --- tasks.c | 48 +++++++++++++++++++----------------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/tasks.c b/tasks.c index 2731c95fb6e..9d2578a1428 100644 --- a/tasks.c +++ b/tasks.c @@ -2191,6 +2191,7 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, { TCB_t * pxTCB; BaseType_t xDeleteTCBInIdleTask = pdFALSE; + BaseType_t xTaskIsRunningOrYielding; traceENTER_vTaskDelete( xTaskToDelete ); @@ -2229,7 +2230,8 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, /* If the task is running (or yielding), we must add it to the * termination list so that an idle task can delete it when it is * no longer running. */ - if( ( xSchedulerRunning != pdFALSE ) && ( taskTASK_IS_RUNNING_OR_SCHEDULED_TO_YIELD( pxTCB ) != pdFALSE ) ) + xTaskIsRunningOrYielding = taskTASK_IS_RUNNING_OR_SCHEDULED_TO_YIELD( pxTCB ); + if( ( xSchedulerRunning != pdFALSE ) && ( xTaskIsRunningOrYielding != pdFALSE ) ) { /* A running task or a task which is scheduled to yield is being * deleted. This cannot complete when the task is still running @@ -2261,25 +2263,13 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, #else portPRE_TASK_DELETE_HOOK( pxTCB, &( xYieldPendings[ pxTCB->xTaskRunState ] ) ); #endif - } - else - { - --uxCurrentNumberOfTasks; - traceTASK_DELETE( pxTCB ); - /* Reset the next expected unblock time in case it referred to - * the task that has just been deleted. */ - prvResetNextTaskUnblockTime(); - } - - /* It is important that to request the deleted task to yield before leaving - * the critical section. The deleted task may be blocked at the entry - * of critical section or scheduler suspension. Without requesting this - * task to yield, this task may void the task deletion by putting itself - * back to another list. */ - #if ( configNUMBER_OF_CORES > 1 ) - { - if( xSchedulerRunning != pdFALSE ) + /* It is important that to request the deleted task to yield before leaving + * the critical section. The deleted task may be blocked at the entry + * of critical section or scheduler suspension. Without requesting this + * task to yield, this task may void the task deletion by putting itself + * back to another list. */ + #if ( configNUMBER_OF_CORES > 1 ) { if( taskTASK_IS_RUNNING( pxTCB ) == pdTRUE ) { @@ -2294,9 +2284,17 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, } } } + #endif /* #if ( configNUMBER_OF_CORES > 1 ) */ } - #endif /* #if ( configNUMBER_OF_CORES > 1 ) */ + else + { + --uxCurrentNumberOfTasks; + traceTASK_DELETE( pxTCB ); + /* Reset the next expected unblock time in case it referred to + * the task that has just been deleted. */ + prvResetNextTaskUnblockTime(); + } } taskEXIT_CRITICAL(); @@ -3172,11 +3170,8 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, /* Reset the next expected unblock time in case it referred to the * task that is now in the Suspended state. */ prvResetNextTaskUnblockTime(); - } - if( taskTASK_IS_RUNNING( pxTCB ) == pdTRUE ) - { - if( xSchedulerRunning != pdFALSE ) + if( taskTASK_IS_RUNNING( pxTCB ) == pdTRUE ) { if( pxTCB->xTaskRunState == ( BaseType_t ) portGET_CORE_ID() ) { @@ -3191,11 +3186,6 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, } else { - /* This code path is not possible because only Idle tasks are - * assigned a core before the scheduler is started ( i.e. - * taskTASK_IS_RUNNING is only true for idle tasks before - * the scheduler is started ) and idle tasks cannot be - * suspended. */ mtCOVERAGE_TEST_MARKER(); } } From 5ed50e50b63951932595f2806387a5ed562d0ece Mon Sep 17 00:00:00 2001 From: Ching-Hsin Lee Date: Tue, 6 Feb 2024 18:33:17 +0800 Subject: [PATCH 3/5] Fix uncrustify --- tasks.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tasks.c b/tasks.c index 9d2578a1428..6fe5ec27f3b 100644 --- a/tasks.c +++ b/tasks.c @@ -2231,6 +2231,7 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, * termination list so that an idle task can delete it when it is * no longer running. */ xTaskIsRunningOrYielding = taskTASK_IS_RUNNING_OR_SCHEDULED_TO_YIELD( pxTCB ); + if( ( xSchedulerRunning != pdFALSE ) && ( xTaskIsRunningOrYielding != pdFALSE ) ) { /* A running task or a task which is scheduled to yield is being From 68a172c2e05e326e769b9f088955adf265393650 Mon Sep 17 00:00:00 2001 From: Ching-Hsin Lee Date: Tue, 6 Feb 2024 19:14:06 +0800 Subject: [PATCH 4/5] Add more comment to fix MISRA --- tasks.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tasks.c b/tasks.c index 9d2578a1428..ba9e9c328c4 100644 --- a/tasks.c +++ b/tasks.c @@ -2227,10 +2227,14 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, * not return. */ uxTaskNumber++; + /* Use temp variable as distinct sequence points for reading volatile + * variables prior to a logical operator to ensure compliance with + * MISRA C 2012 Rule 13.5. */ + xTaskIsRunningOrYielding = taskTASK_IS_RUNNING_OR_SCHEDULED_TO_YIELD( pxTCB ); + /* If the task is running (or yielding), we must add it to the * termination list so that an idle task can delete it when it is * no longer running. */ - xTaskIsRunningOrYielding = taskTASK_IS_RUNNING_OR_SCHEDULED_TO_YIELD( pxTCB ); if( ( xSchedulerRunning != pdFALSE ) && ( xTaskIsRunningOrYielding != pdFALSE ) ) { /* A running task or a task which is scheduled to yield is being From 6907d1d62243c9541ff0cbb5ac1ca7d66ebab6ff Mon Sep 17 00:00:00 2001 From: Gaurav Aggarwal Date: Tue, 6 Feb 2024 11:59:06 +0000 Subject: [PATCH 5/5] Update comments Signed-off-by: Gaurav Aggarwal --- tasks.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/tasks.c b/tasks.c index ba9e9c328c4..5cda6ec85ad 100644 --- a/tasks.c +++ b/tasks.c @@ -2268,11 +2268,13 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, portPRE_TASK_DELETE_HOOK( pxTCB, &( xYieldPendings[ pxTCB->xTaskRunState ] ) ); #endif - /* It is important that to request the deleted task to yield before leaving - * the critical section. The deleted task may be blocked at the entry - * of critical section or scheduler suspension. Without requesting this - * task to yield, this task may void the task deletion by putting itself - * back to another list. */ + /* In the case of SMP, it is possible that the task being deleted + * is running on another core. We must evict the task before + * exiting the critical section to ensure that the task cannot + * take an action which puts it back on ready/state/event list, + * thereby nullifying the delete operation. Once evicted, the + * task won't be scheduled ever as it will no longer be on the + * ready list. */ #if ( configNUMBER_OF_CORES > 1 ) { if( taskTASK_IS_RUNNING( pxTCB ) == pdTRUE ) @@ -3162,11 +3164,13 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, } #endif /* if ( configUSE_TASK_NOTIFICATIONS == 1 ) */ - /* It is important that to request the suspended task yield before leaving - * the critical section. The suspended task may be blocked at the entry - * of critical section or scheduler suspension. Without requesting this - * task to yield, this task may void the task suspension by putting itself - * back to another list. */ + /* In the case of SMP, it is possible that the task being suspended + * is running on another core. We must evict the task before + * exiting the critical section to ensure that the task cannot + * take an action which puts it back on ready/state/event list, + * thereby nullifying the suspend operation. Once evicted, the + * task won't be scheduled before it is resumed as it will no longer + * be on the ready list. */ #if ( configNUMBER_OF_CORES > 1 ) { if( xSchedulerRunning != pdFALSE )