Boot: Make ARMC library mutexes dynamic #4731

bulislaw · 2017-07-10T15:35:21Z

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 · 2017-07-10T15:35:35Z

/morph test-nightly

alekshex · 2017-07-10T16:53:56Z

rtos/mbed_boot.c

+    attr.name = "ARMC toolchain mutex";
+    attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
+
+    *m = osMutexNew(&attr);


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?

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

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

@avolinski Does that answer your questions?

mbed-bot · 2017-07-11T04:16:09Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 763

Example Build failed!

bulislaw · 2017-07-11T09:45:08Z

/morph test-nightly

0xc0170

LGTM

0xc0170 · 2017-07-11T12:30:32Z

rtos/mbed_boot.c

+{
+    osMutexAttr_t attr;
+    memset(&attr, 0, sizeof(attr));
+    attr.name = "ARMC toolchain mutex";


ARMCC should be the name?

ARM toolchain or ARMCx

mbed-bot · 2017-07-11T19:58:04Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 781

Example Build failed!

JanneKiiskila · 2017-07-12T13:52:56Z

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 · 2017-07-12T14:15:27Z

retest uvisor

sg- · 2017-07-16T19:49:27Z

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 · 2017-07-17T08:23:30Z

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 · 2017-07-17T15:59:52Z

/morph test

mbed-bot · 2017-07-18T09:45:55Z

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- · 2017-07-19T07:56:28Z

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 · 2017-07-19T08:19:15Z

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 · 2017-07-20T16:09:23Z

@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- · 2017-07-20T16:33:02Z

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 · 2017-07-20T16:34:08Z

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

0xc0170 · 2017-07-21T09:08:06Z

@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

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

0xc0170 · 2017-07-21T09:32:35Z

/morph test

mbed-bot · 2017-07-21T14:20:34Z

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 · 2017-07-24T07:08:16Z

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

bulislaw self-assigned this Jul 10, 2017

bulislaw mentioned this pull request Jul 10, 2017

Number of mbedOS mutexes exceeds maximum allowed when compiling with ARMCC #4688

Closed

alekshex reviewed Jul 10, 2017

View reviewed changes

theotherjimmy added the needs: CI label Jul 10, 2017

0xc0170 approved these changes Jul 11, 2017

View reviewed changes

bulislaw force-pushed the fix_armc_mutexes branch from 533adae to a2af318 Compare July 12, 2017 09:43

bulislaw force-pushed the fix_armc_mutexes branch from a2af318 to 8e8d68e Compare July 12, 2017 14:36

sg- added the release-version: 5.5.4 label Jul 16, 2017

Boot: Provide dynamic mutexes for ARM toolchain

a1736e6

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

bulislaw force-pushed the fix_armc_mutexes branch from 8e8d68e to a1736e6 Compare July 21, 2017 09:10

0xc0170 approved these changes Jul 21, 2017

View reviewed changes

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

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

theotherjimmy removed the ready for merge label Jul 24, 2017

0xc0170 mentioned this pull request Aug 8, 2017

Facing error: KL46 :RTX error code: 0x00000004, task ID: 0x1FFFF160 #4870

Closed

Boot: Make ARMC library mutexes dynamic #4731

Boot: Make ARMC library mutexes dynamic #4731

Conversation

bulislaw commented Jul 10, 2017

Status

Uh oh!

bulislaw commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbed-bot commented Jul 11, 2017

Output

Uh oh!

bulislaw commented Jul 11, 2017

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbed-bot commented Jul 11, 2017

Output

Uh oh!

JanneKiiskila commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jul 12, 2017

Uh oh!

sg- commented Jul 16, 2017

Uh oh!

bulislaw commented Jul 17, 2017

Uh oh!

theotherjimmy commented Jul 17, 2017

Uh oh!

mbed-bot commented Jul 18, 2017

Output

Uh oh!

sg- commented Jul 19, 2017

Uh oh!

bulislaw commented Jul 19, 2017

Uh oh!

bulislaw commented Jul 20, 2017

Uh oh!

sg- commented Jul 20, 2017

Uh oh!

bulislaw commented Jul 20, 2017

Uh oh!

0xc0170 commented Jul 21, 2017

Uh oh!

0xc0170 commented Jul 21, 2017

Uh oh!

mbed-bot commented Jul 21, 2017

Output

Uh oh!

JanneKiiskila commented Jul 24, 2017

Uh oh!

Uh oh!

JanneKiiskila commented Jul 12, 2017 •

edited

Loading