NVStore tests: Tune memory consumption; stop threads greafully #7127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dannybenor
reviewed
Jun 6, 2018
| @@ -378,6 +377,27 @@ static void nvstore_basic_functionality_test() | |||
| delete[] nvstore_testing_buf_get; | |||
| } | |||
|
|
|||
| static void calc_thread_stack_size(int &num_threads, uint32_t min_size, uint32_t max_size, | |||
There was a problem hiding this comment.
Please add a comment to the function about the parameters. There two in/out parameters and the idea of the function is good but not trivial. Beside that looks OK to me
There was a problem hiding this comment.
+1, I was reading the definition how the test gets num threads and stack size (I knew it would be via &), plus what heap_get returns if heap stats are not enabled (memset to 0, thus the check in reserved_size) - these were not obvious to me , neither then what while does in the below on line 391
0xc0170
requested changes
Jun 6, 2018
davidsaada
force-pushed
the
david_nvstore_fix_test_alloc
branch
from
8a37d8d
to
85f0934
Compare
|
Explanations added. |
|
Ok with me |
davidsaada
force-pushed
the
david_nvstore_fix_test_alloc
branch
from
85f0934
to
f0362c3
Compare
0xc0170
previously approved these changes
Jun 6, 2018
|
@maciejbocianski @mprse @fkjagodzinski Please review changes in features/nvstore/TESTS/nvstore/functionality/main.cpp (this seems like it might be useful for other tests as well) |
|
I noticed that this change cause the test is still failing on NRF52 nad ARCH_PRO when run with CI config (develop + MBED_TRAP_ERRORS_ENABLED, MBED_ALL_STATS_ENABLED, MBED_HEAP_STATS_ENABLED, MBED_STACK_STATS_ENABLED)
|
|
|
||
| // Check if we can allocate enough stack size (per thread) for the current number of threads | ||
| while (num_threads) { | ||
| stack_size = (heap_stats.reserved_size - heap_stats.current_size) / num_threads; |
There was a problem hiding this comment.
It could happen that we utilize whole remaining heap memory for threads, we should leave some margin (e.g. 10% of reserved_size or 1kB)
There was a problem hiding this comment.
Understood. Are you saying this as a general note or specifically for this test? Reason I ask is that both min and max values in this test (768/2048) take these spares into account. However, I may create a more general infrastructure for this, as I get more and more low memory boards that fail on these kind of tests, so may take this into account.
There was a problem hiding this comment.
It's general note. How does it take these spares into account?
e.g. If we have 8kB free heap memory , then whole 8kB will be utilized for threads, am I right?
There was a problem hiding this comment.
Sorry, now I understand you comment. Thought the spares you mentioned were to be used by the threads themselves and not by the main thread. Yes, you have a point here.
|
@davidsaada Would you happen to have test results that show that this PR should pass tests with the CI flags enabled on the NRF52? |
davidsaada
force-pushed
the
david_nvstore_fix_test_alloc
branch
2 times, most recently
from
975bf02
to
6d4d0dd
Compare
davidsaada
changed the title
|
Made a change to this PR (also updated PR description), adding a graceful stop to the multi-thread test instead of thread termination, as killing threads causes a few issues if it happens during low level actions (like flash programming in the NRF52 board). |
It's OK now, following PR changes. Test built with heap & stack stats enabled. Here's the test output: |
|
@davidsaada Fyi, you can use ``` ``` to encapsulate CLI output to make it easier to read :) |
You are right of course, and I usually do that. Maybe all this text made me lose my grip... |
|
Hi |
Exporter Build : SUCCESSBuild number : 2113 |
Test : FAILUREBuild number : 2252 |
- Tune thread stack size in nvstore test using heap stats - Stop threads gracefully instead of killing them (in multi-thread test)
davidsaada
force-pushed
the
david_nvstore_fix_test_alloc
branch
from
6d4d0dd
to
1b5b839
Compare
|
Pushed a few small changes following failed test. Believe it should be OK now (also tested on failed board). |
|
Interesting. Since it looks like there were a good chunk of changes with that last commit, I'm going to ask for a re-review. @dannybenor @maciejbocianski @0xc0170 @marcuschangarm Would y'all mind looking at the commit again? |
|
Folks - any chance to get a review? This PR fixes a few issues. |
0xc0170
approved these changes
Jul 10, 2018
dannybenor
approved these changes
Jul 10, 2018
|
/morph build |
Build : SUCCESSBuild number : 2564 Triggering tests/morph test |
Test : SUCCESSBuild number : 2318 |
Exporter Build : SUCCESSBuild number : 2208 |
This was referenced Jul 17, 2018
pan-
pushed a commit
to pan-/mbed
that referenced
this pull request
Aug 22, 2018
…t_alloc NVStore tests: Tune memory consumption; stop threads greafully
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Revise NVStore tests:
Pull request type