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

Boot: Make ARMC library mutexes dynamic #4731

Merged
merged 1 commit into from Jul 24, 2017

Conversation

Projects
None yet
7 participants
@bulislaw
Member

bulislaw commented Jul 10, 2017

ARMC requires variable number of dynamic mutexes. We use combination of RTX mutex pool and heap allocation to achieve that.

Status

READY

CC: @0x6d61726b @c1728p9

Closes #4688

@bulislaw bulislaw self-assigned this Jul 10, 2017

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 10, 2017

/morph test-nightly

rtos/mbed_boot.c Outdated
attr.name = "ARMC toolchain mutex";
attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
*m = osMutexNew(&attr);

This comment has been minimized.

@avolinski

avolinski Jul 10, 2017

Contributor

if osMutexNew succeeds, then you have 2 calls to osMutexNew(&attr)
one here and another one below.
so you have *m overridden with another allocation and previous one leaks
is that intentional?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jul 10, 2017

Contributor

If it succeeds it returns. See the 2 following lines.

This comment has been minimized.

@0xc0170

0xc0170 Jul 11, 2017

Member

If it succeeds it returns. See the 2 following lines.

@avolinski Does that answer your questions?

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 11, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 763

Example Build failed!

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 11, 2017

/morph test-nightly

@0xc0170

LGTM

rtos/mbed_boot.c Outdated
{
osMutexAttr_t attr;
memset(&attr, 0, sizeof(attr));
attr.name = "ARMC toolchain mutex";

This comment has been minimized.

@0xc0170

0xc0170 Jul 11, 2017

Member

ARMCC should be the name?

This comment has been minimized.

@sg-

sg- Jul 12, 2017

Member

ARM toolchain or ARMCx

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 11, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 781

Example Build failed!

@bulislaw bulislaw force-pushed the bulislaw:fix_armc_mutexes branch Jul 12, 2017

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Jul 12, 2017

It crashed due to some Jenkins job, test needs re-running.

Setting pull request status Cam-CI uvisor Build & Test to FAILURE with message: Failed
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
java.io.IOException: remote file operation failed: c:\jsbld\ws\mbed-os\mbed-os-pr-uvisor-test-pipeline@10 at hudson.remoting.Channel@6eb5794:e104819_builds: java.nio.file.DirectoryNotEmptyException: c:\jsbld\ws\mbed-os\mbed-os-pr-uvisor-test-pipeline@10\autogen\4\mbed-os\targets\TARGET_NXP\TARGET_LPC81X\TARGET_LPC812\device
	at hudson.FilePath.act(FilePath.java:986)
	at hudson.FilePath.act(FilePath.java:968)
	at hudson.FilePath.deleteRecursive(FilePath.java:1170)
	at org.jenkinsci.plugins.workflow.steps.DeleteDirStep$Execution.run(DeleteDirStep.java:68)
	at org.jenkinsci.plugins.workflow.steps.DeleteDirStep$Execution.run(DeleteDirStep.java:61)
	at org.jenkinsci.plugins.workflow.steps.AbstractSynchronousNonBlockingStepExecution$1$1.call(AbstractSynchronousNonBlockingStepExecution.java:52)
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 12, 2017

retest uvisor

@bulislaw bulislaw force-pushed the bulislaw:fix_armc_mutexes branch Jul 12, 2017

@sg-

This comment has been minimized.

Member

sg- commented Jul 16, 2017

Looking at this again it seems that this could be simplified to not use a combination of pool + heap and just used all heap. Why are we not doing that?

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 17, 2017

ARM toolchain requires a mutex to use malloc. To boot platform to main you require 5 mutexes, therefore I decided we may as well have a pool of 5.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 17, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 18, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 832

All builds and test passed!

@sg-

This comment has been minimized.

Member

sg- commented Jul 19, 2017

Something still doesn't sit right here. After reading the manual it seems the static mutexes still doesn't guarantee that malloc will use a statically allocated slot therefore the call to malloc could be unprotected. The safest approach seems to be all dynamic mutexes and adding a call to malloc during startup within a critical section at startup before the kernel is running.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 19, 2017

It all happens before the kernel is running, but call to malloc requires a mutex, so fully dynamic solution would get as to malloc->mutex_initialize()->malloc()->mutex_initialize().

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 20, 2017

@sg- I've got a reply from ARM toolchain team, there's a maximum of 8 mutexes + 1 mutex for each fopen. And it's not guaranteed to stay like that between version, but it didn't change for a long time. They advised that our hybrid solution should do the trick. Do you want me to change the number of static pool to 8?

@sg-

This comment has been minimized.

Member

sg- commented Jul 20, 2017

That would be great - even better if there is a documentation link to go with the commit message but OK if that doesn't exist.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 20, 2017

It's not documented, they had to grep the code and figure out where mutex ops are called.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 21, 2017

@sg- I've got a reply from ARM toolchain team, there's a maximum of 8 mutexes + 1 mutex for each fopen. And it's not guaranteed to stay like that between version, but it didn't change for a long time. They advised that our hybrid solution should do the trick. Do you want me to change the number of static pool to 8?

Thanks for sharing, +1 to have it set to 8 then

Boot: Provide dynamic mutexes for ARM toolchain
ARM toolchain requires variable number of dynamic mutexes. We use combination of
RTX mutex pool and heap allocation to achieve that.

@bulislaw bulislaw force-pushed the bulislaw:fix_armc_mutexes branch to a1736e6 Jul 21, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 21, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 21, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 859

All builds and test passed!

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Jul 24, 2017

Can we now get this ASAP to an mbed OS patch release, please? @adbridge @sg-

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jul 24, 2017

@theotherjimmy theotherjimmy merged commit e7f27aa into ARMmbed:master Jul 24, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment