[BUG] Scheduling stop in the prvCheckForRunStateChange function for SMP

[raceant]

Describe the bug
Running on the SMP system, the system stops scheduling, and an assertion is triggered at configASSERT( pxThisTCB->xTaskRunState == taskTASK_SCHEDULED_TO_YIELD ) in the code of the prvCheckForRunStateChange function.

In the vTaskSuspendAll function, the portSET_INTERRUPT_MASK() macro is used to disable interrupts before calling prvCheckForRunStateChange. However, the prvCheckForRunStateChange function code uses the portENABLE_INTERRUPTS() macro to temporarily enable task switching, which results in task switching not actually being enabled. After calling portENABLE_INTERRUPTS(), the interrupts used for scheduling remain masked.

Target

• Development board: [user-defined]
• Instruction Set Architecture: [ARM_CR82]
• IDE and version: [IAR 9.20]

To Reproduce

• Use multi-task with xSemaphoreXXX function on multi-core system.
• Frequent calls to the vTaskSuspendAll function
• Frequent calls to the prvYieldCore( xCoreID ) macro

Additional context
It might be appropriate to use the portDISABLE_INTERRUPTS() macro instead of the portSET_INTERRUPT_MASK() macro in the vTaskSuspendAll function.

[raceant]

Attached is the relevant code

FreeRTOS-Kernel/tasks.c

Lines 3896 to 3933 in e5987bb

	ulState = portSET_INTERRUPT_MASK();
	xCoreID = ( BaseType_t ) portGET_CORE_ID();
	/* This must never be called from inside a critical section. */
	configASSERT( portGET_CRITICAL_NESTING_COUNT( xCoreID ) == 0 );
	/* portSOFTWARE_BARRIER() is only implemented for emulated/simulated ports that
	* do not otherwise exhibit real time behaviour. */
	portSOFTWARE_BARRIER();
	portGET_TASK_LOCK( xCoreID );
	/* uxSchedulerSuspended is increased after prvCheckForRunStateChange. The
	* purpose is to prevent altering the variable when fromISR APIs are readying
	* it. */
	if( uxSchedulerSuspended == 0U )
	{
	prvCheckForRunStateChange();
	}
	else
	{
	mtCOVERAGE_TEST_MARKER();
	}
	/* Query the coreID again as prvCheckForRunStateChange may have
	* caused the task to get scheduled on a different core. The correct
	* task lock for the core is acquired in prvCheckForRunStateChange. */
	xCoreID = ( BaseType_t ) portGET_CORE_ID();
	portGET_ISR_LOCK( xCoreID );
	/* The scheduler is suspended if uxSchedulerSuspended is non-zero. An increment
	* is used to allow calls to vTaskSuspendAll() to nest. */
	++uxSchedulerSuspended;
	portRELEASE_ISR_LOCK( xCoreID );
	portCLEAR_INTERRUPT_MASK( ulState );

FreeRTOS-Kernel/tasks.c

Lines 863 to 871 in e5987bb

	portENABLE_INTERRUPTS();
	/* Enabling interrupts should cause this core to immediately service
	* the pending interrupt and yield. After servicing the pending interrupt,
	* the task needs to re-evaluate its run state within this loop, as
	* other cores may have requested this task to yield, potentially altering
	* its run state. */
	portDISABLE_INTERRUPTS();

[chinglee-iot]

@raceant
Thank you for reporting this issue.
The implementation assumes the following porting macros operate on the same interrupt mask register.

Using Cortex-M as an example:

• portDISABLE_INTERRUPTS - Disable interrupts (set PRIMASK bit to 1)

cpsid i

• portENABLE_INTERRUPTS - Enable interrupts (clear PRIMASK bit to 0)

cpsie i

• portSET_INTERRUPT_MASK - Save current PRIMASK state, then disable interrupts

mrs %0, PRIMASK    ; Read current state
cpsid i            ; Disable interrupts

Returns the saved state for later restoration.

• portCLEAR_INTERRUPT_MASK - Restore previous PRIMASK state

msr PRIMASK, %0    ; Restore saved state

This restores interrupts to their previous state (not unconditionally enabling them), which is critical for nested critical sections.

Using portDISABLE_INTERRUPTS() and portENABLE_INTERRUPTS() in vTaskSuspendAll() will need more consideration because these macros unconditionally disable and enable interrupts without considering nested critical section scenarios.

Could you please update your port to follow this behavior and let us know the result?

[raceant]

Thank you very much for your reply.
According to the modifications above, the issue was resolved after testing.

There are many platform-specific demo code, the macros portDISABLE_INTERRUPTS macro is used to set the interrupt control bits of the processor, while the macros portSET_INTERRUPT_MASK macro is used to control the mask bits of the interrupt controller.

Using ARM_AARCH64 as an example:

• portDISABLE_INTERRUPTS
  

  FreeRTOS-Kernel/portable/GCC/ARM_AARCH64/portmacro.h

  Lines 108 to 111 in e5987bb

  	#define portDISABLE_INTERRUPTS() \
  	__asm volatile ( "MSR DAIFSET, #2" ::: "memory" ); \
  	__asm volatile ( "DSB SY" ); \
  	__asm volatile ( "ISB SY" );

• portENABLE_INTERRUPTS
  

  FreeRTOS-Kernel/portable/GCC/ARM_AARCH64/portmacro.h

  Lines 113 to 116 in e5987bb

  	#define portENABLE_INTERRUPTS() \
  	__asm volatile ( "MSR DAIFCLR, #2" ::: "memory" ); \
  	__asm volatile ( "DSB SY" ); \
  	__asm volatile ( "ISB SY" );

• portSET_INTERRUPT_MASK
  

  FreeRTOS-Kernel/portable/GCC/ARM_AARCH64/port.c

  Lines 495 to 519 in e5987bb

  	UBaseType_t uxPortSetInterruptMask( void )
  	{
  	uint32_t ulReturn;
  	/* Interrupt in the CPU must be turned off while the ICCPMR is being
  	* updated. */
  	portDISABLE_INTERRUPTS();
  	if( portICCPMR_PRIORITY_MASK_REGISTER == ( uint32_t ) ( configMAX_API_CALL_INTERRUPT_PRIORITY << portPRIORITY_SHIFT ) )
  	{
  	/* Interrupts were already masked. */
  	ulReturn = pdTRUE;
  	}
  	else
  	{
  	ulReturn = pdFALSE;
  	portICCPMR_PRIORITY_MASK_REGISTER = ( uint32_t ) ( configMAX_API_CALL_INTERRUPT_PRIORITY << portPRIORITY_SHIFT );
  	__asm volatile ( "dsb sy \n"
  	"isb sy \n" ::: "memory" );
  	}
  	portENABLE_INTERRUPTS();
  	return ulReturn;
  	}

• portCLEAR_INTERRUPT_MASK
  

  FreeRTOS-Kernel/portable/GCC/ARM_AARCH64/port.c

  Lines 122 to 129 in e5987bb

  	#define portCLEAR_INTERRUPT_MASK() \
  	{ \
  	portDISABLE_INTERRUPTS(); \
  	portICCPMR_PRIORITY_MASK_REGISTER = portUNMASK_VALUE; \
  	__asm volatile ( "DSB SY \n" \
  	"ISB SY \n" ); \
  	portENABLE_INTERRUPTS(); \
  	}

Issues may arise on platforms where the two macros define the interrupt mask register differently.
It might be more suitable to support interrupt nesting.
Do you have any advice?

Looking forward to optimizing this code.
Thank you.

[chinglee-iot]

The existing portDISABLE_INTERRUPTS() and portENABLE_INTERRUPTS() macros control CPU IRQ directly via the DAIF register, which masks all interrupts including high-priority ones. This prevents high-priority interrupts from executing.

To support priority-based interrupt masking with SMP configuration and interrupt nesting, we can try to update the interrupt control macros to use the GIC's Interrupt Priority Mask Register (ICCPMR) as the primary masking mechanism, while using CPU IRQ disable (DAIF) only as a temporary lock to ensure atomic ICCPMR updates.

Renaming portDISABLE_INTERRRUPTS()/portENABLE_INTERRUPTS() macros to the following macros for disable/enable CPU-Level IRQ

#define portCPU_IRQ_DISABLE()                       \ 
     __asm volatile ( "MSR DAIFSET, #2" ::: "memory" ); \ 
     __asm volatile ( "DSB SY" );                       \ 
     __asm volatile ( "ISB SY" ); 

 #define portCPU_IRQ_ENABLE()                        \ 
     __asm volatile ( "MSR DAIFCLR, #2" ::: "memory" ); \ 
     __asm volatile ( "DSB SY" );                       \ 
     __asm volatile ( "ISB SY" );

