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

M252KG: Fix kvstore-static_tests failing with OOM #11183

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@ccli8
Copy link
Contributor

commented Aug 8, 2019

Description

This PR is split from #11176 and to fix OOM error in kvstore-static_tests test on NUMAKER_M252KG target. NUMAKER_M252KG just has 32KiB and meets OOM error in this test. This PR fixes it by decreasing forked threads from 3 to 2.

Depends on

#11176

Pull request type

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

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-maintainers Aug 8, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@ccli8, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-storage Aug 8, 2019

static const int num_of_threads = 3;
static const char num_of_keys = 3;
static const char *keys[] = {"key1", "key2", "key3"};
#endif

static const int heap_alloc_threshold_size = 4096;

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Aug 8, 2019

Contributor

Please try if rising this value would make this device to skip the test.

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 15, 2019

Author Contributor

@SeppoTakalo Enlarge heap_alloc_threshold_size to 16384 can skip the test. So I submit another PR to replace this one?

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Aug 15, 2019

Contributor

Modify this PR to use value 4*OS_STACK_SIZE, which is equivalent.
Then remove other modifications.

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 15, 2019

Author Contributor

@SeppoTakalo Do rebase and update as above

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-maintainers Aug 8, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@ccli8, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-storage Aug 8, 2019

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_m252kg_test branch from 631234b to 976c37a Aug 15, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

I can see same value used in following tests as well:

mbed-os/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp
mbed-os/features/storage/TESTS/kvstore/static_tests/main.cpp
mbed-os/features/storage/TESTS/kvstore/filesystemstore_tests/main.cpp
mbed-os/features/storage/TESTS/kvstore/general_tests_phase_2/main.cpp
mbed-os/features/storage/TESTS/kvstore/securestore_whitebox/main.cpp
mbed-os/features/storage/TESTS/kvstore/tdbstore_whitebox/main.cpp

If you could modify those to match as well (if the logic is the same in those).

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Astyle complains, because you need to use spaces around operators:

static const int heap_alloc_threshold_size = 4 * OS_STACK_SIZE;
Fix kvstore-static_tests failing with OOM
Forked 3 threads plus misc, so minimum (4 * OS_STACK_SIZE) heap are required.

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_m252kg_test branch from 976c37a to 500221c Aug 15, 2019

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Astyle complains, because you need to use spaces around operators:

Fixed

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

I can see same value used in following tests as well:
... ...
If you could modify those to match as well (if the logic is the same in those).

Not the same logic. Cannot determine heap_alloc_threshold_size easily.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 19, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 19, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 5904614 into ARMmbed:master Aug 20, 2019

18 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/greentea-test Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8600 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details

@ccli8 ccli8 deleted the OpenNuvoton:nuvoton_m252kg_test branch Aug 20, 2019

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