Update the uxSchedulerSuspended after prvCheckForRunStateChange

[chinglee-iot]

Description

• Update the uxSchedulerSuspended after the prvCheckForRunStateChange to prevent race condition in fromISR APIs

Consider the following sequence:

• Core A tries to suspend the scheduler and are requested to yield ( Task lock is held )
• Core B enters the critical section From ISR ( ISR lock is held ) and reading the uxSchedulerSuspended

The original implementation is to update the uxSchedulerSuspended without ISR lock.
FromISR version API can still read the uxSchedulerSuspended when task is updating the variable.
The problem in xTaskRemoveFromEventList

• After uxSchedulerSuspended is udpated to 0, the task can be put to the ready list directly.
• Before uxSchedulerSuspended is updated to 0, the task will be put to the pending ready list and added back to the ready list when scheduler is resumed. However, if the task is requested to yield. The task in pending ready list will be blocked. This can be reproduced easily with proper delay.

            uxSchedulerSuspended = 0U;

            if( uxPrevCriticalNesting > 0U )
            {
                pxThisTCB->uxCriticalNesting = 0U;
                portRELEASE_ISR_LOCK();
                portRELEASE_TASK_LOCK();
            }
            else
            {
                /* uxPrevSchedulerSuspended must be 1 */
                portRELEASE_TASK_LOCK();
            }

The fix in ab074f5. This ensure that the uxSchedulerSuspended is always 1 in this scenario since portGET_ISR_LOCK ensure the order, which is the before case in previous section and is not expected.

                if( uxPrevCriticalNesting > 0U )
                {
                    portSET_CRITICAL_NESTING_COUNT( 0U );
                }
                else
                {
                    /* Adding proper delay here to increase the probability ISR get the lock 
                     * first to reproduce the problem. In that case, uxSchedulerSuspended is 1
                     * in the from ISR API. */
                    portGET_ISR_LOCK();
                    uxSchedulerSuspended = 0U;
                }

                portRELEASE_ISR_LOCK();
                portRELEASE_TASK_LOCK();

The suggested change in the PR. The uxSchedulerSuspended is update only when the task is not requested to yield. This prevent the race condition.

            /* uxSchedulerSuspended is increased after prvCheckForRunStateChange. The
             * purpose is to prevent fromISR API read the uxSchedulerSuspended if the
             * task is requested to yield. */
            if( uxSchedulerSuspended == 0U )
            {
                if( portGET_CRITICAL_NESTING_COUNT() == 0U )
                {
                    prvCheckForRunStateChange();
                }
                else
                {
                    mtCOVERAGE_TEST_MARKER();
                }
            }
            else
            {
                mtCOVERAGE_TEST_MARKER();
            }

            portGET_ISR_LOCK();

            /* The scheduler is suspended if uxSchedulerSuspended is non-zero. An increment
             * is used to allow calls to vTaskSuspendAll() to nest. */
            ++uxSchedulerSuspended;
            portRELEASE_ISR_LOCK();

            portCLEAR_INTERRUPT_MASK( ulState );

Test Steps

Related Issue

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

[chinglee-iot]

The problem ( the before case )

[chinglee-iot]

The suggested fix in this PR

[chinglee-iot]

This problem can be observed when running demos on XMOS AICORE.
Without this patch, there will be the following assertion

xrun: Program received signal ET_ECALL, Application exception.
      [Switching to tile[0] core[6]]
      prvSyncTask (pvParameters=0x8) at ../../../../Common/Minimal\EventGroupsDemo.c:304

      304               configASSERT( ( uxReturned & ebALL_SYNC_BITS ) != ebALL_SYNC_BITS );
      Current language:  auto; currently minimal
make: *** [run] Error 125