From e6541436243816aaf3ff40cbaf945ccc52c7557f Mon Sep 17 00:00:00 2001 From: Jacob Carver Date: Fri, 13 Oct 2023 12:40:48 +0530 Subject: [PATCH 1/4] Fix task notification determinism. --- tasks.c | 92 +++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 64 insertions(+), 28 deletions(-) diff --git a/tasks.c b/tasks.c index 2cd130a8297..9c3d650223c 100644 --- a/tasks.c +++ b/tasks.c @@ -5127,7 +5127,7 @@ void vTaskPlaceOnEventList( List_t * const pxEventList, configASSERT( pxEventList ); - /* THIS FUNCTION MUST BE CALLED WITH EITHER INTERRUPTS DISABLED OR THE + /* THIS FUNCTION MUST BE CALLED WITH THE * SCHEDULER SUSPENDED AND THE QUEUE BEING ACCESSED LOCKED. */ /* Place the event list item of the TCB in the appropriate event list. @@ -7415,7 +7415,6 @@ TickType_t uxTaskResetEventItemValue( void ) configASSERT( uxIndexToWaitOn < configTASK_NOTIFICATION_ARRAY_ENTRIES ); taskENTER_CRITICAL(); - { /* Only block if the notification count is not already non-zero. */ if( pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] == 0UL ) { @@ -7427,11 +7426,27 @@ TickType_t uxTaskResetEventItemValue( void ) prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); traceTASK_NOTIFY_TAKE_BLOCK( uxIndexToWaitOn ); - /* All ports are written to allow a yield in a critical - * section (some will yield immediately, others wait until the - * critical section exits) - but it is not something that - * application code should ever do. */ - taskYIELD_WITHIN_API(); + /* Suspend the scheduler before enabling interrupts. */ + vTaskSuspendAll(); + taskEXIT_CRITICAL(); + + prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); + + /* All ports are written to allow a yield in a critical + * section (some will yield immediately, others wait until the + * critical section exits) - but it is not something that + * application code should ever do. */ + if( xTaskResumeAll() == pdFALSE ) + { + #if ( configNUMBER_OF_CORES == 1 ) + { + portYIELD_WITHIN_API(); + } + #else + { + vTaskYieldWithinAPI(); + } + #endif } else { @@ -7440,10 +7455,13 @@ TickType_t uxTaskResetEventItemValue( void ) } else { - mtCOVERAGE_TEST_MARKER(); + taskEXIT_CRITICAL(); } } - taskEXIT_CRITICAL(); + else + { + taskEXIT_CRITICAL(); + } taskENTER_CRITICAL(); { @@ -7493,28 +7511,43 @@ TickType_t uxTaskResetEventItemValue( void ) configASSERT( uxIndexToWaitOn < configTASK_NOTIFICATION_ARRAY_ENTRIES ); taskENTER_CRITICAL(); + + /* Only block if a notification is not already pending. */ + if( pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] != taskNOTIFICATION_RECEIVED ) { - /* Only block if a notification is not already pending. */ - if( pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] != taskNOTIFICATION_RECEIVED ) + /* Clear bits in the task's notification value as bits may get + * set by the notifying task or interrupt. This can be used to + * clear the value to zero. */ + pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] &= ~ulBitsToClearOnEntry; + + /* Mark this task as waiting for a notification. */ + pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] = taskWAITING_NOTIFICATION; + + if( xTicksToWait > ( TickType_t ) 0 ) { - /* Clear bits in the task's notification value as bits may get - * set by the notifying task or interrupt. This can be used to - * clear the value to zero. */ - pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] &= ~ulBitsToClearOnEntry; + traceTASK_NOTIFY_WAIT_BLOCK( uxIndexToWaitOn ); - /* Mark this task as waiting for a notification. */ - pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] = taskWAITING_NOTIFICATION; + /* Suspend the scheduler before enabling interrupts. */ + vTaskSuspendAll(); + taskEXIT_CRITICAL(); - if( xTicksToWait > ( TickType_t ) 0 ) - { - prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); - traceTASK_NOTIFY_WAIT_BLOCK( uxIndexToWaitOn ); + prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); - /* All ports are written to allow a yield in a critical - * section (some will yield immediately, others wait until the - * critical section exits) - but it is not something that - * application code should ever do. */ - taskYIELD_WITHIN_API(); + /* All ports are written to allow a yield in a critical + * section (some will yield immediately, others wait until the + * critical section exits) - but it is not something that + * application code should ever do. */ + if( xTaskResumeAll() == pdFALSE ) + { + #if ( configNUMBER_OF_CORES == 1 ) + { + portYIELD_WITHIN_API(); + } + #else + { + vTaskYieldWithinAPI(); + } + #endif } else { @@ -7523,10 +7556,13 @@ TickType_t uxTaskResetEventItemValue( void ) } else { - mtCOVERAGE_TEST_MARKER(); + taskEXIT_CRITICAL(); } } - taskEXIT_CRITICAL(); + else + { + taskEXIT_CRITICAL(); + } taskENTER_CRITICAL(); { From 1043460537ae0174440da82e98ddaa840653cdcd Mon Sep 17 00:00:00 2001 From: Jacob Carver Date: Fri, 13 Oct 2023 07:35:34 +0000 Subject: [PATCH 2/4] Fix formatting. --- tasks.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tasks.c b/tasks.c index 9c3d650223c..6400ca11447 100644 --- a/tasks.c +++ b/tasks.c @@ -7415,16 +7415,17 @@ TickType_t uxTaskResetEventItemValue( void ) configASSERT( uxIndexToWaitOn < configTASK_NOTIFICATION_ARRAY_ENTRIES ); taskENTER_CRITICAL(); - /* Only block if the notification count is not already non-zero. */ - if( pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] == 0UL ) - { - /* Mark this task as waiting for a notification. */ - pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] = taskWAITING_NOTIFICATION; - if( xTicksToWait > ( TickType_t ) 0 ) - { - prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); - traceTASK_NOTIFY_TAKE_BLOCK( uxIndexToWaitOn ); + /* Only block if the notification count is not already non-zero. */ + if( pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] == 0UL ) + { + /* Mark this task as waiting for a notification. */ + pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] = taskWAITING_NOTIFICATION; + + if( xTicksToWait > ( TickType_t ) 0 ) + { + prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); + traceTASK_NOTIFY_TAKE_BLOCK( uxIndexToWaitOn ); /* Suspend the scheduler before enabling interrupts. */ vTaskSuspendAll(); From acbb51b003417c16a7483f2d982b247668a87b54 Mon Sep 17 00:00:00 2001 From: Rahul Kar Date: Fri, 13 Oct 2023 12:43:19 +0000 Subject: [PATCH 3/4] Remove duplicate statement --- tasks.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tasks.c b/tasks.c index 6400ca11447..8c36569cad7 100644 --- a/tasks.c +++ b/tasks.c @@ -7424,7 +7424,6 @@ TickType_t uxTaskResetEventItemValue( void ) if( xTicksToWait > ( TickType_t ) 0 ) { - prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); traceTASK_NOTIFY_TAKE_BLOCK( uxIndexToWaitOn ); /* Suspend the scheduler before enabling interrupts. */ From 3df0cba839d203bd683ec99012724d569bd76a9d Mon Sep 17 00:00:00 2001 From: Gaurav Aggarwal Date: Mon, 16 Oct 2023 20:38:03 +0530 Subject: [PATCH 4/4] Code review suggestions Signed-off-by: Gaurav Aggarwal --- tasks.c | 119 +++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 84 insertions(+), 35 deletions(-) diff --git a/tasks.c b/tasks.c index 8c36569cad7..4339c04e223 100644 --- a/tasks.c +++ b/tasks.c @@ -7409,6 +7409,7 @@ TickType_t uxTaskResetEventItemValue( void ) TickType_t xTicksToWait ) { uint32_t ulReturn; + BaseType_t xAlreadyYielded; traceENTER_ulTaskGenericNotifyTake( uxIndexToWaitOn, xClearCountOnExit, xTicksToWait ); @@ -7426,27 +7427,51 @@ TickType_t uxTaskResetEventItemValue( void ) { traceTASK_NOTIFY_TAKE_BLOCK( uxIndexToWaitOn ); - /* Suspend the scheduler before enabling interrupts. */ + /* We MUST suspend 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. + */ vTaskSuspendAll(); - taskEXIT_CRITICAL(); + { + taskEXIT_CRITICAL(); - prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); + prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); + } + xAlreadyYielded = xTaskResumeAll(); - /* All ports are written to allow a yield in a critical - * section (some will yield immediately, others wait until the - * critical section exits) - but it is not something that - * application code should ever do. */ - if( xTaskResumeAll() == pdFALSE ) + if( xAlreadyYielded == pdFALSE ) { - #if ( configNUMBER_OF_CORES == 1 ) - { - portYIELD_WITHIN_API(); - } - #else - { - vTaskYieldWithinAPI(); - } - #endif + taskYIELD_WITHIN_API(); } else { @@ -7504,7 +7529,7 @@ TickType_t uxTaskResetEventItemValue( void ) uint32_t * pulNotificationValue, TickType_t xTicksToWait ) { - BaseType_t xReturn; + BaseType_t xReturn, xAlreadyYielded; traceENTER_xTaskGenericNotifyWait( uxIndexToWaitOn, ulBitsToClearOnEntry, ulBitsToClearOnExit, pulNotificationValue, xTicksToWait ); @@ -7527,27 +7552,51 @@ TickType_t uxTaskResetEventItemValue( void ) { traceTASK_NOTIFY_WAIT_BLOCK( uxIndexToWaitOn ); - /* Suspend the scheduler before enabling interrupts. */ + /* We MUST suspend 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. + */ vTaskSuspendAll(); - taskEXIT_CRITICAL(); + { + taskEXIT_CRITICAL(); - prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); + prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE ); + } + xAlreadyYielded = xTaskResumeAll(); - /* All ports are written to allow a yield in a critical - * section (some will yield immediately, others wait until the - * critical section exits) - but it is not something that - * application code should ever do. */ - if( xTaskResumeAll() == pdFALSE ) + if( xAlreadyYielded == pdFALSE ) { - #if ( configNUMBER_OF_CORES == 1 ) - { - portYIELD_WITHIN_API(); - } - #else - { - vTaskYieldWithinAPI(); - } - #endif + taskYIELD_WITHIN_API(); } else {