Merge branch 'bugfix/yield_after_critical_section' into 'master'

freertos: Delay context switch from queue/task APIs until exiting critical section

Closes #374: https://github.com/espressif/esp-idf/issues/374

See merge request !610

components/freertos/include/freertos/portmacro.h

@@ -79,6 +79,8 @@ extern "C" {
 #include <xtensa/config/core.h>
 #include <xtensa/config/system.h>	/* required for XSHAL_CLIB */
 #include <xtensa/xtruntime.h>
+#include "esp_crosscore_int.h"
+
 
 //#include "xtensa_context.h"
 
@@ -261,6 +263,18 @@ void vPortYield( void );
 void _frxt_setup_switch( void );
 #define portYIELD()					vPortYield()
 #define portYIELD_FROM_ISR()		_frxt_setup_switch()
+
+static inline uint32_t xPortGetCoreID();
+
+/* Yielding within an API call (when interrupts are off), means the yield should be delayed
+   until interrupts are re-enabled.
+
+   To do this, we use the "cross-core" interrupt as a trigger to yield on this core when interrupts are re-enabled.This
+   is the same interrupt & code path which is used to trigger a yield between CPUs, although in this case the yield is
+   happening on the same CPU.
+*/
+#define portYIELD_WITHIN_API() esp_crosscore_int_send_yield(xPortGetCoreID())
+
 /*-----------------------------------------------------------*/
 
 /* Task function macros as described on the FreeRTOS.org WEB site. */

components/freertos/queue.c

@@ -123,15 +123,9 @@ zero. */
 #if( configUSE_PREEMPTION == 0 )
 	/* If the cooperative scheduler is being used then a yield should not be
 	performed just because a higher priority task has been woken. */
-	#define queueYIELD_IF_USING_PREEMPTION_MUX()
 	#define queueYIELD_IF_USING_PREEMPTION()
 #else
 	#define queueYIELD_IF_USING_PREEMPTION() portYIELD_WITHIN_API()
-	#define queueYIELD_IF_USING_PREEMPTION_MUX(mux) { \
-					taskEXIT_CRITICAL(mux); \
-					portYIELD_WITHIN_API(); \
-					taskENTER_CRITICAL(mux); \
-					} while(0)
 #endif
 
 /*
@@ -290,7 +284,7 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue;
 			{
 				if( xTaskRemoveFromEventList( &( pxQueue->xTasksWaitingToSend ) ) == pdTRUE )
 				{
-					queueYIELD_IF_USING_PREEMPTION_MUX(&pxQueue->mux);
+					queueYIELD_IF_USING_PREEMPTION();
 				}
 				else
 				{
@@ -753,7 +747,7 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue;
 							/* The queue is a member of a queue set, and posting
 							to the queue set caused a higher priority task to
 							unblock. A context switch is required. */
-							queueYIELD_IF_USING_PREEMPTION_MUX(&pxQueue->mux);
+							queueYIELD_IF_USING_PREEMPTION();
 						}
 						else
 						{
@@ -772,7 +766,7 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue;
 								our own so yield immediately.  Yes it is ok to
 								do this from within the critical section - the
 								kernel takes care of that. */
-								queueYIELD_IF_USING_PREEMPTION_MUX(&pxQueue->mux);
+								queueYIELD_IF_USING_PREEMPTION();
 							}
 							else
 							{
@@ -785,7 +779,7 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue;
 							executed if the task was holding multiple mutexes
 							and the mutexes were given back in an order that is
 							different to that in which they were taken. */
-							queueYIELD_IF_USING_PREEMPTION_MUX(&pxQueue->mux);
+							queueYIELD_IF_USING_PREEMPTION();
 						}
 						else
 						{
@@ -805,7 +799,7 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue;
 							our own so yield immediately.  Yes it is ok to do
 							this from within the critical section - the kernel
 							takes care of that. */
-							queueYIELD_IF_USING_PREEMPTION_MUX(&pxQueue->mux);
+							queueYIELD_IF_USING_PREEMPTION();
 						}
 						else
 						{
@@ -818,7 +812,7 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue;
 						executed if the task was holding multiple mutexes and
 						the mutexes were given back in an order that is
 						different to that in which they were taken. */
-						queueYIELD_IF_USING_PREEMPTION_MUX(&pxQueue->mux);
+						queueYIELD_IF_USING_PREEMPTION();
 					}
 					else
 					{
@@ -1493,7 +1487,7 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue;
 					{
 						if( xTaskRemoveFromEventList( &( pxQueue->xTasksWaitingToSend ) ) == pdTRUE )
 						{
-							queueYIELD_IF_USING_PREEMPTION_MUX(&pxQueue->mux);
+							queueYIELD_IF_USING_PREEMPTION();
 						}
 						else
 						{
@@ -1522,7 +1516,7 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue;
 						if( xTaskRemoveFromEventList( &( pxQueue->xTasksWaitingToReceive ) ) != pdFALSE )
 						{
 							/* The task waiting has a higher priority than this task. */
-							queueYIELD_IF_USING_PREEMPTION_MUX(&pxQueue->mux);
+							queueYIELD_IF_USING_PREEMPTION();
 						}
 						else
 						{

components/freertos/tasks.c

@@ -120,14 +120,8 @@ functions but without including stdio.h here. */
 	/* If the cooperative scheduler is being used then a yield should not be
 	performed just because a higher priority task has been woken. */
 	#define taskYIELD_IF_USING_PREEMPTION()
-	#define queueYIELD_IF_USING_PREEMPTION_MUX(mux)
 #else
 	#define taskYIELD_IF_USING_PREEMPTION() portYIELD_WITHIN_API()
-	#define taskYIELD_IF_USING_PREEMPTION_MUX(mux) { \
-					taskEXIT_CRITICAL(mux); \
-					portYIELD_WITHIN_API(); \
-					taskENTER_CRITICAL(mux); \
-					} while(0)
 #endif
 
 
@@ -1166,7 +1160,7 @@ static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode
 		{
 			if( xCoreID == xPortGetCoreID() )
 			{
-				taskYIELD_IF_USING_PREEMPTION_MUX(&xTaskQueueMutex);
+				taskYIELD_IF_USING_PREEMPTION();
 			}
 			else {
 				taskYIELD_OTHER_CORE(xCoreID, pxNewTCB->uxPriority);
@@ -1703,7 +1697,7 @@ static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode
 
 				if( xYieldRequired == pdTRUE )
 				{
-					taskYIELD_IF_USING_PREEMPTION_MUX(&xTaskQueueMutex);
+					taskYIELD_IF_USING_PREEMPTION();
 				}
 				else
 				{
@@ -1895,7 +1889,7 @@ static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode
 						/* This yield may not cause the task just resumed to run,
 						but will leave the lists in the correct state for the
 						next yield. */
-						taskYIELD_IF_USING_PREEMPTION_MUX(&xTaskQueueMutex);
+						taskYIELD_IF_USING_PREEMPTION();
 					}
 					else if( pxTCB->xCoreID != xPortGetCoreID() )
 					{
@@ -2206,7 +2200,7 @@ BaseType_t xAlreadyYielded = pdFALSE;
 						xAlreadyYielded = pdTRUE;
 					}
 					#endif
-					taskYIELD_IF_USING_PREEMPTION_MUX(&xTaskQueueMutex);
+					taskYIELD_IF_USING_PREEMPTION();
 				}
 				else
 				{

components/freertos/test/test_freertos_eventgroups.c

@@ -52,14 +52,15 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]")
     }
 
     /* Tasks all start instantly, but this task will resume running at the same time as the higher priority tasks on the
-       other processor may still be setting up, so give a tick for them to also block on BIT_CALL... */
+       other processor may still be setting up, so allow time for them to also block on BIT_CALL... */
-    vTaskDelay(1);
+    vTaskDelay(10);
 
     for (int i = 0; i < COUNT; i++) {
         /* signal all tasks with "CALL" bit... */
         xEventGroupSetBits(eg, BIT_CALL);
 
-        TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, 100 / portMAX_DELAY));
+        /* Only wait for 1 tick, the wakeup should be immediate... */
+        TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, 1));
     }
 
     /* Ensure all tasks cleaned up correctly */

components/freertos/test/test_preemption.c

@@ -0,0 +1,108 @@
+/*
+ Unit tests for FreeRTOS preemption
+*/
+
+#include <esp_types.h>
+#include <stdio.h>
+#include "rom/ets_sys.h"
+
+#include "freertos/FreeRTOS.h"
+#include "freertos/task.h"
+#include "freertos/semphr.h"
+#include "freertos/queue.h"
+#include "freertos/xtensa_api.h"
+#include "unity.h"
+#include "soc/cpu.h"
+
+static volatile bool trigger;
+static volatile bool flag;
+
+/* Task:
+   - Waits for 'trigger' variable to be set
+   - Reads the cycle count on this CPU
+   - Pushes it into a queue supplied as a param
+   - Busy-waits until the main task terminates it
+*/
+static void task_send_to_queue(void *param)
+{
+    QueueHandle_t queue = (QueueHandle_t) param;
+    uint32_t ccount;
+
+    while(!trigger) {}
+
+    RSR(CCOUNT, ccount);
+    flag = true;
+    xQueueSendToBack(queue, &ccount, 0);
+    /* This is to ensure that higher priority task
+       won't wake anyhow, due to this task terminating.
+
+       The task runs until terminated by the main task.
+    */
+    while(1) {}
+}
+
+TEST_CASE("Yield from lower priority task, same CPU", "[freertos]")
+{
+    /* Do this 3 times, mostly for the benchmark value - the first
+       run includes a cache miss so uses more cycles than it should. */
+    for (int i = 0; i < 3; i++) {
+        TaskHandle_t sender_task;
+        QueueHandle_t queue = xQueueCreate(1, sizeof(uint32_t));
+        flag = false;
+        trigger = false;
+
+        /* "yield" task sits on our CPU, lower priority to us */
+        xTaskCreatePinnedToCore(task_send_to_queue, "YIELD", 2048, (void *)queue, UNITY_FREERTOS_PRIORITY - 1, &sender_task, UNITY_FREERTOS_CPU);
+
+        vTaskDelay(1); /* make sure everything is set up */
+        trigger = true;
+
+        uint32_t yield_ccount, now_ccount, delta;
+        TEST_ASSERT( xQueueReceive(queue, &yield_ccount, 100 / portTICK_PERIOD_MS) );
+        RSR(CCOUNT, now_ccount);
+        TEST_ASSERT( flag );
+
+        delta = now_ccount - yield_ccount;
+        printf("Yielding from lower priority task took %u cycles\n", delta);
+        TEST_ASSERT(delta < 10000);
+
+        vQueueDelete(queue);
+        vTaskDelete(sender_task);
+    }
+}
+
+
+TEST_CASE("Yield from lower priority task, other CPU", "[freertos]")
+{
+    uint32_t trigger_ccount, yield_ccount, now_ccount, delta;
+
+    /* Do this 3 times, mostly for the benchmark value - the first
+       run includes a cache miss so uses more cycles than it should. */
+    for (int i = 0; i < 3; i++) {
+        TaskHandle_t sender_task;
+        QueueHandle_t queue = xQueueCreate(1, sizeof(uint32_t));
+        trigger = false;
+        flag = false;
+
+        /* "send_to_queue" task sits on the other CPU, lower priority to us */
+        xTaskCreatePinnedToCore(task_send_to_queue, "YIELD", 2048, (void *)queue, UNITY_FREERTOS_PRIORITY - 1,
+                                &sender_task, !UNITY_FREERTOS_CPU);
+
+        vTaskDelay(2); /* make sure everything is set up */
+        trigger = true;
+        RSR(CCOUNT, trigger_ccount);
+
+        // yield_ccount is not useful in this test as it's the other core's CCOUNT
+        // so we use trigger_ccount instead
+        TEST_ASSERT( xQueueReceive(queue, &yield_ccount, 100 / portTICK_PERIOD_MS) );
+        RSR(CCOUNT, now_ccount);
+        TEST_ASSERT( flag );
+
+        delta = now_ccount - trigger_ccount;
+        printf("Yielding from task on other core took %u cycles\n", delta);
+        TEST_ASSERT(delta < 10000);
+
+        vQueueDelete(queue);
+        vTaskDelete(sender_task);
+    }
+}

tools/unit-test-app/components/unity/include/unity_config.h

@@ -9,6 +9,10 @@
 
 #include <esp_err.h>
 
+/* Some definitions applicable to Unity running in FreeRTOS */
+#define UNITY_FREERTOS_PRIORITY 5
+#define UNITY_FREERTOS_CPU 0
+
 #define UNITY_EXCLUDE_FLOAT
 #define UNITY_EXCLUDE_DOUBLE
 

tools/unit-test-app/main/app_main.c

@@ -2,7 +2,7 @@
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
 #include "unity.h"
-
+#include "unity_config.h"
 
 void unityTask(void *pvParameters)
 {
@@ -15,6 +15,6 @@ void app_main()
 {
     // Note: if unpinning this task, change the way run times are calculated in
     // unity_platform
-    xTaskCreatePinnedToCore(unityTask, "unityTask", 4096, NULL, 5, NULL, 0);
+    xTaskCreatePinnedToCore(unityTask, "unityTask", 4096, NULL,
+                            UNITY_FREERTOS_PRIORITY, NULL, UNITY_FREERTOS_CPU);
 }
-