Skip to content
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.

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

Update xTaskGetIdleTaskHandle() For SMP #868

Merged

Conversation

Dazza0
Copy link
Contributor

@Dazza0 Dazza0 commented Oct 30, 2023

Description

Currently, when SMP is enabled (i.e., configNUMBER_OF_CORES > 1), the xTaskGetIdleTaskHandle() function has a change in API by accepting an extra xCoreID function. Ideally, if SMP supports all single-core API, all existing single-core applications/libraries can be built for SMP without any modification (the kernel will just parallelize things across multiple cores transparently).

This PR updates xTaskGetIdleTaskHandle() for SMP in the following ways:

  • xTaskGetIdleTaskHandle() no longer accepts xCoreID argument in SMP so that it has the exact same API with single-core.
  • xTaskGetIdleTaskHandle() now returns the Active idle task handle in SMP, which matches the behavior in single-core.
  • Added xTaskGetIdleTaskHandleForCore() in SMP which accepts an xCoreID argument. This function can be used to obtain the Passive idle task handles.

Notes:

  • I spot a couple of other SMP breaking API changes as well (namely vApplicationGetIdleTaskMemory())
  • It might also make sense for SMP specific API to be made available for single-core builds. For example, calling xTaskCreateAffinitySet() in single-core just results an a wrapping call to xTaskCreate() with the uxCoreAffinityMask argument being dropped. This will prevent applications needing to add numerous preprocessor checks of configNUMBER_OF_CORES and allow them to be written in a "core-agnostic" manner.

Test Steps

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

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

@Dazza0 Dazza0 requested a review from a team as a code owner October 30, 2023 00:00
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0ce725) 93.64% compared to head (4701d35) 93.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #868      +/-   ##
==========================================
+ Coverage   93.64%   93.77%   +0.12%     
==========================================
  Files           6        6              
  Lines        3179     3179              
  Branches      885      885              
==========================================
+ Hits         2977     2981       +4     
+ Misses         95       91       -4     
  Partials      107      107              
Flag Coverage Δ
unittests 93.77% <100.00%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shubnil
Copy link
Member

shubnil commented Nov 2, 2023

Thanks @Dazza0 for the PR. This needs a Backward compatibility decision making, which is best taken by feature owners. I have updated the feature owners and they are having a look at it.

chinglee-iot
chinglee-iot previously approved these changes Nov 6, 2023
@chinglee-iot chinglee-iot dismissed their stale review November 9, 2023 05:22

Bring back the requirement for more discussion.

@chinglee-iot
Copy link
Member

@Dazza0

Thank you for creating this PR and pointing out some of the API compatible problems.

We would like to consider the following requirements:

  • SMP application not using SMP specific APIs should be able to be built with single-core without any modification. Single-core can be considered one of the configuration of SMP.
  • Single core application should be updated to use core-aware API ( for example, xTaskGetIdleTaskHandleForCore ) if they are going to be built with SMP. If single core application assumes the system only has one core or use core-unaware ( xTaskGetIdleTaskHandle ) API, the result of the application may not be correct.
  • SMP application using SMP specific APIs should specify alternative behavior in the application if built with single-core. The SMP specific APIs includes core affinity APIs, preemption enable/disable ...

In this PR, xTaskGetIdleTaskHandleForCore should be available in single-core as well. On the contrary, xTaskGetIdleTaskHandle is only available in single-core for backward compatibility.

I would like create a PR to your branch to share my suggestion with xTaskGetIdleTaskHandle. We would also update vApplicationGetIdleTaskMemory for SMP in another PR.

@chinglee-iot
Copy link
Member

The PR is created. Dazza0#1
Please take a look and feedback.

@Dazza0
Copy link
Contributor Author

Dazza0 commented Nov 14, 2023

@chinglee-iot Thanks for the review.

We would like to consider the following requirements:

Just to double check that I understand the current requirements, does the following table accurately describe what the single-core vs SMP API usage policy will be for FreeRTOS going forward?

App API Usage \ Kernel Config configNUMBER_OF_CORES == 1 configNUMBER_OF_CORES > 1
Uses single-core API only Standard single-core app. Same as before Not supported. Single-core apps need to be explicitly updated to call SMP API.
Uses SMP API Supported. SMP APIs need to provide single-core behavior when called Standard SMP app.

Dazza0 and others added 2 commits November 16, 2023 16:14
This commit updates xTaskGetIdleTaskHandle() for SMP in the following ways:

- xTaskGetIdleTaskHandle() no longer accepts xCoreID argument in SMP so that
there is not change in API between single-core and SMP
- xTaskGetIdleTaskHandle() now returns the Active idle task handle in SMP,
which matches the behavior in single-core.
- Added xTaskGetIdleTaskHandleForCore() in SMP which accepts an xCoreID
argument. This function can be used to obtain the Passive idle task handles.
@Dazza0 Dazza0 force-pushed the change/idle_task_handle_functions branch from a03fe67 to b3bd665 Compare November 16, 2023 08:14
@Dazza0
Copy link
Contributor Author

Dazza0 commented Nov 16, 2023

@chinglee-iot I've merged Dazza0#1. Could you please verify if the table I posted accurately describes the SMP API requirements?

@chinglee-iot
Copy link
Member

@chinglee-iot Thanks for the review.

We would like to consider the following requirements:

Just to double check that I understand the current requirements, does the following table accurately describe what the single-core vs SMP API usage policy will be for FreeRTOS going forward?
App API Usage \ Kernel Config configNUMBER_OF_CORES == 1 configNUMBER_OF_CORES > 1
Uses single-core API only Standard single-core app. Same as before Not supported. Single-core apps need to be explicitly updated to call SMP API.
Uses SMP API Supported. SMP APIs need to provide single-core behavior when called Standard SMP app.

Thank you for the table. It is correct and clear to me.

@Saiiijchan
Copy link
Contributor

Excuse me, I am confused about idle task and passive idle task.
The idle task is located on xIdleTaskHandles[0] and passive ilde tasks are located on xIdleTaskHandles[1]...xIdleTaskHandles[coreNumer-1]. Whether it is an idle task or a passive idle task, there is no affinity on any core.
Why return xIdleTaskHandles[ portGET_CORE_ID() ] can get the idle task on the running core? xIdleTaskHandles[0] can run on core1 and xIdleTaskHandles[1] can run on core0.
return xIdleTaskHandles[ portGET_CORE_ID() ];

@chinglee-iot
Copy link
Member

The idle task is located on xIdleTaskHandles[0] and passive ilde tasks are located on xIdleTaskHandles[1]...xIdleTaskHandles[coreNumer-1]. Whether it is an idle task or a passive idle task, there is no affinity on any core.

This is correct. Currently, the idle tasks don't have affinity on cores.

Why return xIdleTaskHandles[ portGET_CORE_ID() ] can get the idle task on the running core? xIdleTaskHandles[0] can run on core1 and xIdleTaskHandles[1] can run on core0.
return xIdleTaskHandles[ portGET_CORE_ID() ];

The implementation uses the xCoreID parameter as index.

    TaskHandle_t xTaskGetIdleTaskHandleForCore( BaseType_t xCoreID )
    {
        traceENTER_xTaskGetIdleTaskHandleForCore( xCoreID );

        /* Ensure the core ID is valid. */
        configASSERT( taskVALID_CORE_ID( xCoreID ) == pdTRUE );

        /* If xTaskGetIdleTaskHandle() is called before the scheduler has been
         * started, then xIdleTaskHandles will be NULL. */
        configASSERT( ( xIdleTaskHandles[ xCoreID ] != NULL ) );

        traceRETURN_xTaskGetIdleTaskHandleForCore( xIdleTaskHandles[ xCoreID ] );

        return xIdleTaskHandles[ xCoreID ];
    }

Can you help to point out the line of code you mentioned?

@chinglee-iot
Copy link
Member

@Dazza0
I create a PR FreeRTOS/FreeRTOS#1119 to fix and add new unit test cases to cover the xTaskGetIdleTaskHandleForCore() and will continue to have this PR merged.

@Saiiijchan
Copy link
Contributor

xTaskGetIdleTaskHandleForCore

Hi @chinglee-iot
Maybe I misunderstand xTaskGetIdleTaskHandleForCore( xCoreID ), I thought it got the handler of running idle task on xCoreID. So it just gets xIdleTaskHandles[ xCoreID ]?

@chinglee-iot
Copy link
Member

@Saiiijchan
Yes, this API returns the idle task handle only. Set xCoreID to 0 to get active idle task handle and 1 ~ ( confignUMBER_OF_CORES - 1 ) to get passive idle task handles. You can get the running state of the idle task with other API like eTaskGetState.

Copy link

sonarqubecloud bot commented Dec 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@chinglee-iot chinglee-iot merged commit cd5c774 into FreeRTOS:main Dec 5, 2023
17 checks passed
@Dazza0 Dazza0 deleted the change/idle_task_handle_functions branch February 1, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants