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

Bluetooth: Controller: Support Link Time Optimizations (LTO) #70251

Merged
merged 4 commits into from
May 22, 2024

Conversation

cvinayak
Copy link
Contributor

@cvinayak cvinayak commented Mar 14, 2024

Support for using Link Time Optimization (LTO) when
building application with open source Bluetooth Low
Energy Controller. This reduces code space usage.

Use Link Time Optimisation (LTO) when building Controller
with Isochronous channels support.

nRF53x SoCs has 256KB flash and using speed optimisation,
which is the default today, overflows flash when adding
new features related to ISO support.

Signed-off-by: Vinayak Kariappa Chettimada [email protected]

@cvinayak cvinayak marked this pull request as ready for review March 14, 2024 21:40
@cvinayak cvinayak added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Mar 14, 2024
kruithofa
kruithofa previously approved these changes Mar 15, 2024
aescolar
aescolar previously approved these changes Mar 15, 2024
@henrikbrixandersen
Copy link
Member

Is this really the way we want to go? Certain subsystems enabling LTO by default, other don't?

@cvinayak
Copy link
Contributor Author

Is this really the way we want to go? Certain subsystems enabling LTO by default, other don't?

LTO has been introduced recently and not enabled for default across the builds probably for a reason? Atleast, for the Bluetooth Controller included builds, i am not observing any regression in my (may be limited) manual testing.

Suggestions welcome if we want all builds to be Link Time Optimized.

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

I think it's too early to automatically enable LTO, even for a subsystem. We are still seeing issues like #70305

@cvinayak cvinayak marked this pull request as draft March 19, 2024 04:12
@nordicjm
Copy link
Collaborator

Is this really the way we want to go? Certain subsystems enabling LTO by default, other don't?

Users can enable the setting if they want it manually, I don't really see the value in having it automatically set. Which, I would assume, would then cause problems if they use a debug build and try to step through the bluetooth code

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

I think it's too early to automatically enable LTO, even for a subsystem. We are still seeing issues like #70305

I agree, considering the complexity of Zephyr, the need for whole-archive, link post-processing, etc. then I think there are too many variables to enable this per-default.

Although this has been tested for a single sub-system, then what happens when that subsystem is combined with the myriad of other options / subsystems / SoCs which may not have been verified to work properly with LTO ?

Also, having this config option inside a sub-system can give the false impression that the setting only applies to part of the build, which is not the case.

Technically, if using partial linking of subsystems before a final linking it would be possible to apply lto on just part of the linking (gcc/ld) on a subsystem, but that's not what this setting does, and would be a completely different task 🙂 .

Comment on lines 586 to 606
config BT_CTLR_LTO
bool "Link Time Optimization"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a good location for this setting.

It may give the impression that LTO is only applied on this subsystem, which is actually not the case.
Enabling LTO here will enable LTO for the Zephyr build.

One may say, of course, that should be obvious, but in Zephyr we are actually having parts which could be using LTO while other parts would not be using it, for example for the TF-M secure build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now added depends on ARM && BT_HCI_RAW, this will restrict LTO to samples/application that build only the controller subsystem (aka hci_uart, hci_ipc... and so on) with a HCI transport.

And on the hci_ipc for now needs reduced code space usage in-order to support ISO features and be able to have spare space for DFU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed && BT_HCI_RAW as normal samples/application would like to use LTO.

@cvinayak cvinayak dismissed stale reviews from aescolar and kruithofa via c32dc9e March 19, 2024 08:32
@cvinayak cvinayak force-pushed the github_bt_clr_lto branch 2 times, most recently from c32dc9e to 63c627f Compare March 19, 2024 08:39
@cvinayak cvinayak marked this pull request as ready for review March 19, 2024 08:43
@zephyrbot zephyrbot added area: Samples Samples area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Mar 19, 2024
@cvinayak cvinayak changed the title Bluetooth: Controller: Default to Link Time Optimizations (LTO) Bluetooth: Controller: Support Link Time Optimizations (LTO) Mar 19, 2024
@cvinayak cvinayak force-pushed the github_bt_clr_lto branch from 2521a0e to d17a985 Compare April 29, 2024 04:48
jhedberg
jhedberg previously approved these changes Apr 29, 2024
@@ -598,12 +598,29 @@ config BT_CTLR_DYNAMIC_INTERRUPTS
permit use of SoC's peripheral for custom use when Bluetooth is not
enabled.

choice
prompt "Optimazation options"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a typo snuck in here: "Optimazation" -> "Optimization"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, just realized that moving to choice means I need to also have the optimize for space which would be the alternative when optimize for speed need to be disabled (and no LTO) for the Controller.

Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

There are some interdependencies that looks messy to me.

Currently it's possible to set LTO and BT_CTLR_OPTIMIZE_FOR_SPEED

@cvinayak cvinayak force-pushed the github_bt_clr_lto branch 3 times, most recently from 90d01e9 to 59dd08c Compare April 29, 2024 09:44
@cvinayak cvinayak force-pushed the github_bt_clr_lto branch 2 times, most recently from 894d284 to fecb9fa Compare April 29, 2024 12:07
Thalley
Thalley previously approved these changes Apr 29, 2024
@cvinayak
Copy link
Contributor Author

cvinayak commented May 2, 2024

@henrikbrixandersen and @tejlmand please re-review.

@henrikbrixandersen and @tejlmand Any chance you can take a second look as to if the concerns are addressed?

@cvinayak cvinayak removed the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label May 15, 2024
Remove legacy LL optimize for speed dependency required to
support encryption feature in Controller where crypto was
performed in highest priority ISRs to setup encryption in 3
radio events (this now requires 5 radio events in split LL).

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@cvinayak cvinayak dismissed stale reviews from henrikbrixandersen and Thalley via 5d40111 May 15, 2024 10:27
@cvinayak cvinayak force-pushed the github_bt_clr_lto branch from fecb9fa to 5d40111 Compare May 15, 2024 10:27
cvinayak added 3 commits May 15, 2024 16:27
Support for using Link Time Optimization (LTO) when
building application with open source Bluetooth Low
Energy Controller. This reduces code space usage.

Current Memory Usage at the time of this PR (hci_ipc):

BT_CTLR_OPTIMIZE_FOR_SPEED:
Memory region         Used Size  Region Size  %age Used
           FLASH:      260112 B       256 KB     99.22%
             RAM:       60136 B        64 KB     91.76%
           SRAM1:           0 B        64 KB      0.00%
        IDT_LIST:           0 B        32 KB      0.00%

BT_CTLR_OPTIMIZE_FOR_SIZE:
Memory region         Used Size  Region Size  %age Used
           FLASH:      232292 B       256 KB     88.61%
             RAM:       60128 B        64 KB     91.75%
           SRAM1:           0 B        64 KB      0.00%
        IDT_LIST:           0 B        32 KB      0.00%

BT_CTLR_OPTIMIZE_FOR_APP_DEFAULT:
Memory region         Used Size  Region Size  %age Used
           FLASH:      232292 B       256 KB     88.61%
             RAM:       60128 B        64 KB     91.75%
           SRAM1:           0 B        64 KB      0.00%
        IDT_LIST:           0 B        32 KB      0.00%

BT_CTLR_LTO:
Memory region         Used Size  Region Size  %age Used
           FLASH:      221484 B       256 KB     84.49%
             RAM:       60120 B        64 KB     91.74%
           SRAM1:           0 B        64 KB      0.00%
        IDT_LIST:           0 B        32 KB      0.00%

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Added build time coverage for added new Controller
optimization related Kconfigs.

New Kconfigs are either covered in samples.yaml or in the
tests/bluetooth/init conf files.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Use Link Time Optimisation (LTO) when building Controller
with Isochronous channels support.

nRF53x SoCs has 256KB flash and using speed optimisation,
which is the default today, overflows flash when adding
new features related to ISO support.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@cvinayak cvinayak force-pushed the github_bt_clr_lto branch from 5d40111 to 4ad6817 Compare May 15, 2024 14:30
@dleach02 dleach02 merged commit bd3a2f5 into zephyrproject-rtos:main May 22, 2024
24 checks passed
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.