core: add thread flags #4103

kaspar030 · 2015-10-16T11:08:41Z

This PR adds functionality to set multiple (32) "flags" per thread, and also wait for them in any combination, efficiently.

While adding 8 bytes to each TCB, this method allows faster conditional sleep than mutexes and is way more flexible. It is possible to implement mutex_wait and msg_wait with these flags, making it easy to wait for, e.g., a button to be pressed, a mutex to become free and a timer to be shot, at the same time.

This is WIP, but as people are asking (#4102), I decided to share.

OlegHahm · 2015-10-27T19:01:17Z

While adding 8 bytes to each TCB, this method allows faster conditional sleep than mutexes and is way more flexible.

Wow, do you remember us celebrating when we could minimize tcb size by a couple of bytes a year ago or so?

I have to admit that I haven't understood the whole idea of this PR, but trading more RAM against improved runtime seems a to be a bad compromise on IoT devices.

zhuoshuguo · 2016-01-23T10:10:20Z

@kaspar030 Ping, a kind reminder for rebase.

kaspar030 · 2016-01-26T12:10:29Z

rebased

kaspar030 · 2016-01-26T12:22:29Z

I have to admit that I haven't understood the whole idea of this PR

A common use case is what we're usually using mutexes for: wait for an event that might happen in an isr, eg., arrival of a network packet or a byte on a serial line.

Currently, a thread usually waits in a loop:

mutex_lock();
while (1) {
   mutex_lock());
   /* handle event */
 mutex_unlock();
}

and an ISR would unlock the mutex whenever an event occurs. You can find that kind of logic (sometimes using messages) all over the place.

While it kinda works, it has several disadvantages:

it is only possible to wait for one mutex, or a message, ...
if a message is used to pass events, and if the message queue is full, the event is lost (remember "possible lost interrupt"?)

The main concept behind this PR is that events can be atomically flagged and waited for in any combination:

while(1) {
  flag = thread_flag_wait_any(); /* waits for flag */
  switch(flag) {
   case FLAG_MSG:
     msg_receive(&msg);
   case FLAG_TAPDEV_ISR:
   handle_isr();
 }
}

As is, no flags for messages or mutexes are generated, but that's easy to add.

Wow, do you remember us celebrating when we could minimize tcb size by a couple of bytes a year ago or so?

Of course! ;) I will try to move the needed struct out of tcb_t. I had that in an early version, but it was less efficient...

Kijewski · 2016-01-26T12:28:13Z

core/include/atomic.h

+ *
+ * @return The value of *var* before the operation.
+ */
+static inline int atomic_or(atomic_int_t *var, int val)


Maybe make this file a PR on its own, and also add xor and not to complete the family?

OlegHahm · 2016-01-27T01:53:56Z

Thanks for the explanation.

kaspar030 · 2016-02-24T19:55:30Z

I've changed the implementation to use tcb->wait_data for the wait mask, and also changed the flags type to uint16_t. That way, on 32bit platforms, sizeof(tcb_t) doesn't get bigger due to alignment luck, and gains 2 byte on other platforms.

Also, I've changed core/msg.c to set some flags and xtimer_msg_receive_timeout() to make use of them.

OlegHahm · 2016-02-24T20:31:30Z

Nice!

kaspar030 · 2016-02-24T21:38:30Z

changed the message flag semantics so it now always shows whether any message is waiting

@OlegHahm, would you like to review this?

Should I split the the msg changes?

OlegHahm · 2016-02-24T21:44:08Z

core/include/tcb.h

    char *sp;                   /**< thread's stack pointer         */
    uint16_t status;            /**< thread's status                */

    kernel_pid_t pid;           /**< thread's process id            */
    uint16_t priority;          /**< thread's priority              */

+    thread_flags_t flags;       /**< currently set flags            */


Couldn't this be optional, too, if we have a pseudomodule anyway?

It could. But I've also changed msg.c to use thread flags internally, which makes some things a lot nicer.

e.g., currently in xtimer_receive_msg_timeout, there's a race between receiving the message and deleting the timer, and if the thread has a message queue, that might lead to the timer message to be queued right after the actual received message.

This PR's xtimer_receive_msg_timeout() implementation uses a thread flag to pass the timeout information, solving the race problem.

The pseudomodule basically only disables the thread flags user API.

edit sorry, the messaging changes are unrelated to the xtimer changes.

Ok, I see. I guess it makes sense.

OlegHahm · 2016-02-24T21:52:29Z

@OlegHahm, would you like to review this?

I can give it a try.

Should I split the the msg changes?

Would make it easier to review.

kaspar030 · 2016-02-24T22:25:05Z

cut out msg and xtimer changes

kaspar030 · 2016-02-24T22:30:32Z

core/include/tcb.h

@@ -1,7 +1,7 @@
 /*
 * Copyright (C) 2013 Freie Universität Berlin
 *
- * This file is subject to the terms and conditions of the GNU Lesser
+ * This file is subject to the terms and thread_conditions of the GNU Lesser


kaspar030 · 2016-03-29T20:42:01Z

I this still going into the release? Or should we postpone it?

let me rebase and have another look. IMHO it should be ready.

kaspar030 · 2016-03-29T21:20:00Z

rebased on 4923

@haukepetersen IMHO ready to merge.

haukepetersen · 2016-03-29T21:39:27Z

core/include/thread_flags.h

+thread_flags_t thread_flags_wait_all(thread_flags_t mask);
+
+/**
+ * @brief Wait for any flags in mask to become set (blocking), one at a time


most or least significant first? Please document

haukepetersen · 2016-03-29T22:01:14Z

Code looks good and works as expected (at least the test application)

ACK when addressed, rebased and squashed.

OlegHahm · 2016-03-29T23:44:39Z

Needs a rebase after #4923 got merged.

kaspar030 · 2016-03-30T08:46:25Z

rebased

kaspar030 · 2016-03-30T09:19:52Z

Dammit, removed the "waiting for pr" label too late...

kaspar030 · 2016-03-30T09:20:59Z

@cgundogan would you take another look?

kaspar030 · 2016-03-30T09:21:48Z

@cgundogan Ah, not needed, there are already two ACKs. ;)

cgundogan · 2016-03-30T09:22:31Z

if you don't want my personal ACK (;

kaspar030 · 2016-03-30T09:28:02Z

Don't want to overuse your ACKs, I know reviewing is hard! :)

cgundogan · 2016-03-30T09:56:29Z

core/thread_flags.c

+{
+    unsigned state = irq_disable();
+    mask &= thread->flags;
+    thread->flags &= ~mask;


I am not 100% sure, but I think

mask &= thread->flags; thread->flags &= ~mask;

can be reduced to thread->flags &= ~mask; only?

No, as the function needs to return the actually cleared flags.

dammit, I didn't look that far..

kaspar030 · 2016-03-30T10:17:50Z

Murdock is fine -> go.

cgundogan · 2016-03-30T10:22:21Z

core/thread_flags.c

+    _thread_flags_wait_any(mask);
+    thread_t *me = (thread_t*) sched_active_thread;
+    unsigned tmp = me->flags & mask;
+    return _thread_flags_clear_atomic(me, thread_flags_clear(1 << bitarithm_lsb(tmp)));


If you are trying to unset all bits, but not the least significant one, then you may use tmp &= -tmp, or tmp &= ~tmp + 1 (equivalent) instead of 1 << bitarithm_lsb(tmp).

Well, I'm trying to unset only the least signigicant bit. :)

1 << bitarithm_lsb(tmp) isn't this the same as unsetting all bits but not the lsb?

Ah yes.

>>> a = 0x1110; a &= -a; print(bin(a)) 0b10000 >>>

Is this due to pythons signed bit handling?

yes, this only works if the number is unsigned. The number gets negated and 1 is added to the result for negative unsigned numbers .. AFAIK

See #5200. Saves a whopping 32 bytes of code. ;)

(16byte on 32bit ARM).

kaspar030 mentioned this pull request Oct 16, 2015

threads: introduce new state #4102

Closed

kaspar030 mentioned this pull request Nov 4, 2015

POSIX sockets: implement (p)select #4222

Closed

OlegHahm modified the milestone: Release 2016.03 Dec 8, 2015

kaspar030 force-pushed the add_thread_flags branch from 2aac8a0 to aa83865 Compare January 23, 2016 19:59

Kijewski reviewed Jan 26, 2016
View reviewed changes

miri64 assigned Kijewski Jan 26, 2016

kaspar030 force-pushed the add_thread_flags branch from aa83865 to 33ac75f Compare February 24, 2016 19:53

OlegHahm reviewed Feb 24, 2016
View reviewed changes

OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 24, 2016

kaspar030 force-pushed the add_thread_flags branch from 9283d7f to 450fe77 Compare February 24, 2016 22:24

kaspar030 assigned OlegHahm and unassigned Kijewski Feb 24, 2016

kaspar030 reviewed Feb 24, 2016
View reviewed changes

kaspar030 force-pushed the add_thread_flags branch from 666a0c0 to bde6f47 Compare March 29, 2016 21:19

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

haukepetersen reviewed Mar 29, 2016
View reviewed changes

kaspar030 force-pushed the add_thread_flags branch from 22ca025 to 6d15f62 Compare March 30, 2016 08:38

kaspar030 added 2 commits March 30, 2016 10:38

core: introduce thread flags

abff2e3

tests: thread_flags: initial commit

37bbdac

kaspar030 force-pushed the add_thread_flags branch from 6d15f62 to 37bbdac Compare March 30, 2016 08:39

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 30, 2016

kaspar030 mentioned this pull request Mar 30, 2016

core: make messaging optional #3851

Merged

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 30, 2016

kaspar030 assigned cgundogan Mar 30, 2016

cgundogan reviewed Mar 30, 2016
View reviewed changes

kaspar030 added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Mar 30, 2016

kaspar030 merged commit 5f81284 into RIOT-OS:master Mar 30, 2016

kaspar030 deleted the add_thread_flags branch March 30, 2016 10:17

cgundogan reviewed Mar 30, 2016
View reviewed changes

core: add thread flags #4103

core: add thread flags #4103

Uh oh!

Conversation

kaspar030 commented Oct 16, 2015

Uh oh!

OlegHahm commented Oct 27, 2015

Uh oh!

zhuoshuguo commented Jan 23, 2016

Uh oh!

kaspar030 commented Jan 26, 2016

Uh oh!

kaspar030 commented Jan 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegHahm commented Jan 27, 2016

Uh oh!

kaspar030 commented Feb 24, 2016

Uh oh!

OlegHahm commented Feb 24, 2016

Uh oh!

kaspar030 commented Feb 24, 2016

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!

OlegHahm commented Feb 24, 2016

Uh oh!

kaspar030 commented Feb 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Mar 29, 2016

Uh oh!

kaspar030 commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Mar 29, 2016

Uh oh!

OlegHahm commented Mar 29, 2016

Uh oh!

kaspar030 commented Mar 30, 2016

Uh oh!

kaspar030 commented Mar 30, 2016

Uh oh!

kaspar030 commented Mar 30, 2016

Uh oh!

kaspar030 commented Mar 30, 2016

Uh oh!

cgundogan commented Mar 30, 2016

Uh oh!

kaspar030 commented Mar 30, 2016

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!

kaspar030 commented Mar 30, 2016

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!

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!