uVisor: Standardize available legacy heap and stack #3692

Patater · 2017-02-03T16:01:39Z

With the RTOS, the STACK_SIZE specified here is unrelated to the stack
size available for the main thread (that runs pre_main). Save memory by
reducing the stack size to a more reasonable amount.

On uVisor, HEAP_SIZE is both a minimum available and maximum available
heap size. The heap can't grow beyond the end of the heap into the
neighboring stack. On all uVisor-supported platforms, guarantee at least
0x6000 bytes of heap space. This increases the portability of uVisor
applications as the memory available for legacy heap allocations is
guaranteed. This helps to avoid out of memory errors on platforms that
were previously guaranteeing less memory.

Status

READY

@0xc0170 @AlessandroA

AlessandroA · 2017-02-03T16:06:02Z

..._Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/device/TOOLCHAIN_GCC_ARM/MK64FN1M0xxx12.ld

-/* Heap 1/4 of ram and stack 1/8 */
-__stack_size__ = 0x8000;
-__heap_size__ = 0x10000;
+__stack_size__ = 0x400;


@Patater In both 3 files, it's probably a good idea to add a short comment stating that:

These values do not affect the actually available heap for an app (they only guarantee a min).

They do not affect the main stack size.

They are set that way because of uVisor.

(or the changes might be lost in future updates).

AlessandroA

👍 LGTM

0xc0170 · 2017-02-06T09:11:40Z

/morph test

0xc0170 · 2017-02-06T09:12:09Z

I had to restart jenkins CI, as it was green but not reported here.

0xc0170 · 2017-02-06T10:50:45Z

..._Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/device/TOOLCHAIN_GCC_ARM/MK64FN1M0xxx12.ld

-__heap_size__ = 0x10000;
+/* With the RTOS in use, this does not affect the main stack size. The size of
+ * the stack where main runs is determined via the RTOS. */
+__stack_size__ = 0x400;


Looks fine for mbed 5. However, in case RTOS is not used (mbed 2), this radically decreasing stack size for MCU like k64f.

I agree, without RTOS, the stack for the main function is reduced a lot. With RTOS, having this be large is a waste of memory, as the user application can never use this RAM (the RTOS uses it for its stack).

It's almost like we need two different linker scripts, one for with RTOS and one for without, unless we can come up with a good balanced number that isn't too bad of a choice for either platform. Any suggestions?

The linker scripts already contain a conditional assignment like the following:

HEAP_SIZE = DEFINED(__heap_size__) ? __heap_size__ : 0x0400; STACK_SIZE = DEFINED(__stack_size__) ? __stack_size__ : 0x0400;

so we could define those symbols somewhere else. In that way we can also define them depending on RTOS being used or not. @0xc0170 do you agree? What's the best place to place those definitions?

Actually, doesn't the stack just start at the top of SRAM and grow down until it hits the heap? So, this would be a minimum stack size with mbed 2.

True. So this should not be an issue @0xc0170

Yes, I believe so.

LGTM

mbed-bot · 2017-02-06T13:20:57Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1507

Test failed!

bridadan · 2017-02-06T15:23:09Z

The failure was caused by Windows 10 installing an update in the middle of a test. Even though I disabled it Microsoft still pushes them through sometimes it looks like 🙄

/morph test

mbed-bot · 2017-02-06T16:35:03Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1509

All builds and test passed!

sg- · 2017-02-07T16:40:13Z

@Patater Can you resolve the conflict?

With the RTOS, the STACK_SIZE specified here is unrelated to the stack size available for the main thread (that runs pre_main). Save memory by reducing the stack size to a more reasonable amount. On uVisor, HEAP_SIZE is both a minimum available and maximum available heap size. The heap can't grow beyond the end of the heap into the neighboring stack. On all uVisor-supported platforms, guarantee at least 0x6000 bytes of heap space. This increases the portability of uVisor applications as the memory available for legacy heap allocations is guaranteed. This helps to avoid out of memory errors on platforms that were previously guaranteeing less memory.

Patater · 2017-02-07T16:49:13Z

Rebased

bridadan · 2017-02-07T19:14:38Z

Looks like the native equeue test failed in Travis?

5.81s$ make -C events/equeue test clean
make: Entering directory `/home/travis/build/ARMmbed/mbed-os/events/equeue'
gcc -c -MMD -O2 -I. -I.. -std=c99 -Wall -D_XOPEN_SOURCE=600 tests/tests.c -o tests/tests.o
gcc -c -MMD -O2 -I. -I.. -std=c99 -Wall -D_XOPEN_SOURCE=600 equeue.c -o equeue.o
gcc -c -MMD -O2 -I. -I.. -std=c99 -Wall -D_XOPEN_SOURCE=600 equeue_posix.c -o equeue_posix.o
gcc -O2 -I. -I.. -std=c99 -Wall -D_XOPEN_SOURCE=600 tests/tests.o equeue.o equeue_posix.o -pthread -o tests/tests
tests/tests
beginning tests...
simple_call_test: ...simple_call_test: passed
simple_call_in_test: ...simple_call_in_test: passed
simple_call_every_test: ...simple_call_every_test: passed
simple_post_test: ...simple_post_test: passed
destructor_test: ...destructor_test: passed
allocation_failure_test: ...allocation_failure_test: passed
cancel_test: ...cancel_test: passed
cancel_inflight_test: ...cancel_inflight_test: passed
cancel_unnecessarily_test: ...cancel_unnecessarily_test: passed
loop_protect_test: ...loop_protect_test: passed
break_test: ...break_test: passed
period_test: ...period_test: passed
nested_test: ...nested_test: passed
sloth_test: ...sloth_test: passed
background_test: ...background_test: passed
chain_test: ...chain_test: passed
unchain_test: ...unchain_test: passed
multithread_test: ...multithread_test: passed
simple_barrage_test: passed
fragmenting_barrage_test: failed at line 87
multithreaded_barrage_test: passed
done!
make: *** [test] Error 1
make: Leaving directory `/home/travis/build/ARMmbed/mbed-os/events/equeue'

The test of concern being fragmenting_barrage_test: failed at line 87.

Any thoughts @geky @Patater ?

geky · 2017-02-07T19:32:52Z

Huh, never seen that before. The timing of the test uses a delta of 10 milliseconds, so it's possible it may fail if the server is under significant load.

Nothing in this pr is related to those tests at all. Restarted, considering the number of times those tests have been ran, I doubt it will be reproducible.

bridadan · 2017-02-07T20:39:00Z

Test seems to be ok now 👍

mazimkhan · 2017-02-08T14:02:26Z

@bridadan @sg- can this be merged now? It is required for enabling EFM32GG platform for uvisor testing.

sg- · 2017-02-08T14:17:54Z

/morph test-nightly

mbed-bot · 2017-02-08T16:53:13Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1529

All builds and test passed!

mazimkhan · 2017-02-08T18:43:03Z

Ready for merge?

sg- · 2017-02-08T23:35:56Z

We've intentionally moved away from hardcoded heap sizes rather giving the heap remaining RAM. If uVisor enforces this should we use uVisor specific symbols?

0xc0170 · 2017-02-09T10:29:42Z

We've intentionally moved away from hardcoded heap sizes rather giving the heap remaining RAM. If uVisor enforces this should we use uVisor specific symbols?

@Patater @AlessandroA what do you think?

Patater · 2017-02-09T10:34:11Z

@sg-

Yes, HEAP_SIZE is no longer used to hardcode the heap size. However, HEAP_SIZE is currently still used to hardcode a minimum heap size. I don't think it quite makes sense to rename HEAP_SIZE into a uVisor specific symbol, as it is still meaningful without uVisor.

uVisor can't protect a fine-grained, dynamically-sized heap. Additionally, applications are expected to use the uVisor-provided page heap. For these two reasons, the "legacy" heap is capped at the minimum heap size specified by HEAP_SIZE in the linker scripts. This allows the proper protection of the heap by uVisor, as well as frees up space for the uVisor-provided page heap.

Note that when FEATURE_UVISOR is not used, even after this PR is merged, HEAP_SIZE is still just a minimum and the heap is given the remaining RAM.

Ports for Upcoming Targets Fixes and Changes 3432: Target STM USBHOST support ARMmbed/mbed-os#3432 3181: NUCLEO_F207ZG extending PeripheralPins.c: all available alternate functions can be used now ARMmbed/mbed-os#3181 3626: NUCLEO_F412ZG : Add USB Device +Host ARMmbed/mbed-os#3626 3628: Fix warnings ARMmbed/mbed-os#3628 3629: STM32: L0 LL layer ARMmbed/mbed-os#3629 3632: IDE Export support for platform VK_RZ_A1H ARMmbed/mbed-os#3632 3642: Missing IRQ pin fix for platform VK_RZ_A1H ARMmbed/mbed-os#3642 3664: Fix ncs36510 sleep definitions ARMmbed/mbed-os#3664 3655: [STM32F4] Modify folder structure ARMmbed/mbed-os#3655 3657: [STM32L4] Modify folder structure ARMmbed/mbed-os#3657 3658: [STM32F3] Modify folder structure ARMmbed/mbed-os#3658 3685: STM32: I2C: reset state machine ARMmbed/mbed-os#3685 3692: uVisor: Standardize available legacy heap and stack ARMmbed/mbed-os#3692 3621: Fix for #2884, LPC824: export to LPCXpresso, target running with wron ARMmbed/mbed-os#3621 3649: [STM32F7] Modify folder structure ARMmbed/mbed-os#3649 3695: Enforce device_name is valid in targets.json ARMmbed/mbed-os#3695 3723: NCS36510: spi_format function bug fix ARMmbed/mbed-os#3723

AlessandroA suggested changes Feb 3, 2017

View reviewed changes

Patater force-pushed the legacy-heap-guarantee branch from 33775d9 to 139c051 Compare February 3, 2017 16:17

AlessandroA approved these changes Feb 3, 2017

View reviewed changes

Patater mentioned this pull request Feb 3, 2017

storage: Add block device API #3671

Merged

0xc0170 added the needs: CI label Feb 6, 2017

0xc0170 reviewed Feb 6, 2017

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Feb 7, 2017

Patater force-pushed the legacy-heap-guarantee branch from 139c051 to 378655f Compare February 7, 2017 16:48

0xc0170 added needs: CI and removed ready for merge labels Feb 8, 2017

bridadan added ready for merge and removed needs: CI labels Feb 8, 2017

0xc0170 added release-version: 5.3.5 and removed ready for merge labels Feb 9, 2017

0xc0170 merged commit 844b94c into ARMmbed:master Feb 9, 2017

mazimkhan mentioned this pull request Mar 2, 2017

Increase HEAP size for UBLOX_EVK_ODIN_W2 and NUCLEO_F429ZI #3871

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uVisor: Standardize available legacy heap and stack #3692

uVisor: Standardize available legacy heap and stack #3692

Patater commented Feb 3, 2017

AlessandroA Feb 3, 2017

AlessandroA Feb 3, 2017

AlessandroA left a comment

0xc0170 commented Feb 6, 2017

0xc0170 commented Feb 6, 2017

0xc0170 Feb 6, 2017

Patater Feb 6, 2017

AlessandroA Feb 6, 2017

Patater Feb 6, 2017

AlessandroA Feb 6, 2017

0xc0170 Feb 7, 2017

mbed-bot commented Feb 6, 2017

bridadan commented Feb 6, 2017

mbed-bot commented Feb 6, 2017

sg- commented Feb 7, 2017

Patater commented Feb 7, 2017

bridadan commented Feb 7, 2017

geky commented Feb 7, 2017 •

edited

Loading

bridadan commented Feb 7, 2017

mazimkhan commented Feb 8, 2017

sg- commented Feb 8, 2017

mbed-bot commented Feb 8, 2017

mazimkhan commented Feb 8, 2017

sg- commented Feb 8, 2017

0xc0170 commented Feb 9, 2017

Patater commented Feb 9, 2017 •

edited

Loading

uVisor: Standardize available legacy heap and stack #3692

uVisor: Standardize available legacy heap and stack #3692

Conversation

Patater commented Feb 3, 2017

Status

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlessandroA left a comment

Choose a reason for hiding this comment

0xc0170 commented Feb 6, 2017

0xc0170 commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbed-bot commented Feb 6, 2017

Output

bridadan commented Feb 6, 2017

mbed-bot commented Feb 6, 2017

Output

sg- commented Feb 7, 2017

Patater commented Feb 7, 2017

bridadan commented Feb 7, 2017

geky commented Feb 7, 2017 • edited Loading

bridadan commented Feb 7, 2017

mazimkhan commented Feb 8, 2017

sg- commented Feb 8, 2017

mbed-bot commented Feb 8, 2017

Output

mazimkhan commented Feb 8, 2017

sg- commented Feb 8, 2017

0xc0170 commented Feb 9, 2017

Patater commented Feb 9, 2017 • edited Loading

geky commented Feb 7, 2017 •

edited

Loading

Patater commented Feb 9, 2017 •

edited

Loading