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

Heap size adjusted to work for both tls-client and mbed-client #3920

Merged
merged 1 commit into from Mar 22, 2017

Conversation

Projects
None yet
7 participants
@mazimkhan
Contributor

mazimkhan commented Mar 10, 2017

Description

Targets NUCLEO_F429ZI and UBLOX_EVK_ODIN_W2 have 192K RAM.
Heap size in PR #3871 was increased from 48K to 96K as tls-client
example failed with 48K heap. But this resulted in compilation failures
in mbed-client that requires 71K for global/static data.
Hence this PR reduces heap to 64K that minimum required by tls-client
to work. This also meets mbed-client data segment requirements.

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Related PRs

#3871

branch PR
other_pr_production link
other_pr_master link

Todos

  • Tests
  • Documentation

Deploy notes

Notes regarding the deployment of this PR. These should note any
required changes in the build environment, tools, compilers, etc.

Steps to test or reproduce

It should fix issue ARMmbed/mbed-os-example-client#194 (comment)
and build failure mentioned here ARMmbed/mbed-os-example-client#194 (comment)

@0xc0170 @SeppoTakalo

targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/device/TOOLCHAIN_IAR/stm32f429xx_flash.icf Outdated
@@ -17,7 +17,7 @@ define symbol __ICFEDIT_region_CCMRAM_end__ = 0x1000FFFF;
/*-Sizes-*/
/*Heap 1/2 of ram and stack 1/8*/
define symbol __ICFEDIT_size_cstack__ = 0x6000;
define symbol __ICFEDIT_size_heap__ = 0x18000;

This comment has been minimized.

@0xc0170

0xc0170 Mar 10, 2017

Member

is it 1/2 of RAM (comment says so) ? Is it 1/4 as thats usual number ?

Let's make sure this new number works for all reported issues we had within last days, and any future failure shall be a known issue?

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Mar 10, 2017

Contributor

Preferably just remove those /*Heap 1/2 of ram and stack 1/8*/ and just tell the numbers, like /* 64kB of HEAP */

This comment has been minimized.

@mazimkhan

mazimkhan Mar 10, 2017

Contributor

Ok

@mazimkhan mazimkhan force-pushed the mazimkhan:master branch Mar 10, 2017

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 13, 2017

Why do we reserve 24kB of stack?
I have only seen 6kB used in mesh or client examples, when they call mbed TLS. Most of the threads we are running use something like 1kB or 2kB.
24kB for main thread, that is mostly doing nothing, sound like a big.

I still feel like we are trying to tune this just for running the TLS example app.
For the heap, if IAR does not allow it to grow, 64kB sound OK.

@yogpan01

This comment has been minimized.

Contributor

yogpan01 commented Mar 13, 2017

The heap size requirement depends on the application usage of mbed-client, if application decides to create huge number of resources it will increase the heap usage. So, this number is just to fit the current example application, but if someone wants to extend this application , they will run into problem again.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 14, 2017

Why do we reserve 24kB of stack?

mbed 2 that does not use RTOS, and this files come from cmsis and vendors. Thus I assume this is a default value they set it to.

I still feel like we are trying to tune this just for running the TLS example app.
For the heap, if IAR does not allow it to grow, 64kB sound OK.

The heap size requirement depends on the application usage of mbed-client, if application decides to create huge number of resources it will increase the heap usage. So, this number is just to fit the current example application, but if someone wants to extend this application , they will run into problem again.

Correct. This is a fix for the last commit that increased static heap size. It broke some apps as @mazimkhan described above. Thus this should be set for very last time to some sensible default, and apps needs to tune it to their needs (workaround until IAR gets dynamic sized heap). Does that sound fine?

@mazimkhan Travis CI does not report anything below. please have a look

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Mar 14, 2017

@0xc0170 I do not have access to travis. @bridadan can you please help?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 14, 2017

@0xc0170 I do not have access to travis. @bridadan can you please help?

I have just restarted it (it was green!)

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 14, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 15, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1684

Test failed!

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Mar 15, 2017

Seems like a CI error in ci/morph-test-nightly

19:37:42 Process Process-1:
19:37:42 Traceback (most recent call last):
19:37:42   File "c:\python27\Lib\multiprocessing\process.py", line 258, in _bootstrap
19:37:42     self.run()
19:37:42   File "c:\python27\Lib\multiprocessing\process.py", line 114, in run
19:37:42     self._target(*self._args, **self._kwargs)
19:37:42   File "c:\mbed_jenkins\test_node_5\workspace\tm_wrap\1332\.env\lib\site-packages\mbed_host_tests\host_tests_conn_proxy\conn_proxy.py", line 209, in conn_process
19:37:42     data = connector.read(2304)
19:37:42   File "c:\mbed_jenkins\test_node_5\workspace\tm_wrap\1332\.env\lib\site-packages\mbed_host_tests\host_tests_conn_proxy\conn_primitive_serial.py", line 99, in read
19:37:42     time.sleep(self.timeout)
19:37:42 IOError: [Errno 4] Interrupted function call
19:54:30 Agent went offline during the build
19:54:30 ERROR: Connection was broken: java.io.IOException: Connection aborted: org.jenkinsci.remoting.nio.NioChannelHub$MonoNioTransport@2cffc47e[name=test_node_5]
19:54:30 	at org.jenkinsci.remoting.nio.NioChannelHub$NioTransport.abort(NioChannelHub.java:208)
19:54:30 	at org.jenkinsci.remoting.nio.NioChannelHub.run(NioChannelHub.java:629)
19:54:30 	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
19:54:30 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
19:54:30 	at java.util.concurrent.FutureTask.run(FutureTask.java:262)
19:54:30 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
19:54:30 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
19:54:30 	at java.lang.Thread.run(Thread.java:745)
19:54:30 Caused by: java.io.IOException: Connection timed out
19:54:30 	at sun.nio.ch.FileDispatcherImpl.read0(Native Method)
19:54:30 	at sun.nio.ch.SocketDispatcher.read(SocketDispatcher.java:39)
19:54:30 	at sun.nio.ch.IOUtil.readIntoNativeBuffer(IOUtil.java:223)
19:54:30 	at sun.nio.ch.IOUtil.read(IOUtil.java:197)
19:54:30 	at sun.nio.ch.SocketChannelImpl.read(SocketChannelImpl.java:384)
19:54:30 	at org.jenkinsci.remoting.nio.FifoBuffer$Pointer.receive(FifoBuffer.java:137)
19:54:30 	at org.jenkinsci.remoting.nio.FifoBuffer.receive(FifoBuffer.java:310)
19:54:30 	at org.jenkinsci.remoting.nio.NioChannelHub.run(NioChannelHub.java:561)
19:54:30 	... 6 more
19:54:30 
19:54:30 Build step 'Execute shell' marked build as failure
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 15, 2017

/morph test-nightly

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 15, 2017

@mazimkhan Can you rebase this or just do a small change to restart travis? I manually restarted it yesterday, it's green for this PR but does not report back to github, see the results https://travis-ci.org/ARMmbed/mbed-os/jobs/209713138/config

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 15, 2017

This commit reverts back only the F429. Not the Ublox, should it be added here also?

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 15, 2017

Also, I'm really skeptical that we would ever need 24kB of stack on RTOS environment.

This "stack" is not all stacks combined, it is the main stack. All other thread have their own stacks, reserved at compile time (or dynamically).
Most applications probably do while(1) {...} on their main thread, so most of the work is already done in other threads.

Why did you come up with the number 24kB? How did you measure it?
What I'm saying is that maybe we can save over 10kB of RAM by lowering the stack value also. Biggest usage I have seen so far is 6kB on thread using mbed TLS.

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Mar 15, 2017

@SeppoTakalo you are right. But to be compatible with mbed 2.0 we need to leave this. If it was GCC or ARM where stack is located on top and can grow it was ok to reduce it. But in IAR the stack is in the middle and need to be fixed. Hence 24kB is only left for mbed 2.0

Mohammad Azim Khan
Heap size adjusted to work for both tls-client and mbed-client
Targets NUCLEO_F429ZI and UBLOX_EVK_ODIN_W2 have 192K RAM.
Heap size in PR #3871 was increased from 48K to 96K as tls-client
example failed with 48K heap. But this resulted in compilation failures
in mbed-client that requires 71K for global/static data.
Hence this PR reduces heap to 64K that minimum required by tls-client
to work. This also meets mbed-client data segment requirements.

@mazimkhan mazimkhan force-pushed the mazimkhan:master branch to 5a9ea2c Mar 15, 2017

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 15, 2017

Practically stack always grows down, no matter where it is located. (True on ARM, Thumb/Thumb2, X86. Originally ARM supported growing up)
Mostly it is therefore located in top of the RAM. But it always need statically allocated free space under it, where to grow. Its not compiler dependant thing. However, heap also grows, at least in GCC and ARM.
memory usage

But.. This only one of the stacks. That only stands for mbed OS 2, when there are no threads.
With RTOS, we have one stack per thread, and only one of them is located in the top. All others are located, as you said for IAR, in the middle. Somewhere in the .data or .bss sections.
stacks

So far I have spotted following threads inside mbed OS:

As you can see, all these statically allocate their own space for the stack. And are not part of the 24kB you are allocating. And all of the are "in the middle". And all of them are able to run on smaller than 24kB. The 6kB one is the one that calls mbed TLS. Thats why I want to know what requires 24kB on this exact platform. For me, it looks like we are just wasting memory.

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Mar 15, 2017

The linker files in this PR are for IAR. Hence we only need to discuss IAR. Please see IAR stack location explained here https://www.iar.com/kr/support/resources/articles/mastering-stack-and-heap-for-system-reliability/

You are right it is complete waste of memory with RTX in mbed-os 5. 24kB is only specific to IAR on mbed-os 2. We don't know how much stack is used by mbed-os 2.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 15, 2017

Your document does not directly apply.
RTX takes over after the bootstrap, it defines the threads and stacks.

RTX threads:

All those allocate their own stack, somewhere from .data or .bss. So in the end, I don't know if anything other than interrupts end up using the stack you are defining.

Have I misunderstood now something, you say that 24kB is only specific to IAR on mbed-os 2 but the linker files are the same. So the complete 24kB goes into waste in mbed OS 5.

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Mar 15, 2017

@SeppoTakalo I understand what you are saying about the RTX and mbed-os 5. But I don't understand what you are proposing for mbed-os 2 to work.
We have common linker files for mbed os 2 and 5. In this particular context it make this problem of wastage worse.

If I ask you a hypothetically question: Assume mbed-os 2 needs 24kB of stack. What should we put in the IAR liker file?

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 15, 2017

If mbed OS 2 required 24kB, then we put 24kB.
The discussion started when I tried to ask a question that where did you come up with this number? For me, it sounds a quite big for system without threads or events.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 15, 2017

For optimal case, this should be configurable by application, because it is application depend. Not even platform depend.
So the whole stack size should be set by application, or OS with sensible default.
Now we are fine tuning just one particular platform.

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Mar 15, 2017

This is exactly what I have said here ARMmbed/mbed-os-example-client#194 (comment)

We didn't come up with this number. It was there by default.
I just checked K64F https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/device/TOOLCHAIN_IAR/MK64FN1M0xxx12.icf
That is even bigger 32kB.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 15, 2017

The discussion started when I tried to ask a question that where did you come up with this number? For me, it sounds a quite big for system without threads or events.

It's part of the linker file that comes from vendors. They define these values and use those in their apps or drivers. In the most cases, these numbers are not changed for mbed in most cases (my assumption as I even did not change those when I ported a target to mbed).

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 15, 2017

OK.
Actually, the whole discussion is already irrelevant to this PR.
I searched and looks like we have very wide variety of stack sizes starting from 512kB (on NXP KL05Z) to 32kB. So, if this will be changed, it is not in this PR.

This PR fixes the build issue we have in mesh-minimal application when using IAR on UBLOX_EVK_ODIN_W2

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 15, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1686

Test failed!

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 15, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 15, 2017

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1689

All builds and test passed!

@sg- sg- merged commit 3d50554 into ARMmbed:master Mar 22, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test-nightly Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment