RTOS: Fix semaphore #4579

geky · 2017-06-16T21:23:56Z

Before RTX 5, the semaphore count limit was UINT16_MAX as a side effect of using a uint16_t (here). After #4294 was merged, the count limit was dropped to the arbitrary value 1024 (here). Additionally, #4389 added an assert that would halt if the semaphore count exceeded this limit (here and here).

This is especially problematic for applications that are using the semaphore for signaling. In RTX 4, the semaphore was the only library-friendly mechanism available for signaling, and in RTX 5 it is still the only option for the C++ layer.

This has already broken several higher-level apis, and we haven't even made the 5.5 release yet:

Fix semaphore usage on lpc1768 emac #4449 The emac layer for the lpc1768 broke. Looking around it's possible quite a few other emacs are broken as well (k64f, rz_a1h). Workaround seems to be keeping a track of the number of buffers that could possibly be enqueued.
events: Adopt osEventFlags from RTX 5 #4571 The event queue broke, workaround is to adopt the osEventFlags in the C layer.
Semaphore release occurs before acquisition for TCP socket over UART #4575 Sockets broke, no workaround available yet.

If we're already having breakages internally, I'm not really looking forward to next week when the release is out.

On top of the breakage, we don't really have a path for users. In the C++ api, a semaphore is the only library friendly signaling mechanism. RTX has thread signals, but these are limited to a single bit-field that is shared with users. @YarivCol is working on a wrapper for the osEventFlags #4517, but it's not ready to merge yet.

Lets look at how other libraries handle semaphore overflow:

Linux returns EOVERFLOW, but otherwise saturates (ref)
Windows returns false, but otherwise saturates (ref)
FreeRTOS returns errQUEUE_FULL, but otherwise saturates (ref)
Java, an exception heavy language, just saturates without warning (ref)

By changing the semaphores behaviour and ruling out the use of semaphores for signaling, we're challenging the user's expectations rather than providing a tool they are familiar with.

This patch removes the assertion on semaphore overflow and instead relies on the semaphore saturation introduced in RTX 5. This should match how a user expects a semaphore to behave and returns the ability to use semaphores for signaling.

related issues #4580, #4575, #4571, #4560, #4517, #4500, #4492, #4449
cc @sg-, @c1728p9, @deepikabhavnani, @pan-, @kjbracey-arm

theotherjimmy · 2017-06-16T22:12:00Z

rtos/Semaphore.cpp

@@ -27,7 +27,7 @@
 namespace rtos {

 Semaphore::Semaphore(int32_t count) {
-    constructor(count, 1024);
+    constructor(count, UINT16_MAX);


Forgot your include of stdint.h?

Ah good catch 👍

Thanks, but I just translated the CI failure.

Actually including stdint.h didn't work. UINT16_MAX isn't defined until c++11/c99. I've just updated to a hex literal.

Before rtx 5, the max count on semaphores was UINT16_MAX, aftewards it was decreased to 1024 with an assert on overflow. This is especially problematic for semaphores used for signaling, since there is no replacement currently available in C++.

AnotherButler · 2017-06-16T22:44:56Z

@geky Please capitalize "RTOS" and change "Unbreak" to "Fix" or "Improve" in your subject line. Thank you.

pan-

Thanks for the submission that will help a lot to have the previous behavior back.

0xc0170 · 2017-06-17T11:28:00Z

@geky Please capitalize "RTOS" and change "Unbreak" to "Fix" or "Improve" in your subject line. Thank you.

Once done, please start morph CI

deepikabhavnani · 2017-06-17T21:11:13Z

@geky: With this change, i see issue in future with current socket implementation. Semaphores are used for signalling and with every release we allow count to go high till max, but in case of connectivity error (or some bug) receive won't wait for any release signal and continue processing till current count > 0. This means we are pushing bug/connectivity issue to MAX count. You are anyways removing assertion from overflow, what is the need to increase overflow count till max?

geky · 2017-06-19T03:02:22Z

@deepikabhavnani, that's a good thought, and I'm still heavily in favor of #4580. But even with these slow microcontrollers software-only oprations are very fast. The time it takes to decrement the semaphore all the way down from max count is trivial compare to how long a network operation is.

You're right we haven't seen a reason to increase the semaphore back up to 65536, but I haven't seen a reason we're at the current value of 1024, and 65536 is at least backwards compatible.

deepikabhavnani · 2017-06-19T05:15:22Z

@geky: requirement for queue and sockets is to have binary semaphore, shouldn't we set max count argument as 1 for them (not default). Other alternative would be to check the count before performing release and make sure count does not go beyond 1.

kjbracey

Agree with this - for the reasons you've explained we've lots of existing semaphore users who could tend to saturate, and we've not had time to migrate them to anything better.

At some point, allowing it to assert again might be appropriate, once we've got C++ EventFlags (and maybe ConditionVariable?) for these sorts of users migrate to.

0xc0170 · 2017-06-19T07:56:37Z

@tommikas Can you please restart CI , there are at least two jobs from Friday with no update. I cancelled some, please check if I was able to or restart them please.

This one seems to be started 30 mins ago, after the weekend. Did you restart CI ? Should we just wait or it wont update ?

Thanks

tommikas · 2017-06-19T08:10:03Z

@0xc0170 There's been some trouble with Jenkins, requiring at least one restart on friday. That caused a build to hang, blocking all the builds over the weekend. The blockage has been cleared and builds are running now but there's quite a bit of a queue built up. The ones you restarted seem to be running too, it'll just take Jenkins some time to get to them.

0xc0170 · 2017-06-19T10:31:38Z

/morph test-nightly

adbridge · 2017-06-19T11:24:54Z

Fixes #4584

bulislaw

LGTM

mbed-bot · 2017-06-19T20:31:59Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 585

All builds and test passed!

geky mentioned this pull request Jun 16, 2017

TCP socket semaphores changed to binary sempahore #4560

Closed

theotherjimmy reviewed Jun 16, 2017

View reviewed changes

geky force-pushed the rtos-unbreak-semaphore branch from da09aec to 6b02cea Compare June 16, 2017 22:19

pan- approved these changes Jun 16, 2017

View reviewed changes

0xc0170 approved these changes Jun 17, 2017

View reviewed changes

geky changed the title ~~rtos: Unbreak semaphore~~ RTOS: Fix semaphore Jun 19, 2017

kjbracey approved these changes Jun 19, 2017

View reviewed changes

0xc0170 added the needs: CI label Jun 19, 2017

bulislaw approved these changes Jun 19, 2017

View reviewed changes

0xc0170 added the release-version: 5.5.1 label Jun 19, 2017

theotherjimmy added ready for merge and removed needs: CI labels Jun 19, 2017

theotherjimmy merged commit 77b6127 into ARMmbed:master Jun 19, 2017

theotherjimmy removed the ready for merge label Jun 19, 2017

kegilbert mentioned this pull request Jun 28, 2017

Semaphore release occurs before acquisition for TCP socket over UART #4575

Closed

RTOS: Fix semaphore #4579

RTOS: Fix semaphore #4579

Uh oh!

Conversation

geky commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

geky Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

theotherjimmy Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

geky Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

AnotherButler commented Jun 16, 2017

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jun 17, 2017

Uh oh!

deepikabhavnani commented Jun 17, 2017

Uh oh!

geky commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani commented Jun 19, 2017

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tommikas commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jun 19, 2017

Uh oh!

adbridge commented Jun 19, 2017

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

mbed-bot commented Jun 19, 2017

Result: SUCCESS

Output

Uh oh!

Uh oh!

geky commented Jun 16, 2017 •

edited

Loading

geky commented Jun 19, 2017 •

edited

Loading

0xc0170 commented Jun 19, 2017 •

edited

Loading

tommikas commented Jun 19, 2017 •

edited

Loading