Skip to content

M252KG: Fix kvstore-static_tests failing with OOM #11183

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 20, 2019
Merged

M252KG: Fix kvstore-static_tests failing with OOM #11183

merged 1 commit into from
Aug 20, 2019

Conversation

ccli8
Copy link
Contributor

@ccli8 ccli8 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

@ccli8 ccli8 mentioned this pull request Aug 8, 2019
Merged
@ciarmcom ciarmcom requested a review from a team August 8, 2019 03:00
@ciarmcom
Copy link
Member

ciarmcom 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 a team August 8, 2019 03:00
SeppoTakalo
SeppoTakalo reviewed Aug 8, 2019
View reviewed changes
features/storage/TESTS/kvstore/static_tests/main.cpp Outdated
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SeppoTakalo Do rebase and update as above

@ciarmcom ciarmcom requested a review from a team August 8, 2019 09:00
@ciarmcom
Copy link
Member

ciarmcom 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 a team August 8, 2019 09:00
@ccli8 ccli8 force-pushed the nuvoton_m252kg_test branch from 631234b to 976c37a Compare August 15, 2019 09:36
@SeppoTakalo
Copy link
Contributor

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
Copy link
Contributor

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

static const int heap_alloc_threshold_size = 4 * OS_STACK_SIZE;

@ccli8
Fix kvstore-static_tests failing with OOM
500221c 
Forked 3 threads plus misc, so minimum (4 * OS_STACK_SIZE) heap are required.
@ccli8 ccli8 force-pushed the nuvoton_m252kg_test branch from 976c37a to 500221c Compare August 15, 2019 09:54
@ccli8
Copy link
Contributor Author

ccli8 commented Aug 15, 2019

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

Fixed

@ccli8
Copy link
Contributor Author

ccli8 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.

@mbed-ci
Copy link

mbed-ci 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
@ccli8 ccli8 deleted the nuvoton_m252kg_test branch August 20, 2019 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

Assignees
No one assigned
Labels
release-version: 5.13.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ccli8 @ciarmcom @SeppoTakalo @mbed-ci @0xc0170