Feature/smp granular locks v4

[lightbulbkhuang]

This PR adds support for granular locking to the FreeRTOS kernel.

Description

Granular locking introduces the concept of having localized locks per kernel data group for SMP configuration. This method is an optional replacement of the existing kernel locks and is controlled by a new port layer configuration, viz., portUSING_GRANULAR_LOCKS. More details about the approach can be found here.

Test Steps

The implementation has been tested on Espressif SoC targets, viz., the ESP32 using the ESP-IDF framework.

1. Testing on an esp32 target

To test the implementation of the granular locking scheme on esp32, setup the ESP-IDF environment on your local machine. The steps to follow are listed in the Getting Started Guide.
Instead of the main ESP-IDF repository, the granular locks implementation resides in this froked repository - https://github.com/sudeep-mohanty/esp-idf.
Once you have cloned the forked repository and setup the ESP-IDF environment, change your branch to feat/granular_locks_tests.
To run the FreeRTOS unit tests, change the directory to components/freertos/test_apps/freertos where all the test cases are located.
Performance tests are located in the performance subfolder at the same location.
Setup the target device with the command idf.py seet-target esp32.
Select the Amazon FreeRTOS SMP kernel using the menuconfig options. To do this you must enter the command -idf.py menuconfig -> Component config -> FreeRTOS -> Kernel -> Run the Amazon SMP FreeRTOS kernel instead (FEATURE UNDER DEVELOPMENT). Save the configuration and exit the menuconfig.
Now, build and flash the unit test application with the command idf.py build flash monitor.
Once the app is up, you can enter the number of the test case and run the unity test case.

TODO

Test setup for other targets (Raspberry Pi Pico)
Generic target tests to be uploaded to the FreeRTOS repository.

Checklist:

I have tested my changes. No regression in existing tests.
I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

Closes FreeRTOS#905

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

[github-actions]

Granular Locking Implementation Review

Summary

This PR introduces a significant architectural enhancement to the FreeRTOS SMP kernel by implementing granular locking through the "Dual Spinlock With Data Group Locking" approach. The implementation organizes kernel data into separate data groups (Kernel, Queue, Event Group, Stream Buffer, and Timer data groups), each protected by their own pair of spinlocks (xTaskSpinlock and xISRSpinlock).

Key Architectural Changes

✅ Strengths:

• Well-designed hierarchical data group organization prevents deadlocks
• Maintains backward compatibility with existing applications
• Comprehensive documentation in granular_locks_v4.md
• Proper conditional compilation to maintain single-core compatibility
• Thread safety analysis appears thorough

Issues Requiring Attention

MISRA C 2012 Violations (Critical)

The static analysis identified several MISRA C 2012 violations that must be addressed:

1. Rule 20.7: Macro parameters need parentheses protection
2. Rule 20.9: Preprocessor identifiers should be tested for definition before use

Code Quality Issues (High Priority)

1. Macro Definition Inconsistencies: Non-granular lock macros have trailing semicolons causing style inconsistency
2. Return Value Mismatch: Some unlock macros don't preserve expected return values
3. Potential Structure Issue: Duplicate conditional compilation blocks in static structure definitions

Implementation Concerns (Medium Priority)

1. Performance Impact: While the goal is to reduce lock contention, the implementation adds overhead in lock/unlock operations for each data group
2. Complexity: The nested locking approach, while safe, significantly increases code complexity
3. Testing Coverage: No evidence of comprehensive testing on actual SMP hardware configurations

Recommendations

Immediate Actions Required:

1. Fix all MISRA violations identified in the review comments
2. Standardize macro definitions to remove inconsistencies
3. Verify data structure layout to ensure static allocation compatibility

Before Merging:

1. Performance benchmarking on actual SMP hardware to validate lock contention improvements
2. Comprehensive testing with stress scenarios and edge cases
3. Memory overhead analysis to quantify the additional spinlock storage requirements

Code Review Confidence Level: Medium-High

The implementation follows good architectural principles and maintains safety through proper critical section management. However, the MISRA violations and inconsistencies need to be resolved before this can be considered production-ready.

The granular locking approach is theoretically sound and should provide performance benefits in high-contention SMP scenarios, but empirical validation is essential given the complexity introduced.

[github-actions]

MISRA C 2012 Rule 20.7 violation: Macro parameters should be enclosed in parentheses.

The macro parameter pxTaskSpinlock and pxISRSpinlock should be enclosed in parentheses to prevent unintended operator precedence issues when the macro is expanded.

Fix:

#define taskDATA_GROUP_ENTER_CRITICAL( pxTaskSpinlock, pxISRSpinlock )                \
do {                                                                                  \
    /* Disable preemption to avoid task state changes during the critical section. */ \
    vTaskPreemptionDisable( NULL );                                                   \
    {                                                                                 \
        const BaseType_t xCoreID = ( BaseType_t ) portGET_CORE_ID();                  \
        /* Task spinlock is always taken first */                                     \
        portGET_SPINLOCK( xCoreID, ( portSPINLOCK_TYPE * ) (pxTaskSpinlock) );       \
        /* Disable interrupts */                                                      \
        portDISABLE_INTERRUPTS();                                                     \
        /* Take the ISR spinlock next */                                              \
        portGET_SPINLOCK( xCoreID, ( portSPINLOCK_TYPE * ) (pxISRSpinlock) );        \
        /* Increment the critical nesting count */                                    \
        portINCREMENT_CRITICAL_NESTING_COUNT( xCoreID );                              \
    }                                                                                 \
} while( 0 )

[github-actions]

MISRA C 2012 Rule 20.7 violation: Macro parameters should be enclosed in parentheses.

The macro parameters pxTaskSpinlock and pxISRSpinlock should be enclosed in parentheses.

Fix:

#define taskDATA_GROUP_EXIT_CRITICAL( pxTaskSpinlock, pxISRSpinlock )        \
do {                                                                         \
    const BaseType_t xCoreID = ( BaseType_t ) portGET_CORE_ID();             \
    configASSERT( portGET_CRITICAL_NESTING_COUNT( xCoreID ) > 0U );          \
    /* Release the ISR spinlock */                                           \
    portRELEASE_SPINLOCK( xCoreID, ( portSPINLOCK_TYPE * ) (pxISRSpinlock) ); \
    /* Release the task spinlock */                                          \
    portRELEASE_SPINLOCK( xCoreID, ( portSPINLOCK_TYPE * ) (pxTaskSpinlock) ); \
    /* Decrement the critical nesting count */                               \
    portDECREMENT_CRITICAL_NESTING_COUNT( xCoreID );                         \
    /* Enable interrupts only if the critical nesting count is 0 */          \
    if( portGET_CRITICAL_NESTING_COUNT( xCoreID ) == 0 )                     \
    {                                                                        \
        portENABLE_INTERRUPTS();                                             \
    }                                                                        \
    else                                                                     \
    {                                                                        \
        mtCOVERAGE_TEST_MARKER();                                            \
    }                                                                        \
    /* Re-enable preemption */                                               \
    vTaskPreemptionEnable( NULL );                                           \
} while( 0 )

[github-actions]

MISRA C 2012 Rule 20.9 violation: All identifiers used in the controlling expression of #if or #elif preprocessing directives shall be #define'd before evaluation.

The preprocessor identifier portUSING_GRANULAR_LOCKS should be tested for definition before being used in conditional compilation.

Fix:

#if ( defined( portUSING_GRANULAR_LOCKS ) && ( portUSING_GRANULAR_LOCKS == 1 ) )
    #define taskDATA_GROUP_ENTER_CRITICAL( pxTaskSpinlock, pxISRSpinlock )                \
    do {                                                                                  \
        /* ... macro body ... */                                                         \
    } while( 0 )
#endif /* #if ( defined( portUSING_GRANULAR_LOCKS ) && ( portUSING_GRANULAR_LOCKS == 1 ) ) */

Or ensure portUSING_GRANULAR_LOCKS is always defined in FreeRTOS.h before this point (which appears to be done, but the static analyzer may be checking individual files).

[github-actions]

MISRA C 2012 Rule 20.7 violation: Macro parameters should be enclosed in parentheses.

The macro parameter pxQueue should be enclosed in parentheses to prevent unintended operator precedence issues.

Fix:

#define queueENTER_CRITICAL( pxQueue )                                      taskDATA_GROUP_ENTER_CRITICAL( &(pxQueue)->xTaskSpinlock, &(pxQueue)->xISRSpinlock )
#define queueENTER_CRITICAL_FROM_ISR( pxQueue, puxSavedInterruptStatus )    taskDATA_GROUP_ENTER_CRITICAL_FROM_ISR( &(pxQueue)->xISRSpinlock, puxSavedInterruptStatus )
#define queueEXIT_CRITICAL( pxQueue )                                       taskDATA_GROUP_EXIT_CRITICAL( &(pxQueue)->xTaskSpinlock, &(pxQueue)->xISRSpinlock )
#define queueEXIT_CRITICAL_FROM_ISR( uxSavedInterruptStatus, pxQueue )      taskDATA_GROUP_EXIT_CRITICAL_FROM_ISR( uxSavedInterruptStatus, &(pxQueue)->xISRSpinlock )

[github-actions]

MISRA C 2012 Rule 20.9 violation: All identifiers used in the controlling expression of #if or #elif preprocessing directives shall be #define'd before evaluation.

Similar to the previous violation with portUSING_GRANULAR_LOCKS and configNUMBER_OF_CORES. Ensure all preprocessor identifiers are properly defined before use.

Fix:

#if ( defined( portUSING_GRANULAR_LOCKS ) && ( portUSING_GRANULAR_LOCKS == 1 ) && defined( configNUMBER_OF_CORES ) && ( configNUMBER_OF_CORES > 1 ) )
        portSPINLOCK_TYPE xTaskSpinlock;
        portSPINLOCK_TYPE xISRSpinlock;
#endif /* #if ( defined( portUSING_GRANULAR_LOCKS ) && ( portUSING_GRANULAR_LOCKS == 1 ) && defined( configNUMBER_OF_CORES ) && ( configNUMBER_OF_CORES > 1 ) ) */

[github-actions]

Code Style Issue: Inconsistent macro definition style.

The non-granular lock versions of these macros have semicolons, which is inconsistent with FreeRTOS style and can cause issues in certain contexts.

Fix:

#else /* #if ( portUSING_GRANULAR_LOCKS == 1 ) */
    #define event_groupsENTER_CRITICAL( pxEventBits )                                      taskENTER_CRITICAL()
    #define event_groupsENTER_CRITICAL_FROM_ISR( pxEventBits, puxSavedInterruptStatus )    do { *( puxSavedInterruptStatus ) = taskENTER_CRITICAL_FROM_ISR(); } while( 0 )
    #define event_groupsEXIT_CRITICAL( pxEventBits )                                       taskEXIT_CRITICAL()
    #define event_groupsEXIT_CRITICAL_FROM_ISR( uxSavedInterruptStatus, pxEventBits )      taskEXIT_CRITICAL_FROM_ISR( uxSavedInterruptStatus )
#endif /* #if ( portUSING_GRANULAR_LOCKS == 1 ) */

Remove the trailing semicolons to maintain consistency with FreeRTOS coding standards.

[github-actions]

Macro Issue: Missing return value in unlock macro.

The event_groupsUNLOCK macro should return a value consistent with the xTaskResumeAll() function it replaces.

Fix:

#if ( ( portUSING_GRANULAR_LOCKS == 1 ) && ( configNUMBER_OF_CORES > 1 ) )
    #define event_groupsLOCK( pxEventBits )      prvLockEventGroupForTasks( pxEventBits )
    #define event_groupsUNLOCK( pxEventBits )    prvUnlockEventGroupForTasks( pxEventBits )
#else /* #if ( ( portUSING_GRANULAR_LOCKS == 1 ) && ( configNUMBER_OF_CORES > 1 ) ) */
    #define event_groupsLOCK( pxEventBits )      vTaskSuspendAll()
    #define event_groupsUNLOCK( pxEventBits )    xTaskResumeAll()
#endif /* #if ( ( portUSING_GRANULAR_LOCKS == 1 ) && ( configNUMBER_OF_CORES > 1 ) ) */

Note the removal of the semicolon in the granular lock version to match the non-granular version's return behavior.

[github-actions]

Potential Issue: Duplicate conditional compilation blocks.

There appear to be two separate #if ( configUSE_TASK_PREEMPTION_DISABLE == 1 ) blocks on consecutive lines, which may indicate a copy-paste error.

Current code:

#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
    UBaseType_t xDummy25;
#endif
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
    BaseType_t xDummy26;
#endif

Please verify: Is this intentional or should these be combined into a single block?

Suggested fix if combining:

#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
    UBaseType_t xDummy25;
    BaseType_t xDummy26;
#endif