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

Allow OS_THREAD_LIBSPACE_NUM as a macro #11571

Merged
merged 1 commit into from Oct 28, 2019

Conversation

@AriParkkila
Copy link
Contributor

AriParkkila commented Sep 26, 2019

Description

OS_THREAD_LIBSPACE_NUM is defined to 4, which is not a good default in all cases. This change makes it possible to give OS_THREAD_LIBSPACE_NUM also as a macro.

Mbed netsocket tests over cellular require OS_THREAD_LIBSPACE_NUM to be 5.

Pull request type

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

Reviewers

@kivaisan

Release Notes

Copy link
Member

0xc0170 left a comment

Changes to RTX should be also sent upstream, see https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/RTOS2/RTX/Config/RTX_Config.h - this file should get the update there. Please send a patch for review there.

All the rest of config there contain a check if not already defined but not this one, so should be accepted as fix.

@kivaisan

This comment has been minimized.

Copy link
Contributor

kivaisan commented Sep 26, 2019

  • I think @kjbracey-arm should review this from connectivity/core point-of-view.
@0xc0170 0xc0170 requested a review from kjbracey-arm Sep 26, 2019
@0xc0170 0xc0170 added the needs: work label Sep 26, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Sep 26, 2019

Hmm, why isn't this configurable in RTX? Seems like it should be a setting that can be set, whether or not OS_THREAD_OBJ_MEM is set. Even if it is set, OS_THREAD_MEM only limits system-allocated threads, not total, so you might need more.

What actually triggers the failure? Presumably it's not just number of threads - is it number of threads that access errno?

I'm actually kind of happy to know that errno is actually thread-local on at least 1 toolchain - I thought it never was...

Agree that it should be submitted for upstreaming.

Although having to configure it at all is a bit poor - personally rather than having a separate pool of these things, and needing to search that pool, I'd put them in with the other thread info. They could live inside the os_thread_t itself, or be placed in the stack block. I guess that does mean more RAM though - guaranteeing it on every thread, rather than just threads that need it.

In the Mbed OS part, I'd make it a proper config option in the RTOS mbed_lib.json - some people might want even more. (Or less).

@AriParkkila

This comment has been minimized.

Copy link
Contributor Author

AriParkkila commented Sep 27, 2019

The root cause might be that __user_perthread_libspace reserves TLS, which is never released (not even when a thread is terminated). As @kjbracey-arm suggested TLS should probably be part of thread's structs to follow its lifecycle.

@0xc0170 Thanks for pointing out that upstream commit, I'll do it.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Sep 27, 2019

The root cause might be that __user_perthread_libspace reserves TLS, which is never released (not even when a thread is terminated).

Ah, that's significant. I'd definitely suggest trying an RTX patch to add a free there too.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 30, 2019

Once done upstream, let us know , should be aligned with changes here

@AriParkkila

This comment has been minimized.

Copy link
Contributor Author

AriParkkila commented Oct 1, 2019

@0xc0170 Please find upstream PR at ARM-software/CMSIS_5#691

@AriParkkila

This comment has been minimized.

Copy link
Contributor Author

AriParkkila commented Oct 18, 2019

@0xc0170 Upstream PR ARM-software/CMSIS_5#691 was merged. This should be ready for merge.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Oct 18, 2019
@0xc0170 0xc0170 self-requested a review Oct 18, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 18, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 18, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 18, 2019

Unittests failed [2019-10-18T11:28:44.880Z] 31: mbed assertation failed: 0, file: /builds/ws/mbed-os-ci_unittests@8/unitTest-4201/mbed-os/features/lorawan/LoRaWANStack.cpp, line 884 , please review

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Oct 18, 2019
@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 28, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 added needs: review and removed needs: work labels Oct 28, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 28, 2019

cc @ARMmbed/mbed-os-test This might need CI update?

@0xc0170 0xc0170 merged commit 5a5b0f6 into ARMmbed:master Oct 28, 2019
25 checks passed
25 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/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8634 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
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.