Merge branch 'bugfix/freertos_resume_scheduler_pending_tasks' into 'master'

freertos: Fix bug with xTaskResumeAll() not resuming all tasks



See merge request !1330

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 )

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

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.
-    
+