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

RTOS threads test: Handle out of memory cases #7744

Merged
merged 1 commit into from Aug 23, 2018

Conversation

Projects
None yet
7 participants
@davidsaada
Contributor

davidsaada commented Aug 9, 2018

Description

The RTOS threads test crashes if it fails to allocate memory on low memory boards. Fix this problem.
Resolves #7535

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change
@cmonr

cmonr approved these changes Aug 14, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

@c1728p9 Mind taking a look before we start CI?

@c1728p9

It seems a bit concerning to skip tests because the device doesn't have enough memory to run them. If the TMPM3H6 doesn't have enough memory to run tests should it be enabled for mbed 5? What is the impact of leaving this target out?

// Don't fail test due to lack of memory. Call function directly instead
if (!child || !dummy) {
increment(counter);

This comment has been minimized.

@c1728p9

c1728p9 Aug 15, 2018

Contributor

If this isn't actually getting called on a new thread doesn't this defeat the purpose of this test?

This comment has been minimized.

@davidsaada

davidsaada Aug 16, 2018

Contributor

This continues our discussion from below, under the assumption we want to allow low memory boards to still pass the test. If we don't have enough memory to launch a new thread, but still need this test to pass under these conditions (which is what this condition checks), then the only way to make this test pass is to manually call the increment function.

@aashishc1988

@davidsaada Can you please take a look at my comment?

@@ -174,6 +196,10 @@ void test_single_thread()
template <int N, void (*F)(counter_t *)>
void test_parallel_threads()
{
char *dummy = new (std::nothrow) char[PARALLEL_THREAD_STACK_SIZE * N];
delete[] dummy;
TEST_SKIP_UNLESS_MESSAGE(dummy, "Not enough memory to run test");

This comment has been minimized.

@aashishc1988

aashishc1988 Aug 15, 2018

Contributor

@davidsaada This doesnt seem right. We are trying to allocate the space and then deleting it right away, and then comparing it using the macro. It will always skip the test. Unless I am missing something here. Can you please check?

This comment has been minimized.

@davidsaada

davidsaada Aug 16, 2018

Contributor

The delete[] command has no effect on the value of dummy variable. The test will be skipped only if memory allocation fails. I could switch these two lines, but it won't change anything.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Aug 16, 2018

It seems a bit concerning to skip tests because the device doesn't have enough memory to run them. If the TMPM3H6 doesn't have enough memory to run tests should it be enabled for mbed 5? What is the impact of leaving this target out?

I have no problem leaving this (or any other low memory) target out of our CI or from mbed-os. This PR was based on the assumption that these boards were included in our CI. Under this assumption, one would need these tests to pass. Diluting the tests to reduce memory consumption is a lose-lose attitude, as it harms the test quality on the one hand, and may not be sufficient for even lower memory boards when these are added. Now, we could have this board specifically black listed in the test code (e.g. have an #ifdef excluding specific targets from the test). These exclusions are pretty ugly IMO and shouldn't be part of any code. What I tried to accomplish here was a "dynamic black list" - exclude boards that haven't got enough memory by simply testing whether memory allocation succeeds. This prevents the need for explicit black or white lists in the code. But again - if one decides to exclude this board (or other boards) from CI, I'm fine with it, other than the fact that it's out of this PR's scope.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Aug 16, 2018

@OPpuolitaival

This comment has been minimized.

Contributor

OPpuolitaival commented Aug 16, 2018

This looks good for me. It is better to run tests in boards which have enough memory than leave out the test. We will run more boards in continuous integration in future.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 17, 2018

Hi @davidsaada, I agree, something like a black list is preferable to modifying each test case. Even that is not ideal though.

I would like to see some minimum requirements for Mbed 5 devices and then require that tests fit in that. Otherwise there will be pressure to keep shrinking/bypassing tests to fit on even smaller devices. I proposed a minimum requirement of 2K for heap and 2K for data in #5285 but even that caused too many devices to fail.

I'm not sure how usable these smaller devices are with Mbed 5 if they can't pass our already space optimized tests. It seems wrong to allow a device which doesn't have enough space to create a second thread (with an already minimized stack size) to be Mbed 5 enabled.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 18, 2018

@c1728p9 It sounds like you're amicable to this PR for now. Would you mind OKing or dismissing your review?
@aashishc1988 Any thoughts?

@c1728p9 @aashishc1988 @davidsaada @OPpuolitaival Does it make sense to create a new issue/internal ticket to track down and enable minimum testing requirements?

I'm leaving how to proceed with this PR up to @davidsaada

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Aug 19, 2018

Does it make sense to create a new issue/internal ticket to track down and enable minimum testing requirements?

Sure does. We also had some internal discussions about it (following this PR and ones alike). Guess a new issue may be a good place to discuss it.

@cmonr cmonr added needs: CI and removed needs: review labels Aug 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 21, 2018

@davidsaada @c1728p9 Could one of y'all open an issue (if one isn't already), and link it here? In the meanwhile, we'll get this PR on its way.

/morph build

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Aug 21, 2018

Opened #7845 to follow up on our discussion regarding low resource boards.

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 21, 2018

Build : SUCCESS

Build number : 2853
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7744/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 requested a review from bulislaw Aug 22, 2018

@mbed-ci

This comment has been minimized.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Aug 22, 2018

Test needs to be rerun. Failure is on a single board (NRF51_DK) on two other tests.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2018

/morph test

1 similar comment
@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 22, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2018

I would like to see some minimum requirements for Mbed 5 devices and then require that tests fit in that. Otherwise there will be pressure to keep shrinking/bypassing tests to fit on even smaller devices. I proposed a minimum requirement of 2K for heap and 2K for data in #5285 but even that caused too many devices to fail.

I'm not sure how usable these smaller devices are with Mbed 5 if they can't pass our already space optimized tests. It seems wrong to allow a device which doesn't have enough space to create a second thread (with an already minimized stack size) to be Mbed 5 enabled.

👍 Would be useful to have the minimum req.

@c1728p9 You happy with this as it is ?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 23, 2018

Opened #7845 to follow up on our discussion regarding low resource boards.

Going to merge since discussion will be moved to referenced issue.

@cmonr cmonr merged commit e530939 into ARMmbed:master Aug 23, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 568 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9953 cycles (+137 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@davidsaada davidsaada deleted the davidsaada:david_threads_test_fix branch Aug 23, 2018

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 29, 2018

Merge pull request ARMmbed#7744 from davidsaada/david_threads_test_fix
RTOS threads test: Handle out of memory cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment