Skip to content

Conversation

@danieldegrasse
Copy link
Contributor

@danieldegrasse danieldegrasse commented Jul 11, 2023

Enable relocation of key networking stack functions to RAM when running
the zperf example. This will enable better performance on platforms with
fast code RAM.

Some performance numbers using this PR:

Board Zephyr Revision Code location TCP RX TCP TX UDP RX UDP TX
RT1170 25c6553 QuadSPI 18.4Mbps 12.8Mbps 38.59Mbps 24.2Mbps
RT1170 feature/zperf_relocate ITCM/QuadSPI 94.0Mbps 69.4Mbps 89.03Mbps 76.7Mbps
RT1060 25c6553 QuadSPI 34.5Mbps 29.0Mbps 80.88Mbps 65.6Mbps
RT1060 feature/zperf_relocate ITCM/QuadSPI 93.8Mbps 70.6Mbps 89.03Mbps 73.2Mbps
RT1050 25c6553 HyperFlash 57.4Mbps 55.9Mbps 89.03Mbps 73.1Mbps
RT1050 feature/zperf_relocate ITCM/Hyperflash 94.0Mbps 71.1Mbps 89.03Mbps 73.0Mbps

Signed-off-by: Daniel DeGrasse [email protected]

@zephyrbot zephyrbot requested a review from ssharks July 11, 2023 21:13
@zephyrbot zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Jul 11, 2023
@zephyrbot
Copy link

zephyrbot commented Jul 11, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@danieldegrasse
Copy link
Contributor Author

The other alternative to this I have considered is something similar to #50792, where the network relocation options are integrated into the subsystem. My reasoning for keeping the relocation directives within the zperf sample is that a user tuning their application for performance will likely want to relocate a specific subset of the networking stack, and I feel that the decision of which portions to relocate is generally an application one.

One unfortunate side effect of this is that the NXP HAL now uses an application specific Kconfig to determine if the MCUX Ethernet driver should be relocated to RAM, which is required in order to realize the performance numbers within this PR.

rlubos
rlubos previously approved these changes Jul 12, 2023
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert on code relocation, but changes in the sample look ok.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Sep 11, 2023
Copy link
Contributor

@ssharks ssharks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never worked with code relocation, but it looks like a valuable addition. It would be a said if it would be lost.
Could you look at the reference to the NXP hal, after a rebase we should be good to proceed.

west.yml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look like an upstreamable change, could you use a hash here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not upstreamable- the relevant PR needs to be merged first

The reason this change is currently stalled is that we need to relocate the NXP HAL driver to RAM as well as the in tree ethernet driver code, and we do not want to make the relocation directive in the HAL dependent on this application defined KConfig.

@github-actions github-actions bot removed the Stale label Sep 12, 2023
@carlescufi
Copy link
Member

@danieldegrasse if you'd like this into 3.5 then please let me know on Discord so we can merge the HAL PR

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Nov 28, 2023
@zephyrbot zephyrbot added the area: Samples Samples label Jan 24, 2024
@zephyrbot zephyrbot removed manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Jan 24, 2024
@danieldegrasse
Copy link
Contributor Author

Updated this PR onto latest main, and removed the HAL changes. The HAL change does enable the Ethernet driver code to be relocated to ITCM, which improves performance further, so here are new performance numbers without this change:

Board Zephyr Revision Code location TCP RX TCP TX UDP RX UDP TX
RT1170 feature/zperf_relocate ITCM/QuadSPI 56Mbps 34.26Mbps 78.82Mbps 76.7Mbps
RT1060 feature/zperf_relocate ITCM/QuadSPI 72.1Mbps 55.9Mbps 88.78Mbps 75.1Mbps
RT1050 feature/zperf_relocate ITCM/Hyperflash 66.0Mbps 44.1Mbps 88.91Mbps 75.0 Mbps

pdgendt
pdgendt previously approved these changes Jan 25, 2024
Enable relocation of key networking stack functions to RAM when running
the zperf example. This will enable better performance on platforms with
fast code RAM.

Signed-off-by: Daniel DeGrasse <[email protected]>
Relocate network stack to RAM for iMX RT boards when running the zperf
sample. Relocating the network stack to ITCM greatly improves
performance on these platforms.

Also, remove overlays for the RT1050 and RT1060 setting the system timer
to systick. This is no longer required as this is the default timer for
these boards.

Signed-off-by: Daniel DeGrasse <[email protected]>
@henrikbrixandersen henrikbrixandersen merged commit 664e710 into zephyrproject-rtos:main Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants