From 20212ee8231476e2030944cb29d269620f701401 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 28 Feb 2017 12:06:36 +1100 Subject: [PATCH 1/3] freertos: Fix cross-core usage of event groups Fixes & re-enabled broken unit tests Adds per-event-group spinlock instead of single global lock --- components/freertos/event_groups.c | 32 ++++++++----- .../freertos/test/test_freertos_eventgroups.c | 46 ++++++++++++------- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/components/freertos/event_groups.c b/components/freertos/event_groups.c index 902a4ad72..69903817f 100644 --- a/components/freertos/event_groups.c +++ b/components/freertos/event_groups.c @@ -119,13 +119,10 @@ typedef struct xEventGroupDefinition UBaseType_t uxEventGroupNumber; #endif + portMUX_TYPE eventGroupMux; } EventGroup_t; -/* Again: one mux for all events. Maybe this can be made more granular. ToDo: look into that. -JD */ -static portMUX_TYPE xEventGroupMux = portMUX_INITIALIZER_UNLOCKED; - - /*-----------------------------------------------------------*/ /* @@ -156,6 +153,8 @@ EventGroup_t *pxEventBits; traceEVENT_GROUP_CREATE_FAILED(); } + vPortCPUInitializeMutex(&pxEventBits->eventGroupMux); + return ( EventGroupHandle_t ) pxEventBits; } /*-----------------------------------------------------------*/ @@ -176,6 +175,7 @@ BaseType_t xTimeoutOccurred = pdFALSE; #endif vTaskSuspendAll(); + taskENTER_CRITICAL(&pxEventBits->eventGroupMux); { uxOriginalBitValue = pxEventBits->uxEventBits; @@ -217,6 +217,7 @@ BaseType_t xTimeoutOccurred = pdFALSE; } } } + taskEXIT_CRITICAL( &pxEventBits->eventGroupMux ); xAlreadyYielded = xTaskResumeAll(); if( xTicksToWait != ( TickType_t ) 0 ) @@ -239,7 +240,7 @@ BaseType_t xTimeoutOccurred = pdFALSE; if( ( uxReturn & eventUNBLOCKED_DUE_TO_BIT_SET ) == ( EventBits_t ) 0 ) { /* The task timed out, just return the current event bit value. */ - taskENTER_CRITICAL( &xEventGroupMux ); + taskENTER_CRITICAL( &pxEventBits->eventGroupMux ); { uxReturn = pxEventBits->uxEventBits; @@ -256,7 +257,7 @@ BaseType_t xTimeoutOccurred = pdFALSE; mtCOVERAGE_TEST_MARKER(); } } - taskEXIT_CRITICAL( &xEventGroupMux ); + taskEXIT_CRITICAL( &pxEventBits->eventGroupMux ); xTimeoutOccurred = pdTRUE; } @@ -295,6 +296,7 @@ BaseType_t xTimeoutOccurred = pdFALSE; #endif vTaskSuspendAll(); + taskENTER_CRITICAL( &pxEventBits->eventGroupMux ); { const EventBits_t uxCurrentEventBits = pxEventBits->uxEventBits; @@ -361,6 +363,7 @@ BaseType_t xTimeoutOccurred = pdFALSE; traceEVENT_GROUP_WAIT_BITS_BLOCK( xEventGroup, uxBitsToWaitFor ); } } + taskEXIT_CRITICAL( &pxEventBits->eventGroupMux ); xAlreadyYielded = xTaskResumeAll(); if( xTicksToWait != ( TickType_t ) 0 ) @@ -382,7 +385,7 @@ BaseType_t xTimeoutOccurred = pdFALSE; if( ( uxReturn & eventUNBLOCKED_DUE_TO_BIT_SET ) == ( EventBits_t ) 0 ) { - taskENTER_CRITICAL( &xEventGroupMux ); + taskENTER_CRITICAL( &pxEventBits->eventGroupMux ); { /* The task timed out, just return the current event bit value. */ uxReturn = pxEventBits->uxEventBits; @@ -405,7 +408,7 @@ BaseType_t xTimeoutOccurred = pdFALSE; mtCOVERAGE_TEST_MARKER(); } } - taskEXIT_CRITICAL( &xEventGroupMux ); + taskEXIT_CRITICAL( &pxEventBits->eventGroupMux ); /* Prevent compiler warnings when trace macros are not used. */ xTimeoutOccurred = pdFALSE; @@ -434,7 +437,7 @@ EventBits_t uxReturn; configASSERT( xEventGroup ); configASSERT( ( uxBitsToClear & eventEVENT_BITS_CONTROL_BYTES ) == 0 ); - taskENTER_CRITICAL( &xEventGroupMux ); + taskENTER_CRITICAL( &pxEventBits->eventGroupMux ); { traceEVENT_GROUP_CLEAR_BITS( xEventGroup, uxBitsToClear ); @@ -445,7 +448,7 @@ EventBits_t uxReturn; /* Clear the bits. */ pxEventBits->uxEventBits &= ~uxBitsToClear; } - taskEXIT_CRITICAL( &xEventGroupMux ); + taskEXIT_CRITICAL( &pxEventBits->eventGroupMux ); return uxReturn; } @@ -498,7 +501,9 @@ BaseType_t xMatchFound = pdFALSE; pxList = &( pxEventBits->xTasksWaitingForBits ); pxListEnd = listGET_END_MARKER( pxList ); /*lint !e826 !e740 The mini list structure is used as the list end to save RAM. This is checked and valid. */ + vTaskSuspendAll(); + taskENTER_CRITICAL(&pxEventBits->eventGroupMux); { traceEVENT_GROUP_SET_BITS( xEventGroup, uxBitsToSet ); @@ -570,6 +575,7 @@ BaseType_t xMatchFound = pdFALSE; bit was set in the control word. */ pxEventBits->uxEventBits &= ~uxBitsToClear; } + taskEXIT_CRITICAL(&pxEventBits->eventGroupMux); ( void ) xTaskResumeAll(); return pxEventBits->uxEventBits; @@ -578,10 +584,11 @@ BaseType_t xMatchFound = pdFALSE; void vEventGroupDelete( EventGroupHandle_t xEventGroup ) { -EventGroup_t *pxEventBits = ( EventGroup_t * ) xEventGroup; -const List_t *pxTasksWaitingForBits = &( pxEventBits->xTasksWaitingForBits ); + EventGroup_t *pxEventBits = ( EventGroup_t * ) xEventGroup; + const List_t *pxTasksWaitingForBits = &( pxEventBits->xTasksWaitingForBits ); vTaskSuspendAll(); + taskENTER_CRITICAL( &pxEventBits->eventGroupMux ); { traceEVENT_GROUP_DELETE( xEventGroup ); @@ -593,6 +600,7 @@ const List_t *pxTasksWaitingForBits = &( pxEventBits->xTasksWaitingForBits ); ( void ) xTaskRemoveFromUnorderedEventList( pxTasksWaitingForBits->xListEnd.pxNext, eventUNBLOCKED_DUE_TO_BIT_SET ); } + taskEXIT_CRITICAL( &pxEventBits->eventGroupMux ); vPortFree( pxEventBits ); } ( void ) xTaskResumeAll(); diff --git a/components/freertos/test/test_freertos_eventgroups.c b/components/freertos/test/test_freertos_eventgroups.c index 32dee2d20..dce770af5 100644 --- a/components/freertos/test/test_freertos_eventgroups.c +++ b/components/freertos/test/test_freertos_eventgroups.c @@ -11,9 +11,10 @@ #define BIT_RESPONSE(TASK) (1 << (TASK+1)) #define ALL_RESPONSE_BITS (((1 << NUM_TASKS) - 1) << 1) -static const int NUM_TASKS = 4; -static const int COUNT = 4000; +static const int NUM_TASKS = 8; +static const int COUNT = 1000; static EventGroupHandle_t eg; +static SemaphoreHandle_t done_sem; static void task_event_group_call_response(void *param) { @@ -32,15 +33,14 @@ static void task_event_group_call_response(void *param) } printf("Task %d done\n", task_num); - - /* Delay is due to not-yet-fixed bug with deleting tasks at same time */ - vTaskDelay(100 / portTICK_PERIOD_MS); + xSemaphoreGive(done_sem); vTaskDelete(NULL); } -TEST_CASE("FreeRTOS Event Groups", "[freertos][ignore]") +TEST_CASE("FreeRTOS Event Groups", "[freertos]") { eg = xEventGroupCreate(); + done_sem = xSemaphoreCreateCounting(NUM_TASKS, 0); /* Note: task_event_group_call_response all have higher priority than us, so will block together. @@ -50,7 +50,7 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos][ignore]") for (int c = 0; c < NUM_TASKS; c++) { xTaskCreatePinnedToCore(task_event_group_call_response, "tsk_call_resp", 4096, (void *)c, configMAX_PRIORITIES - 1, NULL, c % portNUM_PROCESSORS); } - /* Scheduler weirdness, if we don't sleep a few ticks here then the tasks on the other CPU aren't running yet... */ + /* Scheduler weirdness (bug?), if we don't sleep a few ticks here then the tasks on the other CPU aren't running yet... */ vTaskDelay(10); for (int i = 0; i < COUNT; i++) { @@ -60,11 +60,17 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos][ignore]") /* signal all tasks with "CALL" bit... */ xEventGroupSetBits(eg, BIT_CALL); - while (xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, portMAX_DELAY) != ALL_RESPONSE_BITS) { - } + TEST_ASSERT_EQUAL(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, 100 / portMAX_DELAY)); } -} + /* Ensure all tasks cleaned up correctly */ + for (int c = 0; c < NUM_TASKS; c++) { + TEST_ASSERT( xSemaphoreTake(done_sem, 100/portTICK_PERIOD_MS) ); + } + + vSemaphoreDelete(done_sem); + vEventGroupDelete(eg); +} #define BIT_DONE(X) (1<<(NUM_TASKS+1+X)) @@ -82,24 +88,32 @@ static void task_test_sync(void *param) } int after_done = xEventGroupSetBits(eg, BIT_DONE(task_num)); - printf("Done %d = %x\n", task_num, after_done); + printf("Done %d = 0x%08x\n", task_num, after_done); - /* Delay is due to not-yet-fixed bug with deleting tasks at same time */ - vTaskDelay(100 / portTICK_PERIOD_MS); + xSemaphoreGive(done_sem); vTaskDelete(NULL); } -TEST_CASE("FreeRTOS Event Group Sync", "[freertos][ignore]") +TEST_CASE("FreeRTOS Event Group Sync", "[freertos]") { eg = xEventGroupCreate(); + done_sem = xSemaphoreCreateCounting(NUM_TASKS, 0); for (int c = 0; c < NUM_TASKS; c++) { xTaskCreatePinnedToCore(task_test_sync, "task_test_sync", 4096, (void *)c, configMAX_PRIORITIES - 1, NULL, c % portNUM_PROCESSORS); } for (int c = 0; c < NUM_TASKS; c++) { - printf("Waiting on %d (%x)\n", c, BIT_DONE(c)); - xEventGroupWaitBits(eg, BIT_DONE(c), false, false, portMAX_DELAY); + printf("Waiting on %d (0x%08x)\n", c, BIT_DONE(c)); + TEST_ASSERT( xEventGroupWaitBits(eg, BIT_DONE(c), false, false, portMAX_DELAY) ); } + + /* Ensure all tasks cleaned up correctly */ + for (int c = 0; c < NUM_TASKS; c++) { + TEST_ASSERT( xSemaphoreTake(done_sem, 100/portTICK_PERIOD_MS) ); + } + + vSemaphoreDelete(done_sem); + vEventGroupDelete(eg); } From 8de26e434cf9585726be9ca1e1f4396b8caba52e Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 28 Feb 2017 15:13:30 +1100 Subject: [PATCH 2/3] freertos: Schedule tasks immediately when they are created on opposite core --- components/freertos/tasks.c | 96 +++++++++++-------- .../freertos/test/test_freertos_eventgroups.c | 21 ++-- 2 files changed, 66 insertions(+), 51 deletions(-) diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 0804bb3eb..b37e59254 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -1046,25 +1046,59 @@ UBaseType_t x; } /*-----------------------------------------------------------*/ -static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode, const BaseType_t xCoreID ) +static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode, BaseType_t xCoreID ) { - TCB_t *curTCB; - BaseType_t i; + TCB_t *curTCB, *tcb0, *tcb1; /* Ensure interrupts don't access the task lists while the lists are being updated. */ taskENTER_CRITICAL(&xTaskQueueMutex); { uxCurrentNumberOfTasks++; - //If the task has no affinity and nothing is scheduled on this core, just throw it this core. - //If it has affinity, throw it on the core that needs it if nothing is already scheduled there. - BaseType_t xMyCore = xCoreID; - if ( xMyCore == tskNO_AFFINITY) xMyCore = xPortGetCoreID(); - if( pxCurrentTCB[ xMyCore ] == NULL ) + + // Determine which core this task starts on + if ( xCoreID == tskNO_AFFINITY ) + { + if ( portNUM_PROCESSORS == 1 ) + { + xCoreID = 0; + } + else + { + // if the task has no affinity, put it on either core if nothing is currently scheduled there. Failing that, + // put it on the core where it will preempt the lowest priority running task. If neither of these are true, + // queue it on the currently running core. + tcb0 = pxCurrentTCB[0]; + tcb1 = pxCurrentTCB[1]; + if ( tcb0 == NULL ) + { + xCoreID = 0; + } + else if ( tcb1 == NULL ) + { + xCoreID = 1; + } + else if ( tcb0->uxPriority < pxNewTCB->uxPriority && tcb0->uxPriority < tcb1->uxPriority ) + { + xCoreID = 0; + } + else if ( tcb1->uxPriority < pxNewTCB->uxPriority ) + { + xCoreID = 1; + } + else + { + xCoreID = xPortGetCoreID(); // Both CPU have higher priority tasks running on them, so this won't run yet + } + } + } + + // If nothing is running on this core, put the new task there now + if( pxCurrentTCB[ xCoreID ] == NULL ) { /* There are no other tasks, or all the other tasks are in the suspended state - make this the current task. */ - pxCurrentTCB[ xMyCore ] = pxNewTCB; + pxCurrentTCB[ xCoreID ] = pxNewTCB; if( uxCurrentNumberOfTasks == ( UBaseType_t ) 1 ) { @@ -1090,19 +1124,11 @@ static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode so far. */ if( xSchedulerRunning == pdFALSE ) { - /* Scheduler isn't running yet. We need to determine on which CPU to run this task. */ - for ( i=0; iuxPriority <= pxNewTCB->uxPriority ) { - /* Can we schedule this task on core i? */ - if (xCoreID == tskNO_AFFINITY || xCoreID == i) - { - /* Schedule if nothing is scheduled yet, or overwrite a task of lower prio. */ - if ( pxCurrentTCB[i] == NULL || pxCurrentTCB[i]->uxPriority <= pxNewTCB->uxPriority ) - { - pxCurrentTCB[i] = pxNewTCB; - break; - } - } + pxCurrentTCB[xCoreID] = pxNewTCB; } } else @@ -1130,37 +1156,27 @@ static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode if( xSchedulerRunning != pdFALSE ) { - taskENTER_CRITICAL(&xTaskQueueMutex); - curTCB = pxCurrentTCB[ xPortGetCoreID() ]; + taskENTER_CRITICAL(&xTaskQueueMutex); + + curTCB = pxCurrentTCB[ xCoreID ]; /* Scheduler is running. If the created task is of a higher priority than an executing task - then it should run now. - ToDo: This only works for the current core. If a task is scheduled on an other processor, - the other processor will keep running the task it's working on, and only switch to the newer - task on a timer interrupt. */ - //No mux here, uxPriority is mostly atomic and there's not really any harm if this check misfires. - if( curTCB->uxPriority < pxNewTCB->uxPriority ) + then it should run now. + */ + if( curTCB == NULL || curTCB->uxPriority < pxNewTCB->uxPriority ) { - /* Scheduler is running. If the created task is of a higher priority than an executing task - then it should run now. - No mux here, uxPriority is mostly atomic and there's not really any harm if this check misfires. - */ - if( tskCAN_RUN_HERE( xCoreID ) && curTCB->uxPriority < pxNewTCB->uxPriority ) + if( xCoreID == xPortGetCoreID() ) { taskYIELD_IF_USING_PREEMPTION_MUX(&xTaskQueueMutex); } - else if( xCoreID != xPortGetCoreID() ) { + else { taskYIELD_OTHER_CORE(xCoreID, pxNewTCB->uxPriority); } - else - { - mtCOVERAGE_TEST_MARKER(); - } } else { mtCOVERAGE_TEST_MARKER(); } - taskEXIT_CRITICAL(&xTaskQueueMutex); + taskEXIT_CRITICAL(&xTaskQueueMutex); } else { diff --git a/components/freertos/test/test_freertos_eventgroups.c b/components/freertos/test/test_freertos_eventgroups.c index dce770af5..f32c20ade 100644 --- a/components/freertos/test/test_freertos_eventgroups.c +++ b/components/freertos/test/test_freertos_eventgroups.c @@ -25,8 +25,7 @@ static void task_event_group_call_response(void *param) for (int i = 0; i < COUNT; i++) { /* Wait until the common "call" bit is set, starts off all tasks (clear on return) */ - while (!xEventGroupWaitBits(eg, BIT_CALL, true, false, portMAX_DELAY)) { - } + TEST_ASSERT( xEventGroupWaitBits(eg, BIT_CALL, true, false, portMAX_DELAY) ); /* Set our individual "response" bit */ xEventGroupSetBits(eg, BIT_RESPONSE(task_num)); @@ -42,25 +41,25 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]") eg = xEventGroupCreate(); done_sem = xSemaphoreCreateCounting(NUM_TASKS, 0); - /* Note: task_event_group_call_response all have higher priority than us, so will block together. + /* Note: task_event_group_call_response all have higher priority than this task, so on this core + they will always preempt this task. - This is important because we need to know they'll all have blocked on BIT_CALL each time we - signal it, or they get out of sync. + This is important because we need to know all tasks have blocked on BIT_CALL each time we signal it, + or they get out of sync. */ for (int c = 0; c < NUM_TASKS; c++) { xTaskCreatePinnedToCore(task_event_group_call_response, "tsk_call_resp", 4096, (void *)c, configMAX_PRIORITIES - 1, NULL, c % portNUM_PROCESSORS); } - /* Scheduler weirdness (bug?), if we don't sleep a few ticks here then the tasks on the other CPU aren't running yet... */ - vTaskDelay(10); + + /* 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... */ + vTaskDelay(1); for (int i = 0; i < COUNT; i++) { - if (i % 100 == 0) { - //printf("Call %d\n", i); - } /* signal all tasks with "CALL" bit... */ xEventGroupSetBits(eg, BIT_CALL); - TEST_ASSERT_EQUAL(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, 100 / portMAX_DELAY)); + TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, 100 / portMAX_DELAY)); } /* Ensure all tasks cleaned up correctly */ From 2230b2c8bce812a51a14144176119b6bb4bb4bc7 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 28 Feb 2017 15:31:40 +1100 Subject: [PATCH 3/3] freertos tests: Enable test_freertos_task_delete --- components/freertos/test/test_freertos_task_delete.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/components/freertos/test/test_freertos_task_delete.c b/components/freertos/test/test_freertos_task_delete.c index d8cc2755e..68a6683fc 100644 --- a/components/freertos/test/test_freertos_task_delete.c +++ b/components/freertos/test/test_freertos_task_delete.c @@ -10,13 +10,17 @@ static void task_delete_self(void *param) { printf("Task %p running on core %d. Deleting shortly...\n", xTaskGetCurrentTaskHandle(), xPortGetCoreID()); + vTaskDelay(5); vTaskDelete(NULL); } -TEST_CASE("FreeRTOS Delete Tasks", "[freertos][ignore]") +TEST_CASE("FreeRTOS Delete Tasks", "[freertos]") { + uint32_t before_count = uxTaskGetNumberOfTasks(); + xTaskCreatePinnedToCore(task_delete_self, "tsk_self_a", 4096, NULL, configMAX_PRIORITIES - 1, NULL, 0); xTaskCreatePinnedToCore(task_delete_self, "tsk_self_a", 4096, NULL, configMAX_PRIORITIES - 1, NULL, 0); + TEST_ASSERT_EQUAL(before_count + 2, uxTaskGetNumberOfTasks()); vTaskDelay(200 / portTICK_PERIOD_MS); - printf("Done?\n"); + TEST_ASSERT_EQUAL(before_count, uxTaskGetNumberOfTasks()); }