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

Revert "kernel: init: activate FPU for main thread" #31763

Merged

Conversation

ioannisg
Copy link
Member

@ioannisg ioannisg commented Jan 28, 2021

Activating K_FP_REGS flags introduces additional stack memory overhead for the main thread in Cortex-M architecture.
Several ARM platforms experience main thread stack overflows when building with FPU_SHARING=y. Enabling FPU sharing in main thread should not be the default configuration. Users are welcome to enable FP usage on the main thread in the
application code, in main(). [Actually, for Cortex-M this is done seamlessly.] There are use cases where ARM developers want to use the FPU in their own application-threads, so there is not need to waste extra memory on the main thread.

This reverts commit 8453a73.

Signed-off-by: Ioannis Glaropoulos Ioannis.Glaropoulos@nordicsemi.no

Activating K_FP_REGS flags introduces stack memory
overhead for the main thread in Cortex-M architecture.
Several ARM platforms experience main thread stack
overflows when building with FPU_SHARING=y.
Enabling FPU sharing in main thread should not be
the default configuration. Users are welcome to
enable FP sharing on the main thread in the
application code, in main().

This reverts commit 8453a73.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg ioannisg added the bug The issue is a bug, or the PR is fixing a bug label Jan 28, 2021
@ioannisg ioannisg requested a review from andyross as a code owner January 28, 2021 20:50
@ioannisg ioannisg added this to the v2.5.0 milestone Jan 28, 2021
@pabigot
Copy link
Collaborator

pabigot commented Jan 31, 2021

Users are welcome to enable FP usage on the main thread in the application code, in main(). [Actually, for Cortex-M this is done seamlessly.]

Can you elaborate on how this is done? It's not immediately obvious to me how the thread options can be changed after the thread has been created.

@katsuster
Copy link
Member

I planned to add k_float_enable() at PR #31816 to enable FPU on main thread and other system threads.
I'm happy if you give ideas about this issue...

@ioannisg
Copy link
Member Author

ioannisg commented Feb 1, 2021

Users are welcome to enable FP usage on the main thread in the application code, in main(). [Actually, for Cortex-M this is done seamlessly.]

Can you elaborate on how this is done? It's not immediately obvious to me how the thread options can be changed after the thread has been created.

@pabigot this is architecture-specific. Please, check the full floating point services documentation here: https://docs.zephyrproject.org/1.12.0/kernel/other/float.html

In brief: for Cortex-M, tagging any thread with K_FP_REGS only affects the configuration of the MPU Stack Guard size, otherwise, it does not matter at all. Users may simply start using the floating point registers at their will - then the thread becomes FPU-using. The point of my revert is that by default the Main thread should not be pre-tagged to save 96 bytes from the thread stack memory; most of the time, users will want to enable FPU calculations in their application threads.

In x86 threads can still start using the FPU regardless of pre-tagging; this triggers an exception, AFAIK, and that exception will take care of enabling FPU. I don't remember the details of the other ARCHes.

To answer your question, options are not changed, easily, but it does not seem to matter.

FYI there are some related PRs to this one, that make the FP support even better

@ioannisg ioannisg requested review from tejlmand and carlescufi and removed request for andrewboie February 1, 2021 10:58
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

To answer your question, options are not changed, easily, but it does not seem to matter.

I understand that we don't yet have k_float_enable() to match the existing (but not present in generated documentation) k_float_disable, so there is currently no way to enable the FPU in the main thread.

There certainly are common application designs where the main thread does most of the active work, and some of us would be surprised if it didn't "just work" after having configuring the application to support floating point.

So I think I like what #30455 did, and can't support removing this behavior at this time, though I also don't hold that opinion strongly enough to prevent this from going through.

@nashif
Copy link
Member

nashif commented Feb 1, 2021

@ioannisg another option is to enable this in main via a new kconfig(CONFIG_ENABLE_FPU_IN_MAIN), so that it is disabled by default and can be enabled by the application.

@ioannisg
Copy link
Member Author

ioannisg commented Feb 1, 2021

Well,

@ioannisg another option is to enable this in main via a new kconfig(CONFIG_ENABLE_FPU_IN_MAIN), so that it is disabled by default and can be enabled by the application.

I am fine with that, of course, except that we add yet another Kconfig option. So, instead I'm giving it a last try to go without it.

I understand that we don't yet have k_float_enable() to match the existing (but not present in generated documentation) k_float_disable, so there is currently no way to enable the FPU in the main thread.

k_float_enable exists for x86 and also we do have arch_float_enable() for ARC and RiscV. ARM does not need it. So I wonder, what the objections are, against reverting this patch and setting a rule that in main thread FPU should be enabled by user code. Particularly, since the RiscV maintainer has approved (thinking that the original patch was sent for the riscv architecture, anyway).

There certainly are common application designs where the main thread does most of the active work, and some of us would be surprised if it didn't "just work" after having configuring the application to support floating point.

Though I agree that many applications would like the main thread to do FP operations, i thikn it cannot be added by default, since

  • it comes with a non-negligible cost
  • there are ways for the user to do FP enabling in all architectures, that don't require the pre-tagging

@ioannisg
Copy link
Member Author

ioannisg commented Feb 1, 2021

@ioannisg another option is to enable this in main via a new kconfig(CONFIG_ENABLE_FPU_IN_MAIN), so that it is disabled by default and can be enabled by the application.

@nashif another reason, this solution is not ideal, is the fact that it is not clear what this option means. E.g. in ARM we won't ever want to set CONFIG_ENABLE_FPU_IN_MAIN, i.e. to set K_FP_REGS=1 for main at boot, although we can certainly enable FPU in main at runtime regardless of K_FP_REGS.

@nashif nashif merged commit 40aab32 into zephyrproject-rtos:master Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Kernel bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants