kernel: remove CONFIG_APPLICATION_MEMORY in favor of app shmem subsystem #12990

andrewboie · 2019-02-01T21:16:16Z

This large PR completely removes CONFIG_APPLICATION_MEMORY from the kernel. It was implemented as a stopgap when userspace was introduced to get test cases up and running, but was never suitable for real-world use-cases where multiple logical applications need to run in isolation.

All of our test cases now use CONFIG_APP_SHARED_MEM, which is an end-to-end solution that ties together creating memory domain partitions for global data in properly aligned linker sections.

1.14 is the first release where userspace is no longer marked experimental and the APIs related to it are now fixed.

This PR supersedes, and includes some patches from #12608

More detail on the changes:

A problem with automatic cleanup of static kernel objects has been fixed; we now only automatically cleanup dynamic kernel objects.
A bug on x86 where app shared memory was not being copied with CONFIG_XIP has been fixed
App shared memory did not compute the bounds of the partition sections correctly for non power of two architectures; we now unconditionally use the gen_app_partitions.py script for all arches to properly do this.
Some new macros have been introduced so that memory pools can be placed within app shared memory partitions.
Much runtime logic for setting up partitions has been moved to build-time.
Some fixes to tests which were found when CONFIG_APPLICATION_MEMORY is turned off, but userspace still enabled.
gen_app_partitions.py can now be configured to include globals any number from third-party libraries
C library globals and malloc arenas are now exposed as k_mem_partitions
ztest now automatically instantiates and adds the main thread to a memory domain, containing ztest and C library globals.
all test cases that depended on CONFIG_APPLICATION_MEMORY to work now use app shared memory

codecov-io · 2019-02-01T22:51:52Z

Codecov Report

Merging #12990 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12990      +/-   ##
==========================================
+ Coverage   48.73%   48.78%   +0.05%     
==========================================
  Files         315      315              
  Lines       46540    46546       +6     
  Branches    10743    10748       +5     
==========================================
+ Hits        22679    22708      +29     
+ Misses      19349    19325      -24     
- Partials     4512     4513       +1

Impacted Files	Coverage Δ
lib/os/fdtable.c	`42.22% <ø> (-0.48%)`	⬇️
kernel/init.c	`73.43% <ø> (ø)`	⬆️
include/kernel.h	`94.44% <ø> (ø)`	⬆️
drivers/clock_control/nrf_power_clock.c	`50% <0%> (-2.11%)`	⬇️
kernel/sched.c	`90.12% <0%> (+0.31%)`	⬆️
boards/posix/nrf52_bsim/time_machine.c	`52.45% <0%> (+4.91%)`	⬆️
boards/posix/nrf52_bsim/irq_handler.c	`78.3% <0%> (+13.2%)`	⬆️
boards/posix/nrf52_bsim/argparse.c	`46.98% <0%> (+13.25%)`	⬆️

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 d9da0b0...ddd484a. Read the comment docs.

andrewboie · 2019-02-02T11:49:47Z

recheck

dbkinder

one doc typo...

doc/kernel/usermode/kernelobjects.rst

We'll fix later, changes not related to this PR.

ruuddw

This introduces some new 'overlapping MPU region' exceptions on ARC, but these can be handled separately. Overall PR is sound.

ioannisg

I am going to look at the patch ASAP. For now I can confirm that the memory protection tests suite (tests/kernel/mem_protect) executes successfully on ARMv8-M architecture (nrf9160_pca10090 board).

ioannisg

Overall, looking good.
The arch/arm changes were minimal - just removing the CONFIG_APPLicATION_MEMORY blocks

Left some comments and questions, pls, address :)

I am going to run the remaining tests that have been changed in this PR on ARMv8-M and report back.

doc/reference/kernel/usermode/kernelobjects.rst

arch/arm/core/cortex_m/mpu/arm_core_mpu.c

tests/kernel/mem_protect/mem_protect/src/inherit.c

include/misc/libc-hooks.h

ioannisg · 2019-02-07T22:33:59Z

CMakeLists.txt

-    set(ALIGN_SIZING_DEP app_sizing_prebuilt linker_app_sizing_script)
-  endif()
+endif()
+if(CONFIG_CPU_HAS_MPU AND CONFIG_USERSPACE)


Here it should be CONFIG_MEMORY_PROTECTION instead of CONFIG_CPU_HAS_MPU

We need the configuration option, not the hw capabilities

Or, could you simply rewrite this as it looks it is only for ARM? In arm USERSPACE implies CPU_HAS_MPU...

ioannisg · 2019-02-07T22:40:49Z

CMakeLists.txt


-if(CONFIG_ARM)
+if(CONFIG_CPU_HAS_MPU AND CONFIG_USERSPACE AND CONFIG_ARM)


Same here, USERSPACE implies CPU_HAS_MPU in arm...

ioannisg

Just a simple cleanup left to do in cmakelists, from my side.
Pls address and we are good to go.

Dynamic kernel objects enforce that the permission state of an object is also a reference count; using a kernel object without permission regardless of caller privilege level is a programming bug. However, this is not the case for static objects. In particular, supervisor threads are allowed to use any object they like without worrying about permissions, and the logic here was causing cleanup functions to be called over and over again on kernel objects that were actually in use. The automatic cleanup mechanism was intended for dynamic objects anyway, so just skip it entirely for static objects. Signed-off-by: Andrew Boie <[email protected]>

We want CONFIG_APPLICATION_MEMORY specifically disabled for this test, but it was being transitively selected by CONFIG_TEST_USERSPACE which defaults to on for CONFIG_TEST. Turn it off so that disabling application memory in the config actually has an effect. Signed-off-by: Andrew Boie <[email protected]>

This is a separate data section which needs to be copied into RAM. Most arches just use the kernel's _data_copy(), but x86 has its own optimized copying code. Signed-off-by: Adithya Baglody <[email protected]> Signed-off-by: Andrew Boie <[email protected]>

This is unnecessary. Signed-off-by: Andrew Boie <[email protected]>

CONFIG_APPLICATION_MEMORY is being removed from Zephyr. Signed-off-by: Andrew Boie <[email protected]>

This patch will run the python scripts for all architectures to determine the partitions available. This is needed for correct operation of the app_shared_mem feature. Signed-off-by: Adithya Baglody <[email protected]> Signed-off-by: Andrew Boie <[email protected]>

This particular rule of cmake was causing problems when run in multi-threaded environment. This change in the dependency will resolve all such issues and ensures that this script is executed correctly. Signed-off-by: Adithya Baglody <[email protected]>

* K_APP_DMEM_SECTION/K_MEM_BMEM_SECTION macros now exist to specifically define the name of the sections for data and bss respectively. * All boards now use the gen_app_partitions.py script, the padding hacks for non-power-of-two arches didn't work right in all cases. Linker scripts have been updated. * The defined k_mem_partition is now completely initialized at build time. The region data structures now only exist to zero BSS. Based on some work submitted by Adithya Baglody <[email protected]> Signed-off-by: Andrew Boie <[email protected]>

Some tests instantiate a lot of thread objects. These were not tagged with __kernel, and CONFIG_APPLICATION_MEMORY was enabled, so the kernel was not adding them to the kernel object database. However, with CONFIG_APPLICATION_MEMORY disabled, this overflowed the default max number of thread objects (16). Increase the max to 32 for these particular tests. Signed-off-by: Andrew Boie <[email protected]>

If CONFIG_APP_SHARED_MEM is enabled, ztest will set up a default memory domain with a single partition on startup. Test case globals may be added to the partition via ZTEST_BMEM/ZTEST_DMEM macros, add their own partitions, or leave the domain and join another one. Signed-off-by: Andrew Boie <[email protected]>

Zephyr may be linked against third-party libraries which were not part of the build. These may contain globals which need to end up in a memory partition. We can now specify the names of these libraries, as well as a destination partition for their globals. Some excessively long variables were renamed. Signed-off-by: Andrew Boie <[email protected]>

This diverges from policy for all of our other arches and C libraries. Global access to the malloc arena may not be desirable. Forthcoming patch will expose, for all C libraries, a k_mem_partition with the malloc arena which can be added to domains as desired. Signed-off-by: Andrew Boie <[email protected]>

* Newlib now defines a special z_newlib_partition containing all globals relevant to newlib. Most of these are in libc.a with a heap tracking variable in newlib's hooks. * Both C libraries now expose a k_mem_partition containing the bounds of the malloc heap arena. Threads that want to use libc malloc() will need to add this to their memory domain. * z_newlib_get_heap_bounds has been removed, in favor of the memory partition for the heap arena * ztest now includes the C library partitions in its memory domain. * The mem_alloc test now runs in user mode to prove that this all works for both C libraries. Signed-off-by: Andrew Boie <[email protected]>

CONFIG_APPLICATION_MEMORY was a stopgap feature that is being removed from the kernel. Convert tests and samples to use the application shared memory feature instead, in most cases using the domain set up by ztest. Signed-off-by: Andrew Boie <[email protected]>

This was never a long-term solution, more of a gross hack to get test cases working until we could figure out a good end-to-end solution for memory domains that generated appropriate linker sections. Now that we have this with the app shared memory feature, and have converted all tests to remove it, delete this feature. To date all userspace APIs have been tagged as 'experimental' which sidesteps deprecation policies. Signed-off-by: Andrew Boie <[email protected]>

SebastianBoe · 2019-02-08T17:15:19Z

Wow, it is great to see deletion of so much code that has caused headaches and blocked progress the last year.

Fantastic.

andrewboie requested review from andyross, dbkinder, galak, MaureenHelm, nashif, nniranjhana, pfalcon, ramakrishnapallala, ruuddw, SebastianBoe and spoorthik as code owners February 1, 2019 21:16

andrewboie requested review from ioannisg and agross-oss February 1, 2019 21:17

andrewboie force-pushed the app-shmem-refactoring branch from e877dd2 to ae82ce8 Compare February 1, 2019 21:25

This was referenced Feb 1, 2019

app_memory: Cleanup of the interface. #11896

Closed

app_memory: cleanup and fixed issues #12608

Closed

andrewboie requested a review from stephensmalley February 1, 2019 21:29

zephyrproject-rtos deleted a comment from zephyrbot Feb 1, 2019

andrewboie force-pushed the app-shmem-refactoring branch from ae82ce8 to 7307573 Compare February 1, 2019 23:22

andrewboie added the area: Memory Protection label Feb 2, 2019

andrewboie modified the milestones: v1.12.1, v1.14.0 Feb 2, 2019

andrewboie mentioned this pull request Feb 2, 2019

app shared memory rules in CMakeLists.txt breaks incremental builds #13001

Closed

ruuddw mentioned this pull request Feb 4, 2019

add the handling of APP_SHARED_MEM in arc mpu driver #12906

Merged

andrewboie force-pushed the app-shmem-refactoring branch from 7307573 to 44906b1 Compare February 4, 2019 20:35

dbkinder suggested changes Feb 5, 2019

View reviewed changes

doc/kernel/usermode/kernelobjects.rst Outdated Show resolved Hide resolved

andrewboie force-pushed the app-shmem-refactoring branch from 44906b1 to 6bc43cc Compare February 5, 2019 05:46

ruuddw approved these changes Feb 7, 2019

View reviewed changes

ioannisg reviewed Feb 7, 2019

View reviewed changes

nashif approved these changes Feb 7, 2019

View reviewed changes

ioannisg requested changes Feb 7, 2019

View reviewed changes

andrewboie force-pushed the app-shmem-refactoring branch from 8da7a8e to 62169f6 Compare February 7, 2019 20:10

agross-oss approved these changes Feb 7, 2019

View reviewed changes

ioannisg reviewed Feb 7, 2019

View reviewed changes

ioannisg requested changes Feb 7, 2019

View reviewed changes

Andrew Boie and others added 15 commits February 7, 2019 21:21

tests: userspace: remove extra_sections

988c563

This is unnecessary. Signed-off-by: Andrew Boie <[email protected]>

tests: remove app_memory test

6dae68d

CONFIG_APPLICATION_MEMORY is being removed from Zephyr. Signed-off-by: Andrew Boie <[email protected]>

andrewboie force-pushed the app-shmem-refactoring branch from 62169f6 to ddd484a Compare February 8, 2019 05:22

ioannisg approved these changes Feb 8, 2019

View reviewed changes

nashif merged commit 41f6011 into zephyrproject-rtos:master Feb 8, 2019

andrewboie deleted the app-shmem-refactoring branch April 10, 2019 14:55


		if(CONFIG_ARM)
		if(CONFIG_CPU_HAS_MPU AND CONFIG_USERSPACE AND CONFIG_ARM)

kernel: remove CONFIG_APPLICATION_MEMORY in favor of app shmem subsystem #12990

kernel: remove CONFIG_APPLICATION_MEMORY in favor of app shmem subsystem #12990

Uh oh!

Conversation

andrewboie commented Feb 1, 2019

Uh oh!

codecov-io commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andrewboie commented Feb 2, 2019

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ruuddw left a comment

Choose a reason for hiding this comment

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ioannisg Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

ioannisg Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

ioannisg Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

andrewboie Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

ioannisg Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewboie Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

SebastianBoe commented Feb 8, 2019

Uh oh!

Uh oh!

codecov-io commented Feb 1, 2019 •

edited

Loading

ioannisg Feb 7, 2019 •

edited

Loading