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

CONFIG_MULTITHREADING=n builds call main() with interrupts locked #8393

Closed
carlescufi opened this issue Jun 14, 2018 · 39 comments
Closed

CONFIG_MULTITHREADING=n builds call main() with interrupts locked #8393

carlescufi opened this issue Jun 14, 2018 · 39 comments
Assignees
Labels
area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Milestone

Comments

@carlescufi
Copy link
Member

For consistency with multithreaded builds, main() should be called with interrupts unlocked and ready to go.

@carlescufi carlescufi added bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug area: Kernel labels Jun 14, 2018
@andyross
Copy link
Contributor

The root cause here is that where the default Zephyr thread startup sets the lock state to a known value inside arch code, where we are at the end of _Cstart() is unspecified. We never took a lock, so we can't release it.

There needs to be an arch hook for "enable interrupts unconditionally", either a new API call or a special key value that can be passed to irq_unlock().

@carlescufi carlescufi added priority: high High impact/importance bug and removed priority: medium Medium impact/importance bug labels Jun 15, 2018
@carlescufi
Copy link
Member Author

Raising priority because this cannot be easily worked around on the application code in a way that is cross-arch and clean.

@carlescufi
Copy link
Member Author

@agross-linaro @andyross I also notice that even when enabling interrupts I see behavior that is not explained by my code. Could it be that the ISR wrapper in https://github.com/zephyrproject-rtos/zephyr/blob/master/arch/arm/core/isr_wrapper.S#L42 also needs conditional compilation based on CONFIG_MULTITHREADING?
Right now this is completely preventing proper use of MCUboot.

@agross-oss
Copy link
Collaborator

If no multithreading then yeah this shouldn't be setting the context switch.

@carlescufi
Copy link
Member Author

@agross-linaro I've disabled CONFIG_SYS_POWER_MANAGEMENT and CONFIG_PREEMPT_ENABLE and I believe that should compile out all of the code we don't need, though confirmation would be appreciated.

carlescufi added a commit to carlescufi/mcuboot that referenced this issue Jun 15, 2018
Due to an issue described here:
zephyrproject-rtos/zephyr#8393
interrupts are not enabled when multithreading is disabled.
Enable interrupts to allow the serial recovery mode UART to receive
characters.

Note: This commit must be reverted once the issue is addressed.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
carlescufi added a commit to carlescufi/mcuboot that referenced this issue Jun 16, 2018
Due to an issue described here:
zephyrproject-rtos/zephyr#8393
interrupts are not enabled when multithreading is disabled.
Enable interrupts to allow the serial recovery mode UART to receive
characters.

Note: This commit must be reverted once the issue is addressed.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
carlescufi added a commit to carlescufi/mcuboot that referenced this issue Jun 18, 2018
Due to an issue described here:
zephyrproject-rtos/zephyr#8393
interrupts are not enabled when multithreading is disabled.
Enable interrupts to allow the serial recovery mode UART to receive
characters.

Note: This commit must be reverted once the issue is addressed.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
carlescufi added a commit to carlescufi/mcuboot that referenced this issue Jun 18, 2018
Due to an issue described here:
zephyrproject-rtos/zephyr#8393
interrupts are not enabled when multithreading is disabled.
Enable interrupts to allow the serial recovery mode UART to receive
characters.

Note: This commit must be reverted once the issue is addressed.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
carlescufi added a commit to carlescufi/mcuboot that referenced this issue Jun 18, 2018
Due to an issue described here:
zephyrproject-rtos/zephyr#8393
interrupts are not enabled when multithreading is disabled.
Enable interrupts to allow the serial recovery mode UART to receive
characters.

Note: This commit must be reverted once the issue is addressed.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
@carlescufi
Copy link
Member Author

Added an MCUboot issue to be resolved once this one is closed: mcu-tools/mcuboot#302

carlescufi added a commit to mcu-tools/mcuboot that referenced this issue Jun 20, 2018
Due to an issue described here:
zephyrproject-rtos/zephyr#8393
interrupts are not enabled when multithreading is disabled.
Enable interrupts to allow the serial recovery mode UART to receive
characters.

Note: This commit must be reverted once the issue is addressed.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
@andyross andyross added priority: medium Medium impact/importance bug and removed priority: high High impact/importance bug labels Jun 28, 2018
@andyross
Copy link
Contributor

Drop priority as there's an active workaround and it's not blocking work. It remains a real bug and needs a fix for 1.13.

@nashif
Copy link
Member

nashif commented Aug 26, 2018

@andyross ping

@nashif nashif added this to the v1.13.0 milestone Aug 26, 2018
andyross pushed a commit to andyross/zephyr that referenced this issue Aug 27, 2018
Some applications have a use case for a tiny MULTITHREADING=n build
(which lacks most of the kernel) but still want special-purpose
drivers in that mode that might need to handle interupts.  This
creates a chicken and egg problem, as arch code (for obvious reasons)
runs _Cstart() with interrupts disabled, and enables them only on
switching into a newly created thread context.  Zephyr does not have a
"turn interrupts on now, please" API at the architecture level.

So this creates one as an arch-specific wrapper around
_arch_irq_unlock().  It's implemented as an optional macro the arch
can define to enable this behavior, falling back to the previous
scheme (and printing a helpful message) if it doesn't find it defined.
Only ARM and x86 are enabled in this patch.

Fixes zephyrproject-rtos#8393

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
@nashif
Copy link
Member

nashif commented Jul 24, 2020

@carlescufi would be great to specify why all of this is needed, are we still going after the few kilobytes from the kernel or is there some other rationale for having this supported?

@carlescufi
Copy link
Member Author

carlescufi commented Jul 24, 2020

@carlescufi would be great to specify why all of this is needed, are we still going after the few kilobytes from the kernel or is there some other rationale for having this supported?

The idea is to be able to use Zephyr as an almost "bare-metal" development platform. Threads and threading can be expensive in RAM and ROM footprint, but Zephyr provides a lot of value beyond its RTOS underpinnings:

  • A flexible build and configuration system
  • A hardware description language along with an API to access it
  • Platform and architecture-agnostic primitives for basic operations such as booting and handling interrupts
  • In some cases (flash drivers are a good example) basic driver functionality under a single API

I personally believe that maintaining the possibility of using Zephyr in this way it's added value at a (relative) low cost for the project, so I would be very much in favor of formally defining the requirements and limitations and then officially supporting this "feature".

@carlescufi
Copy link
Member Author

which is 18.4% at 5836 bytes.

Hmm, I measure 4290 bytes for the whole kernel above, so most of this overhead is probably coming from somewhere else and when multithreading is enabled, probably thru logging. With log minimal enable I get;

[109/114] Linking C executable zephyr/zephyr_prebuilt.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       33780 B        48 KB     68.73%
            SRAM:       25120 B       256 KB      9.58%
        IDT_LIST:          88 B         2 KB      4.30%

There are some additional benchmarks here.

pabigot pushed a commit to pabigot/zephyr that referenced this issue Jul 25, 2020
Some applications have a use case for a tiny MULTITHREADING=n build
(which lacks most of the kernel) but still want special-purpose
drivers in that mode that might need to handle interupts.  This
creates a chicken and egg problem, as arch code (for obvious reasons)
runs _Cstart() with interrupts disabled, and enables them only on
switching into a newly created thread context.  Zephyr does not have a
"turn interrupts on now, please" API at the architecture level.

So this creates one as an arch-specific wrapper around
_arch_irq_unlock().  It's implemented as an optional macro the arch
can define to enable this behavior, falling back to the previous
scheme (and printing a helpful message) if it doesn't find it defined.
Only ARM and x86 are enabled in this patch.

Fixes zephyrproject-rtos#8393

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
@nashif nashif added priority: low Low impact/importance bug and removed priority: high High impact/importance bug labels Sep 3, 2020
@nashif
Copy link
Member

nashif commented Sep 3, 2020

single threading is scheduled for deprecation, so lower priority.

@pabigot
Copy link
Collaborator

pabigot commented Sep 3, 2020

Why keep this open? It's unlikely that the decision will change, and if it does part of the task will be to identify all the things that need to work, in which case this issue will be rediscovered. Keeping it open affects the low bug count which can now impact release.

I'd rather close all these issues and move on. Maybe discuss next triage.

@nashif
Copy link
Member

nashif commented Sep 23, 2020

clearing assignee as this feature is being deprecated, we can talk about this next and discuss if someone will step up and own this and if those issues will be assigned to them. The bug is open, but the code is still in the tree, even with deprecation. Deprecated code does not mean we stop fixing bugs.

@pabigot
Copy link
Collaborator

pabigot commented Sep 23, 2020

@ioannisg re our discussion earlier today.

@github-actions
Copy link

This issue 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 issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@carlescufi
Copy link
Member Author

reopening as this still applies

@carlescufi carlescufi reopened this Apr 8, 2021
@github-actions github-actions bot removed the Stale label Apr 9, 2021
@ioannisg
Copy link
Member

@carlescufi you reopened this, however, I am not sure if this still applies. CONFIG_MULTITHREADING=n is only maintained on Cortex-M, for now, and I can confirm that in Cortex-M this is not an issue.

@carlescufi
Copy link
Member Author

@carlescufi you reopened this, however, I am not sure if this still applies. CONFIG_MULTITHREADING=n is only maintained on Cortex-M, for now, and I can confirm that in Cortex-M this is not an issue.

I agree, as long as you confirm that this is not an issue with Cortex-M then we can close this @ioannisg

@ioannisg
Copy link
Member

ioannisg commented May 19, 2021

3b89cf1

@carlescufi ^^
We even have a unit tests that covers it. :)

@carlescufi
Copy link
Member Author

Thanks for confirming @ioannisg. Closing this now.

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 priority: low Low impact/importance bug
Projects
None yet
Development

No branches or pull requests

10 participants