-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Feature/smp granular locks v4 #1154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature/smp granular locks v4 #1154
Conversation
@chinglee-iot @aggarg This PR introduces the granular locks changes to the FreeRTOS kernel. Please have a look and we could have discussions/changes in this PR context. Thank you. cc: @ESP-Marius @Dazza0 |
Thank you for your contribution, I'll forward this request to the team. There are few error in the PR can you please try fixing them |
Hello @sudeep-mohanty, I am just following up if you had time to fix the build issues |
@rawalexe Yes! I shall work on the failures and would also do some refactoring for an easier review process. For now, I've put this PR in draft. |
0159a4a
to
5bf8c33
Compare
Hi @sudeep-mohanty, |
5bf8c33
to
9f8acc7
Compare
Hi @ActoryOu, I've made some updates which should fix the CI failures however I could not understand why the link-verifier action fails. Seems more like a script failure to me. So this action would still fail. If you have more information on what is causing it, could you let me know and I shall fix it. Thanks. |
c79962d
to
f50d476
Compare
Created a PR to fix the CI failure - FreeRTOS/FreeRTOS#1292 |
70eb466
to
edc1c98
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the partial review suggestion of tasks.c file and also update previous review suggestion:
- Adding core ID to portENTER/EXIT_CRITICAL_DATA_GROUP parameters in these macros doesn't make sense, since the core ID is only valid when the task's executing core remains unchanged.
bf1d964
to
d3784c3
Compare
d3784c3
to
0847cd0
Compare
tasks.c
Outdated
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 ) | ||
|| ( ( taskTASK_IS_RUNNING( pxCurrentTCBs[ xCoreID ] ) ) && ( pxCurrentTCBs[ xCoreID ]->xPreemptionDisable > 0U ) ) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please help me to clarify that the following sequence.
- Task A is running with data group lock
- Task B suspend Task A. prvYieldCore( xTaskACore ) is called
- Task A is requested to yield.
- Task A's xTaskRunState is set to taskTASK_SCHEDULED_TO_YIELD
- Task A runs xTaskSwitchContext(). In this function, the running
state is taskTASK_SCHEDULED_TO_YIELD, which is result in taskTASK_IS_RUNNING is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chinglee-iot,
After reviewing the design, I believe there’s a potential issue, as you’ve rightly pointed out: when a task disables preemption, it may still be susceptible to state changes initiated from the other core. The current design does not appear to account for this scenario.
To address this, we should consider reverting to the earlier approach where the scheduler is suspended entirely, rather than just disabling task preemption. This would offer stronger guarantees against concurrent state modifications across cores.
That said, I’ll also explore whether the current design can be adapted to address this limitation. If you have any other ideas or suggestions, please feel free to share them—happy to discuss further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to discuss the following approach
- check the preemption disable status in the prvYieldCore(). If the core is running a task with preemption disabled, the yield request will be delayed until the preemption enabled.
- Add assertion in
vTaskSwitchContext()
. We should not select task for a core which is running a task with preemption disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following changes should fix the mentioned problems -
- Added task preemption disable/enable during data group critical sections. This helps to avoid problems such as a task "escaping" deletion or being deleted while it is running on another core.
- Added a deferred state change processing when a task's preemption is disabled. This helps to avoid the state nullification problem. Currently, it seems that we only need to defer task deletion and task suspension, with deletion give
prvYieldCore()
now checks for current task's preemption disabled status.vTaskSwitchContext()
now expects that the current's task must not have preemption disabled when it is invoked by adding an assertion.
tasks.c
Outdated
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 ) | ||
|| ( ( taskTASK_IS_RUNNING( pxCurrentTCBs[ xCoreID ] ) ) && ( pxCurrentTCBs[ xCoreID ]->xPreemptionDisable > 0U ) ) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to discuss the following approach
- check the preemption disable status in the prvYieldCore(). If the core is running a task with preemption disabled, the yield request will be delayed until the preemption enabled.
- Add assertion in
vTaskSwitchContext()
. We should not select task for a core which is running a task with preemption disabled.
/* Lock the kernel data group as we are about to access its members */ | ||
UBaseType_t uxSavedInterruptStatus; | ||
|
||
if( portCHECK_IF_IN_ISR() == pdTRUE ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's consider to split this function into two functions.
xTaskRemoveFromEventListSafe()
xTaskRemoveFromEventListFromISRSafe()
Both of the function above calls xTaskRemoveFromEventList. Then we don't have to depend on portCHECK_IF_IN_ISR() function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chinglee-iot I revisited this change and I need some advice about how to handle the scenario where xTaskRemoveFromEventList()
is called from prvNotifyQueueSetContainer()
. The later can be called from both non-ISR functions and from-ISR functions like xQueueGenericSend()
and xQueueGenericSendFromISR()
. If we add Safe
versions of xTaskRemoveFromEventList()
we will need to move the portCHECK_IN_ISR()
check to queue.c
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have nested data group critical problem needs to be updated in the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other review suggestions regarding preemption disabled and entering kernel critical section with nested critical section:
- Consider to set xYieldPendings flag if the core is running a task with preemption disabled in prvYieldCore.
- In vTaskPreemptionEnable, the task should yield when xYieldPendings is set to 1.
- In vTaskExitCritical, xYieldCurrentTask should consider preemption disable. It should be set when preemption is not disabled.
d65b8e6
to
f0609db
Compare
|
f0609db
to
8eb906d
Compare
…herit() xTaskPriorityInherit() is called inside a critical section from queue.c. This commit moves the critical section into xTaskPriorityInherit(). Co-authored-by: Sudeep Mohanty <[email protected]>
Changed xPreemptionDisable to be a count rather than a pdTRUE/pdFALSE. This allows nested calls to vTaskPreemptionEnable(), where a yield only occurs when xPreemptionDisable is 0. Co-authored-by: Sudeep Mohanty <[email protected]>
Adds the required checks for granular locking port macros. Port Config: - portUSING_GRANULAR_LOCKS to enable granular locks - portCRITICAL_NESTING_IN_TCB should be disabled Granular Locking Port Macros: - Spinlocks - portSPINLOCK_TYPE - portINIT_SPINLOCK( pxSpinlock ) - portINIT_SPINLOCK_STATIC - Locking - portGET_SPINLOCK() - portRELEASE_SPINLOCK() Co-authored-by: Sudeep Mohanty <[email protected]>
8eb906d
to
c32b603
Compare
- Updated prvCheckForRunStateChange() for granular locks - Updated vTaskSuspendAll() and xTaskResumeAll() - Now holds the xTaskSpinlock during kernel suspension - Increments/decrements xPreemptionDisable. Only yields when 0, thus allowing for nested suspensions across different data groups Co-authored-by: Sudeep Mohanty <[email protected]>
Updated critical section macros with granular locks. Some tasks.c API relied on their callers to enter critical sections. This assumption no longer works under granular locking. Critical sections added to the following functions: - `vTaskInternalSetTimeOutState()` - `xTaskIncrementTick()` - `vTaskSwitchContext()` - `xTaskRemoveFromEventList()` - `vTaskInternalSetTimeOutState()` - `eTaskConfirmSleepModeStatus()` - `xTaskPriorityDisinherit()` - `pvTaskIncrementMutexHeldCount()` Added missing suspensions to the following functions: - `vTaskPlaceOnEventList()` - `vTaskPlaceOnUnorderedEventList()` - `vTaskPlaceOnEventListRestricted()` Fixed the locking in vTaskSwitchContext() vTaskSwitchContext() must aquire both kernel locks, viz., task lock and ISR lock. This is because, vTaskSwitchContext() can be called from either task context or ISR context. Also, vTaskSwitchContext() must not alter the interrupt state prematurely. Co-authored-by: Sudeep Mohanty <[email protected]>
Updated queue.c to use granular locking - Added xTaskSpinlock and xISRSpinlock - Replaced critical section macros with data group critical section macros such as taskENTER/EXIT_CRITICAL/_FROM_ISR() with queueENTER/EXIT_CRITICAL_FROM_ISR(). - Added vQueueEnterCritical/FromISR() and vQueueExitCritical/FromISR() which map to the data group critical section macros. - Added prvLockQueueForTasks() and prvUnlockQueueForTasks() as the granular locking equivalents to prvLockQueue() and prvUnlockQueue() respectively Co-authored-by: Sudeep Mohanty <[email protected]>
Updated event_groups.c to use granular locking - Added xTaskSpinlock and xISRSpinlock - Replaced critical section macros with data group critical section macros such as taskENTER/EXIT_CRITICAL/_FROM_ISR() with event_groupsENTER/EXIT_CRITICAL/_FROM_ISR(). - Added vEventGroupsEnterCritical/FromISR() and vEventGroupsExitCriti/FromISR() functions that map to the data group critical section macros. - Added prvLockEventGroupForTasks() and prvUnlockEventGroupForTasks() to suspend the event group when executing non-deterministic code. - xEventGroupSetBits() and vEventGroupDelete() accesses the kernel data group directly. Thus, added vTaskSuspendAll()/xTaskResumeAll() to these fucntions. Co-authored-by: Sudeep Mohanty <[email protected]>
Updated stream_buffer.c to use granular locking - Added xTaskSpinlock and xISRSpinlock - Replaced critical section macros with data group critical section macros such as taskENTER/EXIT_CRITICAL/_FROM_ISR() with sbENTER/EXIT_CRITICAL_FROM_ISR(). - Added vStreambuffersEnterCritical/FromISR() and vStreambuffersExitCritical/FromISR() to map to the data group critical section macros. - Added prvLockStreamBufferForTasks() and prvUnlockStreamBufferForTasks() to suspend the stream buffer when executing non-deterministic code. Co-authored-by: Sudeep Mohanty <[email protected]>
Updated timers.c to use granular locking - Added xTaskSpinlock and xISRSpinlock - Replaced critical section macros with data group critical section macros such as taskENTER/EXIT_CRITICAL() with tmrENTER/EXIT_CRITICAL(). - Added vTimerEnterCritical() and vTimerExitCritical() to map to the data group critical section macros. Co-authored-by: Sudeep Mohanty <[email protected]>
Co-authored-by: Sudeep Mohanty <[email protected]>
c32b603
to
da5dccf
Compare
|
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
targetesp32
, setup the ESP-IDF environment on your local machine. The steps to follow are listed in the Getting Started Guide.components/freertos/test_apps/freertos
where all the test cases are located.performance
subfolder at the same location.idf.py seet-target esp32
.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.idf.py build flash monitor
.TODO
Checklist:
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.