Update the interrupt control macros

#define portICCPMR_PRIORITY_MASK \
    ((uint32_t)(configMAX_SYSCALL_INTERRUPT_PRIORITY << portPRIORITY_SHIFT))

#define portDISABLE_INTERRUPTS()                        \
    do {                                                \
        portCPU_IRQ_DISABLE();                          \
        portICCPMR_PRIORITY_MASK_REGISTER = portICCPMR_PRIORITY_MASK; \
        __asm volatile ("DSB SY" ::: "memory");         \
        __asm volatile ("ISB SY" ::: "memory");         \
        portCPU_IRQ_ENABLE();                          \
    } while(0)

#define portENABLE_INTERRUPTS()                         \
    do {                                                \
        portCPU_IRQ_DISABLE();                          \
        portICCPMR_PRIORITY_MASK_REGISTER = portUNMASK_VALUE; \
        __asm volatile ("DSB SY" ::: "memory");         \
        __asm volatile ("ISB SY" ::: "memory");         \
        portCPU_IRQ_ENABLE();                           \
    } while(0)

#define portSET_INTERRUPT_MASK()                        \
    ({                                                  \
        uint32_t __pmr;                                 \
        portCPU_IRQ_DISABLE();                          \
        __pmr = portICCPMR_PRIORITY_MASK_REGISTER;      \
        if (__pmr != portICCPMR_PRIORITY_MASK)          \
        {                                               \
            portICCPMR_PRIORITY_MASK_REGISTER = portICCPMR_PRIORITY_MASK; \
            __asm volatile ("DSB SY" ::: "memory");     \
            __asm volatile ("ISB SY" ::: "memory");     \
        }                                               \
        portCPU_IRQ_ENABLE();                           \
        __pmr;                                          \
    })

#define portCLEAR_INTERRUPT_MASK(pmr)                   \
    do {                                                \
        portCPU_IRQ_DISABLE();                          \
        portICCPMR_PRIORITY_MASK_REGISTER = (pmr);     \
        __asm volatile ("DSB SY" ::: "memory");         \
        __asm volatile ("ISB SY" ::: "memory");         \
        portCPU_IRQ_ENABLE();                           \
    } while(0)

I don't currently have access to ARM AArch64 hardware to test this implementation. If you have an ARM AArch64 platform, I would greatly appreciate your help:

• Test this implementation on your hardware and Share your results - whether successful or not
• Create a PR if it works correctly on your platform

This will enable the community experts to review the changes and verify compatibility across different platforms.

Thank you for your help and creating this issue.

[MuhammedZamroodh]

I have the similar issue in AARCH64, but we have the Macros defined differently.
For interrupt disable, we set the DAIF bit and for interrupt enable we cleared DAIF bit. And for the mask set and clear we wrote to the PMR register. See below

This was causing issues in function prvCheckForRunStateChange. The fix we made is added below as a patch.
What are your thoughts. If this makes sense in the implementation, I will raise a PR for the same.
If not please help me understand the correct method.

#define portDISABLE_INTERRUPTS()                   \
__asm volatile ( "MSR DAIFSET, #2" ::: "memory" ); \
__asm volatile ( "DSB SY" );                       \
__asm volatile ( "ISB SY" );

#define portENABLE_INTERRUPTS()                    \
__asm volatile ( "MSR DAIFCLR, #2" ::: "memory" ); \
__asm volatile ( "DSB SY" );                       \
__asm volatile ( "ISB SY" );

#define portCLEAR_INTERRUPTS()                                                              \
    {                                                                                       \
        uint64_t ulMaskBits;                                                                \
        __asm volatile ( "mrs %0, daif" : "=r" ( ulMaskBits )::"memory" );                  \
        if(!(ulMaskBits & portDAIF_I)) {                                                    \
            portDISABLE_INTERRUPTS();                                                       \
        }                                                                                   \
        __asm volatile ( "msr ICC_PMR_EL1, %0\n" : : "r" ( portUNMASK_VALUE ) : "memory" ); \
        __asm volatile ( "DSB SY        \n"                                                 \
                         "ISB SY        \n");                                               \
        if(!(ulMaskBits & portDAIF_I)) {                                                    \
            portENABLE_INTERRUPTS();                                                        \
        }                                                                                   \
    }

