Skip to content

Initial SMP review - DO NOT MERGE #750

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Initial SMP review - DO NOT MERGE #750

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 31 additions & 30 deletions tasks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
#include <stdio.h>
#endif /* configUSE_STATS_FORMATTING_FUNCTIONS == 1 ) */

#if ( configNUMBER_OF_CORES == 1 )
#if ( configNUMBER_OF_CORES == 1 ) /*_RB_ Not clear why this only applies to single core - please add an explanation. */
#if ( configUSE_PREEMPTION == 0 )

/* If the cooperative scheduler is being used then a yield should not be
Expand Down Expand Up @@ -135,7 +135,7 @@

/*-----------------------------------------------------------*/

#if ( configNUMBER_OF_CORES == 1 )
#if ( configNUMBER_OF_CORES == 1 ) /*_RB_ Again needs an explanation as to why this only applies to single core builds. */
#define taskSELECT_HIGHEST_PRIORITY_TASK() \
do { \
UBaseType_t uxTopPriority = uxTopReadyPriority; \
Expand Down Expand Up @@ -164,7 +164,7 @@

#else /* configUSE_PORT_OPTIMISED_TASK_SELECTION */

#if ( configNUMBER_OF_CORES > 1 )
#if ( configNUMBER_OF_CORES > 1 ) /*_RB_ Please move to FreeRTOS.h with the other contains configuration file checks. */
#error configUSE_PORT_OPTIMISED_TASK_SELECTION not supported in FreeRTOS SMP.
#endif

Expand Down Expand Up @@ -260,34 +260,34 @@
#endif

/* Task state. */
typedef BaseType_t TaskRunning_t;
typedef BaseType_t TaskRunning_t; /*_RB_ I'm not sure this typedef is needed, but if it is, please move it down to where the other typdefs are declared. */

/* Indicates that the task is not actively running on any core. */
#define taskTASK_NOT_RUNNING ( TaskRunning_t ) ( -1 )
#define taskTASK_NOT_RUNNING ( TaskRunning_t ) ( -1 ) /*_RB_ Might be preferable to put parentheses around the whole thing so the cast is only on the "-1", i.e. (( TaskRunning_t ) ( -1 )). Is the typedef needed at all? */

/* Indicates that the task is actively running but scheduled to yield. */
/* Indicates that the task is actively running but scheduled to yield. *//*_RB_ Perhaps taskTASK_SCHEDULED_TO_YILED is a better name then. */
#define taskTASK_YIELDING ( TaskRunning_t ) ( -2 )

/* Returns pdTRUE if the task is actively running and not scheduled to yield. */
#if ( configNUMBER_OF_CORES == 1 )
#define taskTASK_IS_RUNNING( pxTCB ) ( ( ( pxTCB ) == pxCurrentTCB ) ? ( pdTRUE ) : ( pdFALSE ) )
#define taskTASK_IS_RUNNING( pxTCB ) ( ( ( pxTCB ) == pxCurrentTCB ) ? ( pdTRUE ) : ( pdFALSE ) ) /*_RB_ Is the ternary operator needed here? Just ( ( pxTCB) == pxCurrentTCB ) is going to return value if they are not equal anyway. Likewise for the line below. Single line macros are the only place a ternary operator might be allowed. */
#else
#define taskTASK_IS_RUNNING( pxTCB ) ( ( ( ( pxTCB )->xTaskRunState >= ( BaseType_t ) 0 ) && ( ( pxTCB )->xTaskRunState < ( BaseType_t ) configNUMBER_OF_CORES ) ) ? ( pdTRUE ) : ( pdFALSE ) )
#endif

/* Indicates that the task is an Idle task. */
#define taskATTRIBUTE_IS_IDLE ( UBaseType_t ) ( 1UL << 0UL )
#define taskATTRIBUTE_IS_IDLE ( UBaseType_t ) ( 1UL << 0UL ) /* Again best to put parentheses around the whole definition too. */

#if ( ( configNUMBER_OF_CORES > 1 ) && ( portCRITICAL_NESTING_IN_TCB == 1 ) )
#define portGET_CRITICAL_NESTING_COUNT() ( pxCurrentTCBs[ portGET_CORE_ID() ]->uxCriticalNesting )
#if ( ( configNUMBER_OF_CORES > 1 ) && ( portCRITICAL_NESTING_IN_TCB == 1 ) ) /*_RB_ Later usage of these macros only have a preprocessor test on the number of cores, not whether the critical nesting count is in the TCB. */
#define portGET_CRITICAL_NESTING_COUNT() ( pxCurrentTCBs[ portGET_CORE_ID() ]->uxCriticalNesting ) /*_RB_ Macros are prefixed with the file they are defined in, so these should be "task" if defined in tasks.c. */
#define portSET_CRITICAL_NESTING_COUNT( x ) ( pxCurrentTCBs[ portGET_CORE_ID() ]->uxCriticalNesting = ( x ) )
#define portINCREMENT_CRITICAL_NESTING_COUNT() ( pxCurrentTCBs[ portGET_CORE_ID() ]->uxCriticalNesting++ )
#define portDECREMENT_CRITICAL_NESTING_COUNT() ( pxCurrentTCBs[ portGET_CORE_ID() ]->uxCriticalNesting-- )
#endif /* #if ( ( configNUMBER_OF_CORES > 1 ) && ( portCRITICAL_NESTING_IN_TCB == 1 ) ) */

/* Code below here allows infinite loop controlling, especially for the infinite loop
* in idle task function (for example when performing unit tests). */
#ifndef INFINITE_LOOP
#ifndef INFINITE_LOOP /*_RB_ Not sure of the purpose of this. for( ;; ) is the preferred method of creating an infinite loop as it avoids "condition is always true" compiler warnings. Suggest removing to make consistent with the rest of the code. */
#define INFINITE_LOOP() 1
#endif

Expand All @@ -304,7 +304,7 @@ typedef struct tskTaskControlBlock /* The old naming convention is used to
xMPU_SETTINGS xMPUSettings; /**< The MPU settings are defined as part of the port layer. THIS MUST BE THE SECOND MEMBER OF THE TCB STRUCT. */
#endif

#if ( configUSE_CORE_AFFINITY == 1 ) && ( configNUMBER_OF_CORES > 1 )
#if ( configUSE_CORE_AFFINITY == 1 ) && ( configNUMBER_OF_CORES > 1 ) /*_RB_ Ref the comment below - there isn't a confNUM_CORES definition. Is the limitation described in the comment documented anywhere or otherwise tested progamatically? */
UBaseType_t uxCoreAffinityMask; /*< Used to link the task to certain cores. UBaseType_t must have greater than or equal to the number of bits as confNUM_CORES. */
#endif

Expand All @@ -318,7 +318,7 @@ typedef struct tskTaskControlBlock /* The old naming convention is used to
#endif
char pcTaskName[ configMAX_TASK_NAME_LEN ]; /**< Descriptive name given to the task when created. Facilitates debugging only. */ /*lint !e971 Unqualified char types are allowed for strings and single characters only. */

#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 ) /*_RB_ I'm still not philosophically aligned with having this option. */
BaseType_t xPreemptionDisable; /**< Used to prevent the task from being preempted. */
#endif

Expand Down Expand Up @@ -383,7 +383,7 @@ typedef tskTCB TCB_t;
/*lint -save -e956 A manual analysis and inspection has been used to determine
* which static variables must be declared volatile. */
#if ( configNUMBER_OF_CORES == 1 )
portDONT_DISCARD PRIVILEGED_DATA TCB_t * volatile pxCurrentTCB = NULL;
portDONT_DISCARD PRIVILEGED_DATA TCB_t * volatile pxCurrentTCB = NULL; /*_RB_ Do we really need this as it looks like the source of potential duplication? Why not just have an array in all cases, including when configNUMBER_OF_CORES is 1. */
#else
/* MISRA Ref 8.4.1 [Declaration shall be visible] */
/* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-84 */
Expand Down Expand Up @@ -466,7 +466,7 @@ PRIVILEGED_DATA static volatile configRUN_TIME_COUNTER_TYPE ulTotalRunTime[ conf

/* Do not move these variables to function scope as doing so prevents the
* code working with debuggers that need to remove the static qualifier. */
static StaticTask_t xIdleTCBBuffers[ configNUMBER_OF_CORES - 1 ];
static StaticTask_t xIdleTCBBuffers[ configNUMBER_OF_CORES - 1 ]; /*_RB_ Currently static allocation for the idle task is different for core 0 - this needs to be consistent for all cores. */
static StackType_t xIdleTaskStackBuffers[ configNUMBER_OF_CORES - 1 ][ configMINIMAL_STACK_SIZE ];

#endif /* #if ( configSUPPORT_STATIC_ALLOCATION == 1 ) && ( configNUMBER_OF_CORES > 1 ) */
Expand Down Expand Up @@ -551,7 +551,7 @@ static void prvInitialiseTaskLists( void ) PRIVILEGED_FUNCTION;
*/
static portTASK_FUNCTION_PROTO( prvIdleTask, pvParameters ) PRIVILEGED_FUNCTION;
#if ( configNUMBER_OF_CORES > 1 )
static portTASK_FUNCTION_PROTO( prvMinimalIdleTask, pvParameters ) PRIVILEGED_FUNCTION;
static portTASK_FUNCTION_PROTO( prvMinimalIdleTask, pvParameters ) PRIVILEGED_FUNCTION; /*_RB_ We need a better name for this idle task, and better description on the website. */
#endif

/*
Expand Down Expand Up @@ -696,8 +696,8 @@ static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION;
/* This should be skipped if called from an ISR. If the task on the current
* core is no longer running, then vTaskSwitchContext() probably should
* be run before returning, but we don't have a way to force that to happen
* from here. */
if( portCHECK_IF_IN_ISR() == pdFALSE )
* from here. *//*_RB_ This comment is imprecise. Must vTaskSwtichContext() be called or not. When could this get called from an ISR? */
if( portCHECK_IF_IN_ISR() == pdFALSE ) /*_RB_ Many ports already define xPortIsInsideInterrupt(). */
{
/* This function is always called with interrupts disabled
* so this is safe. */
Expand All @@ -718,7 +718,7 @@ static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION;
if( uxPrevCriticalNesting > 0U )
{
portSET_CRITICAL_NESTING_COUNT( 0U );
portRELEASE_ISR_LOCK();
portRELEASE_ISR_LOCK(); /*_RB_ Curious if this is necessary given this function won't run in an ISR. */
}
else
{
Expand Down Expand Up @@ -757,7 +757,7 @@ static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION;
/*-----------------------------------------------------------*/

#if ( configNUMBER_OF_CORES > 1 )
static void prvYieldCore( BaseType_t xCoreID )
static void prvYieldCore( BaseType_t xCoreID ) /*_RB_ This could be a candidate to make a macro as it's in a highs speed path, or, as we are writing new ports for SMP, a portALWAYS_INLINE function. */
{
/* This must be called from a critical section and xCoreID must be valid. */
if( ( portCHECK_IF_IN_ISR() == pdTRUE ) && ( xCoreID == ( BaseType_t ) portGET_CORE_ID() ) )
Expand All @@ -783,7 +783,7 @@ static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION;
#endif /* #if ( configNUMBER_OF_CORES > 1 ) */
/*-----------------------------------------------------------*/

#if ( configNUMBER_OF_CORES > 1 )
#if ( configNUMBER_OF_CORES > 1 ) /*_RB_ We need to simplify - too many configuration options. */
static void prvYieldForTask( const TCB_t * pxTCB )
{
BaseType_t xLowestPriorityToPreempt;
Expand Down Expand Up @@ -1148,6 +1148,7 @@ static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION;

/*-----------------------------------------------------------*/

/*_RB_ The next 25 lines require too much cognitive effort to understand. Please separate the functions. Applies to all task create functions. */
#if ( configSUPPORT_STATIC_ALLOCATION == 1 )

TaskHandle_t xTaskCreateStatic( TaskFunction_t pxTaskCode,
Expand Down Expand Up @@ -1621,13 +1622,13 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,

#if ( ( configNUMBER_OF_CORES > 1 ) && ( configUSE_CORE_AFFINITY == 1 ) )
{
pxNewTCB->uxCoreAffinityMask = tskNO_AFFINITY;
pxNewTCB->uxCoreAffinityMask = tskNO_AFFINITY; /*_RB_ Is this required is pxNewTCB was memset to 0? *//*_RB_ Ah, I see tskNO_AFFINITY is -1, does it have to be? */
}
#endif

#if ( configUSE_TASK_PREEMPTION_DISABLE == 1 )
{
pxNewTCB->xPreemptionDisable = 0;
pxNewTCB->xPreemptionDisable = 0; /* I Don't think this is required if pxNewTCB was memset to 0. */
}
#endif

Expand Down Expand Up @@ -1688,7 +1689,7 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
{
pxNewTCB->xTaskRunState = taskTASK_NOT_RUNNING;

/* Is this an idle task? */
/* Is this an idle task? *//*_RB_ I've not tried compiling this, but from first look, I'm not sure why the casts to TaskFunction_t are necessary as they are all the same type already. Maybe because of pointer comparison? */
if( ( ( TaskFunction_t ) pxTaskCode == ( TaskFunction_t ) prvIdleTask ) || ( ( TaskFunction_t ) pxTaskCode == ( TaskFunction_t ) prvMinimalIdleTask ) )
{
pxNewTCB->uxTaskAttributes |= taskATTRIBUTE_IS_IDLE;
Expand All @@ -1708,7 +1709,7 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
}
}
/*-----------------------------------------------------------*/

/*_RB_ I would like to combine both versions of prvAddNewTaskToReadyList(), which I think could be done if the single core code used the same array of current TCBs than the dual core code - albeit with only one position in the array. */
#if ( configNUMBER_OF_CORES == 1 )

static void prvAddNewTaskToReadyList( TCB_t * pxNewTCB )
Expand Down Expand Up @@ -1918,12 +1919,12 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
/* If the task is running (or yielding), we must add it to the
* termination list so that an idle task can delete it when it is
* no longer running. */
#if ( configNUMBER_OF_CORES == 1 )
#if ( configNUMBER_OF_CORES == 1 ) /*_RB_ How can we combine the two places where this code block differ depending on the configNUMBER_OF_CORES setting? */
if( pxTCB == pxCurrentTCB )
#else
if( pxTCB->xTaskRunState != taskTASK_NOT_RUNNING )
#endif
{
{ /*_RB_ Is the below comment correct if one task deletes a running task on a different core? */
/* A running task is being deleted. This cannot complete within the
* task itself, as a context switch to another task is required.
* Place the task in the termination list. The idle task will
Expand All @@ -1946,7 +1947,7 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
* hence xYieldPending is used to latch that a context switch is
* required. */
#if ( configNUMBER_OF_CORES == 1 )
portPRE_TASK_DELETE_HOOK( pxTCB, &xYieldPendings[ 0 ] );
portPRE_TASK_DELETE_HOOK( pxTCB, &xYieldPendings[ 0 ] ); /*_RB_ Please bracked to make operator precidence explicit, i.e. &( xYieldPendings[ 0 ] ). Sme for line below. */
#else
portPRE_TASK_DELETE_HOOK( pxTCB, &xYieldPendings[ pxTCB->xTaskRunState ] );
#endif
Expand All @@ -1962,7 +1963,7 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
}
}

#if ( configNUMBER_OF_CORES == 1 )
#if ( configNUMBER_OF_CORES == 1 ) /*_RB_ As per other comments, I would like to remove the need for the pre-processor check, perhaps by having things like portGET_CORE_ID() available in the single core version too. Single core versions could just define as a constant to prevent additional code generation. */
{
taskEXIT_CRITICAL();

Expand Down Expand Up @@ -2096,7 +2097,7 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode,
* have put ourselves to sleep. */
if( xAlreadyYielded == pdFALSE )
{
#if ( configNUMBER_OF_CORES == 1 )
#if ( configNUMBER_OF_CORES == 1 ) /*_RB_ Another place where I think the single and multicore versions could be useding the same function or macro. */
portYIELD_WITHIN_API();
#else
vTaskYieldWithinAPI();
Expand Down