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

Refactor socket stats to reduce boiler plate #9959

Merged

Conversation

Projects
None yet
8 participants
@michalpasztamobica
Copy link
Contributor

commented Mar 6, 2019

Description

Add a proper test case setup and teardown which does the socket stats checks in tcp, udp and tls.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@VeijoPesonen
@KariHaapalehto

@ciarmcom ciarmcom requested review from KariHaapalehto, SeppoTakalo, VeijoPesonen and ARMmbed/mbed-os-maintainers Mar 6, 2019

@ciarmcom

This comment has been minimized.

@cmonr cmonr added needs: work and removed needs: review labels Mar 6, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@michalpasztamobica Please take a look at and address the Travis CI astyle issues.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:refactor_socket_stats_usage branch Mar 7, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Travis issues fixed.

@0xc0170

0xc0170 approved these changes Mar 7, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

“bioler” = “boiler” as in “boiler template”

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Or... boilerplate

Not boiler template..
🙂

@michalpasztamobica michalpasztamobica changed the title Refactor socket stats to reduce bioler plate Refactor socket stats to reduce boiler plate Mar 7, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 13, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR8
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Please take a look at the failued build

DISCO_L475VG_IOT01A::IAR::TESTS-NETSOCKET-TLS

        [DEBUG] Errors: Error[Lp011]: section placement failed
        [DEBUG] Errors:             unable to allocate space for sections/blocks with a total estimated
        [DEBUG] Errors:                       minimum size of 0x1'9660 bytes (max align 0x8) in
        [DEBUG] Errors:                       <[0x2000'0100-0x2001'7fff]> (total uncommitted space
        [DEBUG] Errors:                       0x1'7f00).

@cmonr cmonr added needs: work and removed needs: CI labels Mar 13, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

I compared build output with these changes:

[DEBUG] Output:   211 011 bytes of readonly  code memory
[DEBUG] Output:    50 191 bytes of readonly  data memory
[DEBUG] Output:   118 685 bytes of readwrite data memory

And without them:

[DEBUG] Output:   210 933 bytes of readonly  code memory
[DEBUG] Output:    50 121 bytes of readonly  data memory
[DEBUG] Output:   118 661 bytes of readwrite data memory

I guess this is a small change that makes a big difference...

I also checked what GCC returns and it occupies much more memory with no complains:

Total Static RAM memory (data + bss): 27848(+27848) bytes
Total Flash memory (text + data): 303381(+303381) bytes

I also checked that DISCO_L475VG_IOT01A has 1 MB of flash and 128 kB of SRAM, while this code claims to occupy ~262kB of Flash and ~24 kB of SRAM...

Perhaps the issue is not with the code size, but with linker configuration....

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Memory map indicates, that we have exceeded the SRAM1, larger, 96 kB RAM region :

/* [RAM = 96kb + 32kb = 0x20000] */
/* Vector table dynamic copy: Total: 98 vectors * 4 = 392 bytes (0x188) to be reserved in RAM */
define symbol __NVIC_start__          = 0x10000000;
define symbol __NVIC_end__            = 0x10000187;
define symbol __region_SRAM2_start__  = 0x10000188;
define symbol __region_SRAM2_end__    = 0x10007FFF;
define symbol __region_CRASH_DATA_RAM_start__  = 0x20000000;
define symbol __region_CRASH_DATA_RAM_end__  = 0x200000FF;
define symbol __region_SRAM1_start__  = 0x20000100;
define symbol __region_SRAM1_end__    = 0x20017FFF;

SRAM1 holds:

place in SRAM1_region { readwrite, block HEAP };

With this PR the HEAP size has been increased to 0x17000 (94208 kB), which means that SRAM1 has exactly 3839 kB to hold the readwrite.
I managed to compile and link successfully when I did one of these:

  1. decreased heap to 0x15000:
-define symbol __size_heap__   = 0x17000;
+define symbol __size_heap__   = 0x15000;
  1. moved readwrite to SRAM2_region:
-place in SRAM1_region { readwrite, block HEAP };
-place in SRAM2_region { first block CSTACK, zeroinit };
+place in SRAM1_region { block HEAP };
+place in SRAM2_region { first block CSTACK, zeroinit, readwrite };

Considering that hard-coded heap occupies almost the whole SRAM1 region, I think the latter seems particularly reasonable. @SeppoTakalo, do you have any thoughts on this?

Refactor socket stats to reduce bioler plate
Add a proper test case setup and teardown which does the socket stats
checks in tcp, udp and tls.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:refactor_socket_stats_usage branch to b7ed4b5 Mar 14, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Understanding the root cause I applied a workaround in our code base, by making the handlers_t tls_test_case_handlers const, and thus removing it from the readwrite section.
I still think we could consider rearranging the memory allocation here, as it might hit us again sooner or later...
@cmonr , @0xc0170 , would you please restart the CI? DISCO_L475VG_IOT01A compiles fine now.

@cmonr cmonr added needs: review and removed needs: work labels Mar 15, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

@michalpasztamobica Thank you for the analysis of the failure.

I'm a bit concerned that part of the fix involves decreasing the heap, since #9588 seems to be fixing something important:

Fix Pelion client failure on STM32 Discovery IOT board due to heap running out.

Would like to hear from @SeppoTakalo when he has a chance, but also from @naveenkaje/@linlingao about the possible fix.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@cmonr , sorry, perhaps I put too much information into the comments... in the end I used a workaround which just moved one of the structures into readonly memory part by adding a const keyword (this way it did not occupy space in the tiny readwrite section any more).
The heap and icf script, as per this PR is intact :).

I would still welcome a comment from @SeppoTakalo , as we are clearly coming close to the limits of memory on this chip and it is mainly due to the large static heap...

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

The heap and icf script, as per this PR is intact

Whoops! Thanks for clarifying that.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

If noone has anything to add regarding the heap size, then I think it should be fine to merge this PR.
In the end it only contains test updates (no changes in build scripts, etc.). I managed to go around the shrinking memory problem by moving test's internal structures between memory areas.

@cmonr cmonr added needs: CI and removed needs: review labels Mar 19, 2019

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

starting CI

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 20, 2019

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

The failures are:

Test Result (3 failures / +3)
K66F IAR8 / K66F-IAR.components-storage-blockdevice-component_sd-tests-filesystem-seek.Seek tests
K66F IAR8 / K66F-IAR.features-storage-tests-filesystem-general_filesystem.features-storage-tests-filesystem-general_filesystem
K66F IAR8 / K66F-IAR.components-storage-blockdevice-component_sd-tests-filesystem-fopen.components-storage-blockdevice-component_sd-tests-filesystem-fopen

I only refactored socket tests. I think the failure is unrelated.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

I only refactored socket tests. I think the failure is unrelated.

This is both hilarious and worrisome.
Restarting, but also keeping an eye on them for now.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

CI job restarted: jenkins-ci/greentea-test

Failures did not appear related to PR.

@cmonr cmonr added ready for merge and removed needs: CI labels Mar 20, 2019

@0xc0170 0xc0170 merged commit db8a018 into ARMmbed:master Mar 21, 2019

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/greentea-test Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9144 cycles (-1098 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Tests are green now (were restarted)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.