Optimize xTaskIncrementTick for configNUMBER_OF_CORES > 1

[cymizer]

Description

The original implementation only initializes the first variable. After executing xTaskIncrement, the schedule might not behave as expected.

Test Steps

N/A

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

N/A

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[aggarg]

This is not C89 syntax but GNU extension which we cannot use because FreeRTOS is used with a variety of compilers. Please use a loop to initialize the array.

[jefftenney]

There is an existing loop where this change would fit nicely.

Existing Code:

for( xCoreID = 0; xCoreID < ( ( BaseType_t ) configNUMBER_OF_CORES ); xCoreID++ )
{
    if( listCURRENT_LIST_LENGTH( &( pxReadyTasksLists[ pxCurrentTCBs[ xCoreID ]->uxPriority ] ) ) > 1U )
    {
        xYieldRequiredForCore[ xCoreID ] = pdTRUE;
    }
    else
    {
        mtCOVERAGE_TEST_MARKER();
    }
}

Proposed Code:

for( xCoreID = 0; xCoreID < ( ( BaseType_t ) configNUMBER_OF_CORES ); xCoreID++ )
{
    if( listCURRENT_LIST_LENGTH( &( pxReadyTasksLists[ pxCurrentTCBs[ xCoreID ]->uxPriority ] ) ) > 1U )
    {
        xYieldRequiredForCore[ xCoreID ] = pdTRUE;
    }
    else
    {
        xYieldRequiredForCore[ xCoreID ] = pdFALSE;
    }
}

EDIT: Just reviewed the configuration options and found something unexpected. The code I propose above requires that we also resolve what seems to be a small inconsistency in the way we use configUSE_TIME_SLICING. The loop above runs only when configUSE_TIME_SLICING is 1, but the loop later that checks the array to induce the yields runs even if configUSE_TIME_SLICING is 0. That's fine of course, but the code I propose above requires that we add the extra condition (configUSE_TIME_SLICING == 1) on the later loop.

[cymizer]

hi @aggarg , @jefftenney
Thank you for your valuable suggestions. I have made the changes based on your recommendations. Kindly assist with the review.

[jefftenney]

This looks fine to me now, but can't help but wonder if we should eliminate xYieldRequiredForCore altogether. (Sorry for not seeing this earlier.) The code above that sets xYieldRequiredForCore[ xCoreID ] would just set xYieldPendings[ xCoreID ] instead, leaving the original test-coverage marker in the "else" case. Would you mind investigating that possibility? Any concerns @aggarg?

[jefftenney]

Looks good. This PR resolves the issue of the uninitialized array, and it slightly increases performance too. The only functional difference I see is for a task with preemption disabled (via vTaskPreemptionDisable()). For that task, a time-slicing yield is dropped before this PR and is delayed after this PR. We could see that as an improvement too.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[aggarg]

For that task, a time-slicing yield is dropped before this PR and is delayed after this PR.

Thanks for pointing that. It should be okay as the scheduler will not evict a task for which the preemption is disabled - https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/tasks.c#L1230.