Semaphore implementation without IPC #6155

lebrush · 2016-11-22T17:22:53Z

This is a working semaphore implementation using mutex and thread_flags. It seems to work fine but firstly this is a request for comments.

(fix for #6151)

Depends on: #6158

miri64

First superficial look.

miri64 · 2016-11-22T17:26:09Z

sys/include/xtimer.h

+ * @return       0, when returned after mutex was locked
+ * @return       -1, when the timeout occcured
+ */
+int xtimer_mutex_lock_timeout(mutex_t *mutex, uint64_t us);


I would prefer this change to xtimer to be introduced in a separate PR.

Might make sense, it's in a separate commit in any case :-)

Yes, but from a review perspective it makes even more sense. The know-how about semaphores and xtimer can be assumed to be split among the reviewers, so this PR would essentially need to ACKs: one from a person knowing about xtimer and one about semaphores ;-).

miri64 · 2016-11-22T17:28:18Z

sys/include/sema.h

+ *
+ * @return  Current value of the semaphore.
+ */
+#define SEMA_VALUE(sema)        ATOMIC_VALUE(sema->value)


Can this be a inline function instead.

miri64 · 2016-11-22T17:28:35Z

sys/include/sema.h

-    priority_queue_t queue;         /**< list of threads waiting for the semaphore */
+    atomic_int_t value;             /**< value of the semaphore */
+    sema_state_t state;             /**< state of the semaphore */
+    mutex_t mutex;                  /**< mutex of the semaphore */


Do you really need thread flags and mutex?

Absolutely right, I removed the flags dependency 👍

mh.... actually I meant it the other way around. sema used to be based on flags (or rather posix_semaphore was before we took the base implementation out of the POSIX wrapper and made it message-based) and it didn't work out as we thought it might (see also size of the current sema_t struct vs how it was before).

Before it was unsigned int + priority_queue_node_t, assuming a 32-bit processor this is 4 + 12 = 16 byte. Now it is unsigned int + enum + mutex_t which is 4+4+4 = 12 bytes. This is 4 byte less. One could remove the enum and use signed int for the value (negative meaning destroy) and could save 4 bytes more (the posix implementation uses a signed int).

I liked the point to remove the flags dependency because adding the flags adds 2 bytes to every thread stack even if the semaphore is only used by some of the threads.

Okay, I somehow thought priority_queue_node_t was just 4 byte...

lebrush · 2016-11-22T18:12:08Z

I found two race conditions after creating the PR. I'll tackle them tomorrow.

Kijewski · 2016-11-22T23:18:01Z

I am inclined to remove the "enhancement" label because you removed the int sema_wait_timed_msg(sema_t *sema, uint64_t timeout, msg_t *msg) functionality with no replacement. What to do when I get a message while waiting for a semaphore? Who says that the semaphore is more important than a message?

lebrush · 2016-11-23T08:09:25Z

@Kijewski good point. The same situation could be claimed however for the mutex: what if you receive a message while in a mutex? both mutex and semaphore are concurrency control mechanisms. When you wait in your code for something (i.e. mutex) you wait for it, the messages are going to be queued.

In this case I modified the sema implementation so it doesn't use messages anymore, hence the sema_wait_timed_msg (not used anywhere in the code btw, except by lwip) is not applying anymore.

lebrush · 2016-11-23T09:16:46Z

@miri64 I fixed the race conditions and:

removed dependency to atomic
removed dependency to thread_flags

miri64 · 2016-11-23T13:25:51Z

sys/include/sema.h

- * @return  -ECANCELED, if the semaphore was destroyed.
- * @return  -EAGAIN, if the thread received a message while waiting for the lock.
- */
-int sema_wait_timed_msg(sema_t *sema, uint64_t timeout, msg_t *msg);


If you remove that: Doesn't lwIP needs fixing, too?

No, lwIP was calling sema_wait_timed which was calling sema_wait_timed_msg. However, your next point about the return values makes that lwIP requires fixing, I'll rollback the return values to the original ones.

miri64 · 2016-11-23T13:28:44Z

sys/include/sema.h

@@ -129,22 +117,28 @@ int sema_wait_timed(sema_t *sema, uint64_t timeout);
 * @param[in]  sema A semaphore.
 *
 * @return  0 on success
- * @return  -EINVAL, if semaphore is invalid.
- * @return  -ECANCELED, if the semaphore was destroyed.
+ * @return  -1 on error (i.e. semaphore was destroyed)


Here and in other places: Changing these return values changes the behavior of the POSIX wrapper (it uses the result to set errno, as specified by the POSIX specifications). Either we drop this behavior (and don't care about POSIX compatibility and also fix the POSIX wrapper accordingly) or these values needs to stay the same.

I restored the "old" return codes 👍

miri64 · 2016-11-23T14:54:25Z

@Kijewski

I am inclined to remove the "enhancement" label because you removed the int sema_wait_timed_msg(sema_t *sema, uint64_t timeout, msg_t *msg) functionality with no replacement. What to do when I get a message while waiting for a semaphore? Who says that the semaphore is more important than a message?

This PR removes the dependency of IPC from sema, so you won't receive a message while waiting for a semaphore. Did I get this right @lebrush?

lebrush · 2016-11-23T16:30:54Z

Right

lebrush · 2016-11-24T07:52:45Z

@miri64 @Kijewski this has been now split into two PR this one tackling the Semaphore and #6158 for the mutex implementation with timeout. This one will not build without #6158

miri64

~~lwIP wrappers in pkg/lwip/contrib/sys_arch.c need adaption too, btw.~~ (Sorry, thought the API change was more significant).

miri64 · 2016-12-13T16:16:19Z

sys/include/sema.h

+ *
+ * @return  Current value of the semaphore.
+ */
+static inline unsigned int sema_value(sema_t *sema)


Why is this prefered over just taking sema->value (either remove function or rationalize in Doxygen).

Actually it doesn't make a difference, so I removed it 👍 Then there was no need to touch the semaphore.h file :)

miri64 · 2016-12-13T16:19:44Z

sys/sema/sema.c

- * @author  Christian Mehlis <mehlis@inf.fu-berlin.de>
- * @author  Martine Lenders <mlenders@inf.fu-berlin.de>
- * @author  René Kijewski <kijewski@inf.fu-berlin.de>
+ * @author  Víctor Ariño <victor.arino@zii.aero>


Keeping us in the Copyright but removing our authorship? ;-)

(though given the amount of code left I understand the latter step)

Yes, I'm not very clear with that, sorry. Maybe we should write a guideline on when to add/remove an author from a file (I think there's currently any, right?). The criteria I followed is:
sema.h I did some significant additions but most of the code remained, so I added myself. In sema.c the code changed completely, so there was no code left from the previous authors, neither the authors.

I didn't want to remove the copyright because this is the legal thingy... but maybe I should remove it as well. Or do you prefer that I keep all authors and all copyrights?

I don't really care... Just thought it looked quaint that the copyright was amended and the authors stripped ;-)

miri64

Last review. One comment that might need some documentation: I see a problem if the semaphore is destroyed and immediately reused afterwards (using sema_create()). Maybe it should be documented somewhere that a semaphore shall only be reused after all its subscribers received the semaphore's destruction.

miri64 · 2016-12-14T08:34:52Z

sys/include/sema.h

@@ -18,13 +18,14 @@
 * @author  Martine Lenders <mlenders@inf.fu-berlin.de>
 * @author  Christian Mehlis <mehlis@inf.fu-berlin.de>
 * @author  René Kijewski <kijewski@inf.fu-berlin.de>
+ * @author  Víctor Ariño <victor.arino@zii.aero>
 */
 #ifndef SEM_H_


Minor legacy error that might be fixable in this PR: s/SEM_H_/SEMA_H_/g

miri64 · 2016-12-14T08:35:41Z

sys/include/sema.h

 *
 * @return  Statically initialized semaphore.
 */
-#define SEMA_CREATE(value)      { (value), PRIORITY_QUEUE_INIT }
+#define SEMA_CREATE()           { (0), SEMA_OK, MUTEX_INIT() }


I see benefit in initializing a semaphore with a non-zero value e.g. to let more than one thread pass initially (furthermore, you also kept it for dynamic-equivalent sema_create()). So why change it? Or why must it be 0 (as per documentation).

Good point, actually it can only be initialized with a value different from 0. In order to initialize with 0 I need to initialize the mutex as locked, and this requires a change in the mutex.h file to make it clean (see #6210).

If that get's merged fast then I'll update this PR, if this PR gets merged first, I'll create a new one :-) The only addition would be:

/** * @brief Creates 0 initialized semaphore statically. * @return Statically initialized semaphore. */ #define SEMA_CREATE_EMPTY() { (value), SEMA_OK, MUTEX_INIT_LOCKED }

miri64 · 2016-12-14T08:36:51Z

sys/include/sema.h

@@ -58,7 +68,6 @@ typedef struct {
 * @param[in] value Initial value for the semaphore.
 *
 * @return  0 on success.
- * @return  -EINVAL, if semaphore is invalid.


Here and below: please document the assertions you make in the implementation using the @pre doxygen command.

miri64 · 2016-12-14T11:18:52Z

sys/include/sema.h

@@ -33,23 +35,35 @@ extern "C" {
 /**
 * @brief   Creates semaphore statically.
 *
- * @param[in] value Initial value for the semaphore.
+ * @param[in] value Initial value for the semaphore (can't be 0). For a 0
+ *                  initialized semaphore @see SEMA_CREATE_EMPTY


SEMA_CREATE_EMPTY does not exist

I don't understand the reasoning behind this restriction.

See #6155 (comment) the restriction is that when the semaphore value is 0 the mutex needs to be locked.

Ah sorry, github hid that from me.

miri64 · 2016-12-14T11:36:23Z

@lebrush wrote in a hidden comment

If that get's merged fast then I'll update this PR, if this PR gets merged first, I'll create a new one :-) The only addition would be:
/**
 * @brief   Creates 0 initialized semaphore statically.
 * @return  Statically initialized semaphore.
 */
#define SEMA_CREATE_EMPTY()        { (value), SEMA_OK, MUTEX_INIT_LOCKED }

I would call the the macro something like SEMA_CREATE_LOCKED since a semaphore isn't really something that is "filled" (i.e. it can't be empty).

lebrush · 2016-12-15T10:15:02Z

Your wishes, my commands ;-) done! now there's an issue with the list_remove and some platforms... (see #6211) I'll have a look later, any hint? Maybe by just changing the return value of list_remove to void...

miri64 · 2017-01-12T19:18:52Z

Aand #6158 got merged.

lebrush · 2017-01-13T08:33:38Z

Super cool! 💃 I just rebased and squashed this one as well
@miri64 can you approve the changes and set the ready for ci, please? :-) thanks!

lebrush · 2017-01-16T12:30:25Z

ping @miri64

miri64

Some minor kinks left.

miri64 · 2017-01-16T13:30:26Z

sys/include/sema.h

 * @param[in] sema  The semaphore to destroy.
 *
 * @return  0 on success.
- * @return  -EINVAL, if semaphore is invalid.
 */
 int sema_destroy(sema_t *sema);


Might be problematic, since it is an API change, but since we only have one state after return it might make sense to set the return type to void.

I left it to keep the old return values meaningful. This implementation can't fail here so the return value makes no difference, but the same applies to the creation of the semaphore.

I'm happy to change it to void... I've no strong opinion about this one... your call.

void would minimize the code which in our use case is priority no. 1 ;-). So please go ahead.

miri64 · 2017-01-16T13:30:44Z

sys/include/sema.h

@@ -58,92 +78,76 @@ typedef struct {
 * @param[in] value Initial value for the semaphore.
 *
 * @return  0 on success.
- * @return  -EINVAL, if semaphore is invalid.


Might be problematic, since it is an API change, but since we only have one state after return it might make sense to set the return type to void.

About this you already complained earlier (here and here). The old return codes were restored (except if they made no sense anymore) and we aded a @pre tag. In any case this code is only used inside the posix semaphore and lwip and both have been adapted and tested respectively.

About this you already complained earlier (here [1] and here [2]).

Mh, I would argue that [1] was about the change of the value itself and [2] was about the undocumented precondition (which is a different scope than return values because an unmet precondition leads to a crash, while an error reported by a return-value can be handled gracefully). However the comment in OP of this conversation was about arguing to remove, because according to this documentation the function only has one possible return value (0) and will always succeed, so having a return value seems redundant.

miri64 · 2017-01-16T13:31:43Z

sys/include/sema.h

 *
- * @return  0 on success
- * @return  -EINVAL, if semaphore is invalid.
+ * @return  >= 0, on success number of left posts


Weird sentence structure

Me English no native. Will fix ;-)

miri64 · 2017-01-16T13:32:39Z

Sorry, lost track of this one :-/

lebrush · 2017-01-18T13:29:21Z

@miri64 I changed sema_try_wait to return 0 on success, the other situation makes no sense and this way all the API is consistent.

Regarding the return values for sema_create and sema_destroy. they are used in the POSIX implementation and in the lwip adaption. I'd leave them in even if they are always 0 just in case the implementation ever changes.

Let me know if this is Ok for you and I'll squash :-)

miri64

Tests are still working. Needs squashing.

lebrush · 2017-01-18T14:29:04Z

I made them in separate commits for you to easy spot them. Gonna squash :-)

miri64 · 2017-01-18T15:16:39Z

The error Jenkins reports was my bad in #5937 and is revealed, since you removed the include of timex.h from sema.h. Can you please squash the following patch onto the pkg/lwip commit:

diff --git a/pkg/lwip/contrib/sock/lwip_sock.c b/pkg/lwip/contrib/sock/lwip_sock.c
index 8ae799b..20b5faa 100644
--- a/pkg/lwip/contrib/sock/lwip_sock.c
+++ b/pkg/lwip/contrib/sock/lwip_sock.c
@@ -19,6 +19,7 @@
 #include "net/ipv4/addr.h"
 #include "net/ipv6/addr.h"
 #include "net/sock.h"
+#include "timex.h"
 
 #include "lwip/err.h"
 #include "lwip/ip.h"

lebrush · 2017-01-18T15:24:18Z

Thanks for being so responsive :-) just added your patch.
Btw is there a way to check the output of Jenkins quickly without downloading the artifacts? (the fancy new interface seems not to work)

miri64 · 2017-01-18T15:49:56Z

Btw is there a way to check the output of Jenkins quickly without downloading the artifacts? (the fancy new interface seems not to work)

That you have to ask @cgundogan or @smlng. My browser (chrome) just opens the .log files instead of downloading them

lebrush · 2017-01-19T08:00:10Z

mmm locally it works, just rebased to trigger Mr. Murdock again.

arduino_hello-world:nucleo-f030:
Building application "arduino_hello-world" for "nucleo-f030" with MCU "stm32f0".

/data/riotbuild/examples/arduino_hello-world/_sketches.cpp:81:5: error: redefinition of 'int ledPin'
 int ledPin = ARDUINO_LED;
     ^~~~~~
/data/riotbuild/examples/arduino_hello-world/_sketches.cpp:18:5: note: 'int ledPin' previously defined here
 int ledPin = ARDUINO_LED;
     ^~~~~~
(...)

lebrush · 2017-01-19T10:12:33Z

@miri64 green lights everywhere!! 🎉

miri64 · 2017-01-19T11:22:33Z

Then go.

lebrush mentioned this pull request Nov 22, 2016

Sema dropping messages #6151

Closed

miri64 requested changes Nov 22, 2016

View reviewed changes

miri64 added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Nov 22, 2016

miri64 assigned miri64 and kaspar030 Nov 22, 2016

miri64 added this to the Release 2017.01 milestone Nov 22, 2016

lebrush force-pushed the sema-non-ipc branch from e979f92 to 7d5ab34 Compare November 23, 2016 08:59

kaspar030 mentioned this pull request Nov 23, 2016

xtimer: timer already in the list #4841

Closed

miri64 reviewed Nov 23, 2016

View reviewed changes

lebrush force-pushed the sema-non-ipc branch 3 times, most recently from 39e14d2 to 813a9bb Compare November 23, 2016 14:27

lebrush mentioned this pull request Nov 24, 2016

sys: add mutex lock with timeout function #6158

Merged

lebrush force-pushed the sema-non-ipc branch from 813a9bb to 3147907 Compare November 24, 2016 07:50

miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 29, 2016

miri64 requested changes Dec 13, 2016

View reviewed changes

miri64 reviewed Dec 13, 2016

View reviewed changes

lebrush force-pushed the sema-non-ipc branch from 3147907 to b2203b8 Compare December 14, 2016 08:13

miri64 previously requested changes Dec 14, 2016

View reviewed changes

lebrush force-pushed the sema-non-ipc branch from b2203b8 to 934b02b Compare December 14, 2016 09:23

miri64 reviewed Dec 14, 2016

View reviewed changes

lebrush force-pushed the sema-non-ipc branch from c339950 to 73e80da Compare December 15, 2016 10:07

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 12, 2017

lebrush force-pushed the sema-non-ipc branch from 73e80da to e90c2e0 Compare January 13, 2017 08:32

miri64 requested changes Jan 16, 2017

View reviewed changes

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 16, 2017

miri64 approved these changes Jan 18, 2017

View reviewed changes

lebrush force-pushed the sema-non-ipc branch 2 times, most recently from 06a6081 to 6a84655 Compare January 18, 2017 15:12

lebrush force-pushed the sema-non-ipc branch from 6a84655 to 13e43f4 Compare January 18, 2017 15:22

Victor Arino added 3 commits January 19, 2017 08:59

sys/sema: re-implement without IPC

791b68a

sys/posix: adapt semaphore to new implementation

066db58

pkg/lwip: use new sema implementation

49adbf2

lebrush force-pushed the sema-non-ipc branch from 13e43f4 to 49adbf2 Compare January 19, 2017 07:59

miri64 merged commit 2a05385 into RIOT-OS:master Jan 19, 2017

lebrush deleted the sema-non-ipc branch January 19, 2017 12:28

lebrush mentioned this pull request Mar 6, 2017

sys: sema: semaphore bugfixes #6696

Merged

miri64 mentioned this pull request May 10, 2017

lwIP: hangs due to sema change after a while #7035

Closed

Semaphore implementation without IPC #6155

Semaphore implementation without IPC #6155

Conversation

lebrush commented Nov 22, 2016 • edited

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebrush Nov 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebrush commented Nov 22, 2016

Kijewski commented Nov 22, 2016 • edited

lebrush commented Nov 23, 2016

lebrush commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Nov 23, 2016

lebrush commented Nov 23, 2016

lebrush commented Nov 24, 2016

miri64 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Dec 14, 2016

lebrush commented Dec 15, 2016

miri64 commented Jan 12, 2017

lebrush commented Jan 13, 2017

lebrush commented Jan 16, 2017

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Jan 16, 2017

lebrush commented Jan 18, 2017

miri64 left a comment

Choose a reason for hiding this comment

lebrush commented Jan 18, 2017

miri64 commented Jan 18, 2017

lebrush commented Jan 18, 2017

miri64 commented Jan 18, 2017

lebrush commented Jan 19, 2017

lebrush commented Jan 19, 2017

miri64 commented Jan 19, 2017

lebrush commented Nov 22, 2016 •

edited

lebrush Nov 23, 2016 •

edited

Kijewski commented Nov 22, 2016 •

edited

miri64 left a comment •

edited