diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 15d2e135d3..f20715903d 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -2177,7 +2177,6 @@ BaseType_t xAlreadyYielded = pdFALSE; { /* We can schedule the awoken task on this CPU. */ xYieldPending[xPortGetCoreID()] = pdTRUE; - break; } else { @@ -3039,6 +3038,8 @@ BaseType_t xTaskRemoveFromEventList( const List_t * const pxEventList ) { TCB_t *pxUnblockedTCB; BaseType_t xReturn; +BaseType_t xTaskCanBeReady; +UBaseType_t i, uxTargetCPU; /* THIS FUNCTION MUST BE CALLED FROM A CRITICAL SECTION. It can also be called from a critical section within an ISR. */ @@ -3062,7 +3063,24 @@ BaseType_t xReturn; return pdFALSE; } - if( uxSchedulerSuspended[ xPortGetCoreID() ] == ( UBaseType_t ) pdFALSE ) + /* Determine if the task can possibly be run on either CPU now, either because the scheduler + the task is pinned to is running or because a scheduler is running on any CPU. */ + xTaskCanBeReady = pdFALSE; + if ( pxUnblockedTCB->xCoreID == tskNO_AFFINITY ) { + uxTargetCPU = xPortGetCoreID(); + for (i = 0; i < portNUM_PROCESSORS; i++) { + if ( uxSchedulerSuspended[ i ] == ( UBaseType_t ) pdFALSE ) { + xTaskCanBeReady = pdTRUE; + break; + } + } + } else { + uxTargetCPU = pxUnblockedTCB->xCoreID; + xTaskCanBeReady = uxSchedulerSuspended[ uxTargetCPU ] == ( UBaseType_t ) pdFALSE; + + } + + if( xTaskCanBeReady ) { ( void ) uxListRemove( &( pxUnblockedTCB->xGenericListItem ) ); prvAddTaskToReadyList( pxUnblockedTCB ); @@ -3070,8 +3088,8 @@ BaseType_t xReturn; else { /* The delayed and ready lists cannot be accessed, so hold this task - pending until the scheduler is resumed. */ - vListInsertEnd( &( xPendingReadyList[ xPortGetCoreID() ] ), &( pxUnblockedTCB->xEventListItem ) ); + pending until the scheduler is resumed on this CPU. */ + vListInsertEnd( &( xPendingReadyList[ uxTargetCPU ] ), &( pxUnblockedTCB->xEventListItem ) ); } if ( tskCAN_RUN_HERE(pxUnblockedTCB->xCoreID) && pxUnblockedTCB->uxPriority >= pxCurrentTCB[ xPortGetCoreID() ]->uxPriority ) diff --git a/components/freertos/test/test_suspend_scheduler.c b/components/freertos/test/test_suspend_scheduler.c index d3c8ed86a2..c613dc6474 100644 --- a/components/freertos/test/test_suspend_scheduler.c +++ b/components/freertos/test/test_suspend_scheduler.c @@ -12,7 +12,7 @@ #include "driver/timer.h" static SemaphoreHandle_t isr_semaphore; -static volatile unsigned isr_count, task_count; +static volatile unsigned isr_count; /* Timer ISR increments an ISR counter, and signals a mutex semaphore to wake up another counter task */ @@ -29,12 +29,18 @@ static void timer_group0_isr(void *vp_arg) } } -static void counter_task_fn(void *ignore) +typedef struct { + SemaphoreHandle_t trigger_sem; + volatile unsigned counter; +} counter_config_t; + +static void counter_task_fn(void *vp_config) { + counter_config_t *config = (counter_config_t *)vp_config; printf("counter_task running...\n"); while(1) { - xSemaphoreTake(isr_semaphore, portMAX_DELAY); - task_count++; + xSemaphoreTake(config->trigger_sem, portMAX_DELAY); + config->counter++; } } @@ -43,20 +49,23 @@ static void counter_task_fn(void *ignore) In the FreeRTOS implementation, this exercises the xPendingReadyList for that core. */ -TEST_CASE("Handle pending context switch while scheduler disabled", "[freertos]") +TEST_CASE("Scheduler disabled can handle a pending context switch on resume", "[freertos]") { - task_count = 0; isr_count = 0; isr_semaphore = xSemaphoreCreateMutex(); TaskHandle_t counter_task; intr_handle_t isr_handle = NULL; + counter_config_t count_config = { + .trigger_sem = isr_semaphore, + .counter = 0, + }; xTaskCreatePinnedToCore(counter_task_fn, "counter", 2048, - NULL, UNITY_FREERTOS_PRIORITY + 1, + &count_config, UNITY_FREERTOS_PRIORITY + 1, &counter_task, UNITY_FREERTOS_CPU); /* Configure timer ISR */ - const timer_config_t config = { + const timer_config_t timer_config = { .alarm_en = 1, .auto_reload = 1, .counter_dir = TIMER_COUNT_UP, @@ -65,7 +74,7 @@ TEST_CASE("Handle pending context switch while scheduler disabled", "[freertos]" .counter_en = TIMER_PAUSE, }; /* Configure timer */ - timer_init(TIMER_GROUP_0, TIMER_0, &config); + timer_init(TIMER_GROUP_0, TIMER_0, &timer_config); timer_pause(TIMER_GROUP_0, TIMER_0); timer_set_counter_value(TIMER_GROUP_0, TIMER_0, 0); timer_set_alarm_value(TIMER_GROUP_0, TIMER_0, 1000); @@ -76,20 +85,20 @@ TEST_CASE("Handle pending context switch while scheduler disabled", "[freertos]" vTaskDelay(5); // Check some counts have been triggered via the ISR - TEST_ASSERT(task_count > 10); + TEST_ASSERT(count_config.counter > 10); TEST_ASSERT(isr_count > 10); for (int i = 0; i < 20; i++) { vTaskSuspendAll(); esp_intr_noniram_disable(); - unsigned no_sched_task = task_count; + unsigned no_sched_task = count_config.counter; // scheduler off on this CPU... ets_delay_us(20 * 1000); //TEST_ASSERT_NOT_EQUAL(no_sched_isr, isr_count); - TEST_ASSERT_EQUAL(task_count, no_sched_task); + TEST_ASSERT_EQUAL(count_config.counter, no_sched_task); // disable timer interrupts timer_disable_intr(TIMER_GROUP_0, TIMER_0); @@ -99,7 +108,7 @@ TEST_CASE("Handle pending context switch while scheduler disabled", "[freertos]" esp_intr_noniram_enable(); xTaskResumeAll(); - TEST_ASSERT_NOT_EQUAL(task_count, no_sched_task); + TEST_ASSERT_NOT_EQUAL(count_config.counter, no_sched_task); } esp_intr_free(isr_handle); @@ -108,3 +117,133 @@ TEST_CASE("Handle pending context switch while scheduler disabled", "[freertos]" vTaskDelete(counter_task); vSemaphoreDelete(isr_semaphore); } + +/* Multiple tasks on different cores can be added to the pending ready list + while scheduler is suspended, and should be started once the scheduler + resumes. +*/ +TEST_CASE("Scheduler disabled can wake multiple tasks on resume", "[freertos]") +{ + #define TASKS_PER_PROC 4 + TaskHandle_t tasks[portNUM_PROCESSORS][TASKS_PER_PROC] = { 0 }; + counter_config_t counters[portNUM_PROCESSORS][TASKS_PER_PROC] = { 0 }; + + /* Start all the tasks, they will block on isr_semaphore */ + for (int p = 0; p < portNUM_PROCESSORS; p++) { + for (int t = 0; t < TASKS_PER_PROC; t++) { + counters[p][t].trigger_sem = xSemaphoreCreateMutex(); + TEST_ASSERT_NOT_NULL( counters[p][t].trigger_sem ); + TEST_ASSERT( xSemaphoreTake(counters[p][t].trigger_sem, 0) ); + xTaskCreatePinnedToCore(counter_task_fn, "counter", 2048, + &counters[p][t], UNITY_FREERTOS_PRIORITY + 1, + &tasks[p][t], p); + TEST_ASSERT_NOT_NULL( tasks[p][t] ); + } + } + + /* takes a while to initialize tasks on both cores, sometimes... */ + vTaskDelay(TASKS_PER_PROC * portNUM_PROCESSORS * 3); + + /* Check nothing is counting, each counter should be blocked on its trigger_sem */ + for (int p = 0; p < portNUM_PROCESSORS; p++) { + for (int t = 0; t < TASKS_PER_PROC; t++) { + TEST_ASSERT_EQUAL(0, counters[p][t].counter); + } + } + + /* Suspend scheduler on this CPU */ + vTaskSuspendAll(); + + /* Give all the semaphores once. This will wake tasks immediately on the other + CPU, but they are deferred here until the scheduler resumes. + */ + for (int p = 0; p < portNUM_PROCESSORS; p++) { + for (int t = 0; t < TASKS_PER_PROC; t++) { + xSemaphoreGive(counters[p][t].trigger_sem); + } + } + + ets_delay_us(200); /* Let the other CPU do some things */ + + for (int p = 0; p < portNUM_PROCESSORS; p++) { + for (int t = 0; t < TASKS_PER_PROC; t++) { + int expected = (p == UNITY_FREERTOS_CPU) ? 0 : 1; // Has run if it was on the other CPU + ets_printf("Checking CPU %d task %d (expected %d actual %d)\n", p, t, expected, counters[p][t].counter); + TEST_ASSERT_EQUAL(expected, counters[p][t].counter); + } + } + + /* Resume scheduler */ + xTaskResumeAll(); + + /* Now the tasks on both CPUs should have been woken once and counted once. */ + for (int p = 0; p < portNUM_PROCESSORS; p++) { + for (int t = 0; t < TASKS_PER_PROC; t++) { + ets_printf("Checking CPU %d task %d (expected 1 actual %d)\n", p, t, counters[p][t].counter); + TEST_ASSERT_EQUAL(1, counters[p][t].counter); + } + } + + /* Clean up */ + for (int p = 0; p < portNUM_PROCESSORS; p++) { + for (int t = 0; t < TASKS_PER_PROC; t++) { + vTaskDelete(tasks[p][t]); + vSemaphoreDelete(counters[p][t].trigger_sem); + } + } +} + +static volatile bool sched_suspended; +static void suspend_scheduler_5ms_task_fn(void *ignore) +{ + vTaskSuspendAll(); + sched_suspended = true; + for (int i = 0; i <5; i++) { + ets_delay_us(1000); + } + xTaskResumeAll(); + sched_suspended = false; + vTaskDelete(NULL); +} + +#ifndef CONFIG_FREERTOS_UNICORE +/* If the scheduler is disabled on one CPU (A) with a task blocked on something, and a task + on B (where scheduler is running) wakes it, then the task on A should be woken on resume. +*/ +TEST_CASE("Scheduler disabled on CPU B, tasks on A can wake", "[freertos]") +{ + TaskHandle_t counter_task; + SemaphoreHandle_t wake_sem = xSemaphoreCreateMutex(); + xSemaphoreTake(wake_sem, 0); + counter_config_t count_config = { + .trigger_sem = wake_sem, + .counter = 0, + }; + xTaskCreatePinnedToCore(counter_task_fn, "counter", 2048, + &count_config, UNITY_FREERTOS_PRIORITY + 1, + &counter_task, !UNITY_FREERTOS_CPU); + + xTaskCreatePinnedToCore(suspend_scheduler_5ms_task_fn, "suspender", 2048, + NULL, UNITY_FREERTOS_PRIORITY - 1, + NULL, !UNITY_FREERTOS_CPU); + + /* counter task is now blocked on other CPU, waiting for wake_sem, and we expect + that this CPU's scheduler will be suspended for 5ms shortly... */ + while(!sched_suspended) { } + + xSemaphoreGive(wake_sem); + ets_delay_us(1000); + // Bit of a race here if the other CPU resumes its scheduler, but 5ms is a long time... */ + TEST_ASSERT(sched_suspended); + TEST_ASSERT_EQUAL(0, count_config.counter); // the other task hasn't woken yet, because scheduler is off + TEST_ASSERT(sched_suspended); + + /* wait for the rest of the 5ms... */ + while(sched_suspended) { } + + ets_delay_us(100); + TEST_ASSERT_EQUAL(1, count_config.counter); // when scheduler resumes, counter task should immediately count + + vTaskDelete(counter_task); +} +#endif diff --git a/docs/api-guides/freertos-smp.rst b/docs/api-guides/freertos-smp.rst index 7ccba1b10a..a706320d84 100644 --- a/docs/api-guides/freertos-smp.rst +++ b/docs/api-guides/freertos-smp.rst @@ -227,6 +227,9 @@ protection against simultaneous access. Consider using critical sections (disables interrupts) or semaphores (does not disable interrupts) instead when protecting shared resources in ESP-IDF FreeRTOS. +In general, it's better to use other RTOS primitives like mutex semaphores to protect +against data shared between tasks, rather than ``vTaskSuspendAll()``. + .. _tick-interrupt-synchronicity: Tick Interrupt Synchronicity @@ -366,4 +369,4 @@ functionality of ``xTaskCreateStaticPinnedToCore()`` in ESP-IDF FreeRTOS :ref:`CONFIG_FREERTOS_ASSERT_ON_UNTESTED_FUNCTION` will trigger a halt in particular functions in ESP-IDF FreeRTOS which have not been fully tested in an SMP context. - \ No newline at end of file +