kernel: remove single thread support #10275

dcpleung · 2018-09-27T23:14:06Z

This removes single thread supports. Changes are done to
remove CONFIG_MULTITHREADING and #ifdef enclosed by it.

This includes the following reverts which can be trivially
applied:

Revert "drivers: flash: w25qxxdv: Avoid locking when not threaded"
This reverts commit 392da5b.

Revert "drivers: flash: nrf: Avoid locking when not threaded"
This reverts commit 408ea14.

Revert "tests/kernel/threads/no-multithreading: Disable USERSPACE"
This reverts commit 2f95e24.

Revert "tests/kernel: Add a test for CONFIG_MULTITHREADING=n"
This reverts commit 7bbd3a7.

Revert "kernel: Final spin in !MULTITHREADING should be locked"
This reverts commit 8daafd4.

Revert "kernel: Restore CONFIG_MULTITHREADING=n behavior"
This reverts commit 3d14615.

Revert "ext: simplelink: host driver: depend on multithreading"
This reverts commit 4d912b0.

Relates to #9808

Signed-off-by: Daniel Leung daniel.leung@intel.com

codecov-io · 2018-09-27T23:43:42Z

Codecov Report

Merging #10275 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10275      +/-   ##
==========================================
+ Coverage   53.05%   53.06%   +<.01%     
==========================================
  Files         214      214              
  Lines       26201    26196       -5     
  Branches     5645     5645              
==========================================
- Hits        13901    13900       -1     
+ Misses      10028    10024       -4     
  Partials     2272     2272

Impacted Files	Coverage Δ
kernel/include/ksched.h	`94.25% <ø> (ø)`	⬆️
kernel/init.c	`74.57% <ø> (+1.99%)`	⬆️
kernel/thread.c	`93.46% <ø> (ø)`	⬆️
kernel/sched.c	`91.94% <ø> (+0.33%)`	⬆️
include/kernel.h	`98.52% <ø> (ø)`	⬆️
lib/thread_entry.c	`75% <ø> (+15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10c6a0c...85eb3bb. Read the comment docs.

This removes single thread supports. Changes are done to remove CONFIG_MULTITHREADING and #ifdef enclosed by it. This includes the following reverts which can be trivially applied: Revert "drivers: flash: w25qxxdv: Avoid locking when not threaded" This reverts commit 392da5b. Revert "drivers: flash: nrf: Avoid locking when not threaded" This reverts commit 408ea14. Revert "tests/kernel/threads/no-multithreading: Disable USERSPACE" This reverts commit 2f95e24. Revert "tests/kernel: Add a test for CONFIG_MULTITHREADING=n" This reverts commit 7bbd3a7. Revert "kernel: Final spin in !MULTITHREADING should be locked" This reverts commit 8daafd4. Revert "kernel: Restore CONFIG_MULTITHREADING=n behavior" This reverts commit 3d14615. Revert "ext: simplelink: host driver: depend on multithreading" This reverts commit 4d912b0. Relates to zephyrproject-rtos#9808 Signed-off-by: Daniel Leung <daniel.leung@intel.com>

dcpleung · 2018-09-28T07:42:51Z

Do not merge yet. There is a xtensa_asm2 sample app which requires single thread. Need to figure out what to do with it.

carlescufi · 2018-09-28T12:37:34Z

@dcpleung

Thanks for looking into this.

MCUboot uses this option to save on ROM space.
Before we merge this, please build MCUboot and let's make sure we do not add to the ROM footprint. Instructions for building with Zephyr are here.
I suggest using nrf52_pca10040 as a board.

There are several current applications that require the bootloader to fit in 16KB of flash. @sjanc is currently developing one.

Also copied the relevant people: @nvlsianpu and @d3zd3z

nvlsianpu · 2018-09-28T15:20:04Z

@carlescufi Thanks for info. will check things on Monday.

andyross

No complaints about removing the feature, given that the first user ran into bumps with wanting to have drivers work in the mode, which wasn't the original intent.

Still, there are a bunch of bytes to save in code size if you know you will never swap. Even if that's bitrotten currently, Ben had the zephyr minimal kernel down below something like 3kb when it was committed.

Basically, we can remove this now, but I'll bet we want it back at some point. Though doing it again from scratch is probably no more work than fixing what we have.

nashif · 2018-09-28T15:52:33Z

16KB of flash

really? we have mcuboot now building in < 16kb flash? AFAIK when this was introduced we almost got it to fit just below 20k

andyross · 2018-09-28T15:54:36Z

Oh, and you can junk the asm2 thing. That's a testbed for the very lowest level of the Xtensa platform integration. When I did the same thing for x86 I split it out into a separate layer with its own makefile, which worked better. Making that work in the context of a Zephyr build was, in hindsight, more work than it was worth.

And there's no loss of test coverage if you junk it. Needless to say we hit all that code in the context of the other tests by definition. It's just a unit test.

carlescufi · 2018-09-28T16:02:57Z

really? we have mcuboot now building in < 16kb flash? AFAIK when this was introduced we almost got it to fit just below 20k

we do, at least for Nordic targets. @sjanc can give you the exact ROM footprint.

dcpleung · 2018-09-28T17:28:06Z

I tried building mcuboot with board nrf52_pca10040, and there was about 3KB increase in flash size. According to the build output, before this patch, flash used 29096 bytes (and SRAM 22444 bytes). After applying this patch, the flash used 32152 bytes (and SRAM 22796 bytes).

carlescufi · 2018-09-28T17:35:15Z

@dcpleung 3KB is a lot of flash in the Nordic range of devices. Is there something we can do to compensate here?

dcpleung · 2018-09-28T18:53:00Z

I compared the .config files and set the following in mcuboot's prj.conf:

CONFIG_TIMESLICING=n
CONFIG_SIMPLE_FATAL_ERROR_HANDLER=y
CONFIG_NUM_PREEMPT_PRIORITIES=0
CONFIG_NUM_COOP_PRIORITIES=1

However, the flash size is still 2KB larger at 31540 bytes.

Removing sem_lock and k_sem_* function calls in drivers/flash/soc_flash_nrf.c results in built binary with flash size of 30792 bytes (less 748 bytes). The remaining additions are all from the thread initialization and scheduler functions. Those adds up to about 1.6K according to the output of readelf.

carlescufi

Temporarily nacking until we have a better understanding of the consequences of merging this for MCUboot.

sjanc · 2018-09-29T12:31:55Z

Hi, yes we use that option for our bootloader build

currently what we have is
Memory region Used Size Region Size %age Used
FLASH: 14064 B 16 KB 85.84%

If we enable CONFIG_MULTITHREADING=y this numbers grow to
FLASH: 17100 B 16 KB 104.37%

So we would require extra flash page just for bootloader.

sjanc

we must fit 16k

nvlsianpu

As @sjanc said
Mcuboot must fit in 16 KB.

nashif · 2018-10-01T13:58:35Z

yes we use that option for our bootloader build

Is the configuration you are using available somewhere?

sjanc · 2018-10-01T14:55:37Z

We basically disabled everything we could

Disable console and logs

CONFIG_CONSOLE=n
CONFIG_SYS_LOG=n
CONFIG_BOOT_HAVE_LOGGING=n
CONFIG_DEBUG=n

Disable not needed OS features

CONFIG_SYSTEM_CLOCK_DISABLE=y
CONFIG_SYS_POWER_MANAGEMENT=n
CONFIG_ARM_CORE_MPU=n
CONFIG_ARM_MPU_ENABLE=n
CONFIG_ARM_MPU_NRF52X=n
CONFIG_MULTITHREADING=n

CONFIG_MAIN_STACK_SIZE=10240
CONFIG_BOOT_SIGNATURE_TYPE_RSA=n
CONFIG_BOOT_SIGNATURE_TYPE_ECDSA_P256=y
CONFIG_FLASH=y

And I'd rather target 12k in future, not other way around :-)

nvlsianpu · 2018-10-02T13:04:05Z

@sjanc - can you attach .config file you have while compiling the minimal mcuboot on nrf52?

d3zd3z · 2018-10-10T20:05:15Z

This change is pretty much a show stopper for some MCUboot uses, at least from what it looks like with the code size.

There really is a need to be able to use Zephyr more as a BSP for a bare-metal application (such as a bootloader) than as an RTOS. Realistically, if this merges, small constrained devices would have to use another solution (such as MyNewt) to be able to build the bootloader.

d3zd3z · 2018-10-10T20:08:41Z

Also, this would prevent us from being able to build the secure side of a trusted firmware solution using Zephyr. This both has to be small, and specifically must not have a scheduler in it.

What is being added to MCUboot that requires multi-threading (and how do I stop this?). The bootloader should not have a scheduler in it.

-2

d3zd3z

Running without a scheduler is a requirement for multiple applications, including MCUboot and the secure side of a trusted firmware application. Both of these require advanced scrutiny, and require that there be as little code as possible. This isn't just about the size of the code, but about auditing/validating the code as well, especially from a security perspective.

There are people already uncomfortable that MCUboot uses Zephyr at all, and have only been placated by telling them that most of Zephyr is not being included, and Zephyr is only being used for board support, and minimal drivers (generally flash and UART).

hongshui3000 · 2018-10-11T05:27:40Z

A single-threaded programming model can be defined to standardize, single-threaded applications. If you simply remove it will undoubtedly narrow the scope of use.

sjanc · 2018-10-15T16:36:34Z

Why not just simply require from drivers to depend on multi threading support if that is needed by driver? That would give nice early build error.

andrewboie · 2019-03-15T21:23:17Z

We need to come to some kind of resolution on this at least for the short term 1.14 release. Right now, with multithreading disabled, the application runs with interrupts disabled. See my findings #14454

There are some good points being made and I have no particular stake or preference on which way this goes, but if we keep this feature, I suggest we should make it a no-op/unsupported for 1.14 to address these issues, and then do a proper well-tested implementation for 1.15.

nashif · 2019-05-07T13:02:14Z

outdated, close

dcpleung requested a review from nashif September 27, 2018 23:14

dcpleung requested review from andrewboie, andyross, nniranjhana, pswarnak and spoorthik as code owners September 27, 2018 23:14

dcpleung force-pushed the remove_single_thread_9808 branch from 3298776 to 85eb3bb Compare September 28, 2018 02:52

dcpleung changed the title ~~Make multithreading the only one~~ kernel: remove single thread support Sep 28, 2018

dcpleung added the DNM This PR should not be merged (Do Not Merge) label Sep 28, 2018

carlescufi requested review from d3zd3z and nvlsianpu September 28, 2018 12:37

pfalcon requested a review from mike-scott September 28, 2018 13:23

andyross approved these changes Sep 28, 2018

View reviewed changes

carlescufi requested changes Sep 29, 2018

View reviewed changes

sjanc requested changes Sep 30, 2018

View reviewed changes

nvlsianpu requested changes Oct 1, 2018

View reviewed changes

nashif added this to DNM in PR Queue Oct 5, 2018

d3zd3z requested changes Oct 10, 2018

View reviewed changes

mike-scott removed their request for review February 6, 2019 22:24

nashif closed this May 7, 2019

dcpleung deleted the remove_single_thread_9808 branch May 7, 2019 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: remove single thread support #10275

kernel: remove single thread support #10275

dcpleung commented Sep 27, 2018

codecov-io commented Sep 27, 2018 •

edited

Loading

dcpleung commented Sep 28, 2018

carlescufi commented Sep 28, 2018 •

edited

Loading

nvlsianpu commented Sep 28, 2018

andyross left a comment

nashif commented Sep 28, 2018

andyross commented Sep 28, 2018

carlescufi commented Sep 28, 2018

dcpleung commented Sep 28, 2018

carlescufi commented Sep 28, 2018

dcpleung commented Sep 28, 2018 •

edited

Loading

carlescufi left a comment

sjanc commented Sep 29, 2018

sjanc left a comment

nvlsianpu left a comment

nashif commented Oct 1, 2018

sjanc commented Oct 1, 2018

nvlsianpu commented Oct 2, 2018

d3zd3z commented Oct 10, 2018

d3zd3z commented Oct 10, 2018

d3zd3z left a comment

hongshui3000 commented Oct 11, 2018

sjanc commented Oct 15, 2018

andrewboie commented Mar 15, 2019

nashif commented May 7, 2019

kernel: remove single thread support #10275

kernel: remove single thread support #10275

Conversation

dcpleung commented Sep 27, 2018

codecov-io commented Sep 27, 2018 • edited Loading

Codecov Report

dcpleung commented Sep 28, 2018

carlescufi commented Sep 28, 2018 • edited Loading

nvlsianpu commented Sep 28, 2018

andyross left a comment

Choose a reason for hiding this comment

nashif commented Sep 28, 2018

andyross commented Sep 28, 2018

carlescufi commented Sep 28, 2018

dcpleung commented Sep 28, 2018

carlescufi commented Sep 28, 2018

dcpleung commented Sep 28, 2018 • edited Loading

carlescufi left a comment

Choose a reason for hiding this comment

sjanc commented Sep 29, 2018

sjanc left a comment

Choose a reason for hiding this comment

nvlsianpu left a comment

Choose a reason for hiding this comment

nashif commented Oct 1, 2018

sjanc commented Oct 1, 2018

Disable console and logs

Disable not needed OS features

nvlsianpu commented Oct 2, 2018

d3zd3z commented Oct 10, 2018

d3zd3z commented Oct 10, 2018

d3zd3z left a comment

Choose a reason for hiding this comment

hongshui3000 commented Oct 11, 2018

sjanc commented Oct 15, 2018

andrewboie commented Mar 15, 2019

nashif commented May 7, 2019

codecov-io commented Sep 27, 2018 •

edited

Loading

carlescufi commented Sep 28, 2018 •

edited

Loading

dcpleung commented Sep 28, 2018 •

edited

Loading