#define portSET_INTERRUPT_MASK()                  uxPortSetInterruptMask()
#define portCLEAR_INTERRUPT_MASK( x )             vPortClearInterruptMask( x )

UBaseType_t uxPortSetInterruptMask( void )
{
uint32_t ulReturn;

    /* Interrupt in the CPU must be turned off while the ICCPMR is being
       updated. */
    uint64_t ulMaskBits;
    __asm volatile ( "mrs %0, daif" : "=r" ( ulMaskBits )::"memory" );
    if(!(ulMaskBits & portDAIF_I)) {
        portDISABLE_INTERRUPTS();
    }

    if( ulRawReadICC_PMR_EL1() == ( uint32_t ) ( configMAX_API_CALL_INTERRUPT_PRIORITY << portPRIORITY_SHIFT ) )
    {
        /* Interrupts were already masked. */
        ulReturn = pdTRUE;
    }
    else
    {
        ulReturn = pdFALSE;
        ulRawWriteICC_PMR_EL1( ( uint32_t ) ( configMAX_API_CALL_INTERRUPT_PRIORITY << portPRIORITY_SHIFT ) );
        __asm volatile ( "dsb sy        \n"
                         "isb sy        \n"::: "memory" );
    }

    if(!(ulMaskBits & portDAIF_I)) {
        portENABLE_INTERRUPTS();
    }

    return ulReturn;
}

void vPortClearInterruptMask( UBaseType_t uxNewMaskValue )
{
    if( uxNewMaskValue == pdFALSE )
    {
        portCLEAR_INTERRUPTS();
    }
}

To fix this we added the following patch

diff --git a/tasks.c b/tasks.c
index fd3bc638d..fd2ba4ecf 100644
--- a/tasks.c
+++ b/tasks.c
@@ -537,7 +537,7 @@ static BaseType_t prvCreateIdleTasks( void );
  * Checks to see if another task moved the current task out of the ready
  * list while it was waiting to enter a critical section and yields, if so.
  */
-    static void prvCheckForRunStateChange( void );
+    static void prvCheckForRunStateChange( UBaseType_t uxSavedInterruptStatus );
 #endif /* #if ( configNUMBER_OF_CORES > 1 ) */
 
 #if ( configNUMBER_OF_CORES > 1 )
@@ -802,7 +802,7 @@ static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION;
 /*-----------------------------------------------------------*/
 
 #if ( configNUMBER_OF_CORES > 1 )
-    static void prvCheckForRunStateChange( void )
+    static void prvCheckForRunStateChange( UBaseType_t uxSavedInterruptStatus )
     {
         UBaseType_t uxPrevCriticalNesting;
         const TCB_t * pxThisTCB;
@@ -842,6 +842,7 @@ static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION;
             portMEMORY_BARRIER();
             configASSERT( pxThisTCB->xTaskRunState == taskTASK_SCHEDULED_TO_YIELD );
 
+            portCLEAR_INTERRUPT_MASK( uxSavedInterruptStatus );
             portENABLE_INTERRUPTS();
 
             /* Enabling interrupts should cause this core to immediately
@@ -849,7 +850,16 @@ static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION;
              * yielding here then that is a problem. */
             configASSERT( pxThisTCB->xTaskRunState != taskTASK_SCHEDULED_TO_YIELD );
 
+            if( uxSavedInterruptStatus == pdFALSE )
+            {
+                portSET_INTERRUPT_MASK();
+            }
             portDISABLE_INTERRUPTS();
+
+            /* If nesting count changed due to ISR critical sections, adjust
+             * the saved nesting count accordingly. */
+            uxPrevCriticalNesting += portGET_CRITICAL_NESTING_COUNT();
+
             portGET_TASK_LOCK();
             portGET_ISR_LOCK();
 
@@ -3861,7 +3871,7 @@ void vTaskSuspendAll( void )
              * it. */
             if( uxSchedulerSuspended == 0U )
             {
-                prvCheckForRunStateChange();
+                prvCheckForRunStateChange( ulState );
             }
             else
             {
@@ -6985,7 +6995,7 @@ static void prvResetNextTaskUnblockTime( void )
                      * before a context switch and vTaskExitCritical() is called
                      * after pxCurrentTCB changes. Therefore this should not be
                      * used within vTaskSwitchContext(). */
-                    prvCheckForRunStateChange();
+                    prvCheckForRunStateChange(pdTRUE);
                 }
             }
         }