sys: add shared event threads by kaspar030 · Pull Request #12474 · RIOT-OS/RIOT

kaspar030 · 2019-10-16T18:39:51Z

Contribution description

Following the discussion in #12459, this PR shows how shared event threads could be implemented using sys/event.

This is basically a refactored version of https://github.com/haukepetersen/RIOT/tree/add_eventhandlerthread.

Testing procedure

Issues/PRs references

kaspar030 · 2019-10-16T18:42:05Z

With per-thread stack sizes, the array is not so simple anymore ;(. The ifdef based configuration looks quite ugly.

jia200x · 2019-10-16T19:34:09Z

Hi @kaspar030

AFAIS this is a follow up of #12459 (support for multiple threads using events). The mechanism is exactly the same. The event_thread module is a tasklets implementation.

Why don't we continue the discussion in #12459 first instead of opening a PR with the same principle? Or is this intended to be just a PoC?

kaspar030 · 2019-10-16T19:41:33Z

Why don't we continue the discussion in #12459 first instead of opening a PR with the same principle? Or is this intended to be just a PoC?

Well, my whole point is that "tasklets" are just event loops in dedicated threads. IMO that doesn't justify a new API. So yes, this is a PoC.

haukepetersen · 2019-10-17T07:01:18Z

As suggested, lets continue the discussion in #12459 for now.

maribu · 2019-10-17T07:30:22Z

The implementations of both the tasklet/irq_handler and the shared event thread only differ very little, as they both spawn (a) worker thread(s) that work on event queues.

This are the differences:

The irq_handler uses a bool to track if the event is already in the queue. This is not needed, as [event_post() will not touch the event queue if the event is already in the list]. But maybe this check with the bool is a bit faster and it allows for the return value of -EALREADY. (But maybe we adapt event_post() to return that, so it can be passed through? Or maybe there is simply no need to know that the even was already queued?)
The irq_handler/tasklet exposes a void * argument. By putting even_t in a struct and using container_of() this could also be achieved here, but might be a bit more effort to use.

For end users the option to having a void * argument with little effort could be nice. But that could be implemented as static inline functions on top of this API using the same implementation and having no overhead.

maribu · 2019-10-17T07:37:34Z

To be more concrete maybe a sketch of adding the void * version on top of this:

typedef struct {
    even_t event;
    void *arg;
} event_with_arg_t;

static inline void event_post_with_arg(event_with_arg_t *ev, void *arg)
{
    unsigned state = irq_disable();
    ev->arg = arg;
    event_post(&ev->event);
    irq_restore(state);
}

static inline void *event_get_arg(event_t *event_with_arg)
{
    event_with_arg_t *ev = (event_with_arg_t *)event_with_arg;
    return ev->arg;
}

This might be a bit more accessible for application developers if they actually need a void * argument.

haukepetersen · 2019-10-17T07:43:43Z

To be more concrete maybe a sketch of adding the void * version on top of this:

Its already there for quite a while: event/callback.h

maribu · 2019-10-17T07:48:30Z

OK. Then there is no need to for an additional API.

haukepetersen · 2019-10-17T07:51:42Z

OK. Then there is no need to for an additional API.

We getting closer to the point that @kaspar030 was raising in the first place :-)

sys/auto_init/auto_init.c

maribu · 2019-10-17T08:00:21Z

@kaspar030: Mind to steal the idea of using foo_params.h for configuration from #12480? With that, I'd say this fulfills all requirements that were stated so far.

jia200x · 2019-10-17T08:57:05Z

OK. Then there is no need to for an additional API.

See #12459 (comment)

That's my point of view. But of course, that doesn't affect the way the modules work nor the problems we are trying to solve. So, I'm fine with how most people want to proceed ;)

kaspar030 · 2019-10-17T09:21:07Z

@kaspar030: Mind to steal the idea of using foo_params.h for configuration from #12480?

I think it is not that easy, as #12480 defines the stacks within the header. If we ensure that it is only included from event_threads.c, that is ok, otherwise we'll have multiple definitions.
I'll try.

gschorcht · 2019-10-17T12:24:45Z

sys/event/thread.c

+#define EVENT_THREAD_LOWEST_STACKSIZE ISR_STACKSIZE
+#endif
+#ifndef EVENT_THREAD_LOWEST_PRIO
+#define EVENT_THREAD_LOWEST_PRIO   (THREAD_PRIORITY_IDLE - 1)


We would have at most exactly two priorities and two threads, right? Shouldn't the lowest thread priority be (THREAD_PRIORITY_MAIN - 1)? Otherwise drivers would have to use again the high priority thread to ensure that their events are at least handled before the main thread is executed.

Otherwise drivers would have to use again the high priority thread to ensure that their events are at least handled before the main thread is executed.

Maybe they want to, as in, "do this when there is time". That's why I've made it just one higher than idle.

How about I add a third priority ("medium"), which is just one higher than main? Or does the current very-low one make sense at all?

I mean, this is just the auto configuration, cpus / boards / applications can freely launch more handler threads...

How about I add a third priority ("medium"), which is just one higher than main? Or does the current very-low one make sense at all?

What priority should a thread have that is used by a driver to handle an interrupt from the hardware? IMHO, its priority should be at least higher than the priority of the main thread. A medium priority thread seems to be more important than a very low priority thread.

I mean, this is just the auto configuration, cpus / boards / applications can freely launch more handler threads...

Yes of course, but drivers can only use threads that are known from the auto configuration. Otherwise, they would need an additional thread parameter for the initializtion. This in turn would prevent the auto initialization.

What priority should a thread have that is used by a driver to handle an interrupt from the hardware?

I guess that depends... It is easy to add more priorities, so I propose I add one more middle priority (one higher than main) and see how far we go with that.

👍 The three priorities high (0), medium (main-1) and low (idle-1) probably meet the requirements of most use cases.

added medium priority

jia200x · 2019-10-21T11:00:25Z

(I copy&paste the comment from #12480 to continue the discussion here)

I think having multiple queues for now is a good approach. Eventually everything could be represented with a couple of queues, but this gives as a lot of flexibility for now.
One could think of:

#ifndef NET_RX_PRIO
#define NET_RX_PRIO TASKLET_PRIO_HIGH
#endif

#ifndef FOO_PRIO
#define FOO_PRIO TASKLET_PRIO_LOW
#endif

So, this gives the developer the mechanism to adapt the OS depending of the real time requirements.

jia200x · 2019-11-01T10:44:44Z

can we continue with this? The netif rework is waiting for a feature for handling radio events from within the OS (:

kaspar030 · 2020-01-09T14:07:08Z

can we continue with this?

I've added a test script for the test application and license / doxygen to the main code.

kaspar030 · 2020-01-09T14:10:48Z

rebased, squashed, conceptually not WIP anymore.

kaspar030 · 2020-01-09T14:17:19Z

@jia200x want to take another look?

jia200x · 2020-01-27T08:59:06Z

tests/event_threads/Makefile

@@ -0,0 +1,5 @@
+include ../Makefile.tests_common
+
+USEMODULE += event_thread_highest event_thread_medium event_thread_lowest


Can we define these as configuration values (CFLAGS) that depend on the module "event_thread"? Otherwise it's hard to configure it using Kconfig.

jia200x · 2020-01-27T09:05:37Z

Code-wise looks OK. Same for the fundamentals.
I will test it with the script (and maybe I rebase it into one of my PRs where I need this feature).

I only have some comments regarding the "event_thread_***" pseudomodules. Also, it would be great if you can add the Kconfig file for this module.

kaspar030 · 2020-02-06T22:32:33Z

I only have some comments regarding the "event_thread_***" pseudomodules. Also, it would be great if you can add the Kconfig file for this module.

Would you be OK with keeping Kconfig out of this PR for now? Kconfig is quite new to me...

jia200x · 2020-02-07T09:32:13Z

Would you be OK with keeping Kconfig out of this PR for now? Kconfig is quite new to me...

Ok, let's do it in a follow up then

kaspar030 · 2020-02-07T09:42:39Z

@maribu @gschorcht @jia200x I think all comments (apart from Kconfig) were addressed. Mind to take another look?

maribu

ACK. Very nicely boiled down the idea to small and maintainable code :-)

Some boards have to little RAM to compile the test and there is still a bug in the LPC2387 CPU (ISR_STACKSIZE is defined as ((unsigned) &__stack_irq_size), which is a super strange construct; why should an address of a variable be considered to be a size?). I suggest to blacklist that CPU for now, until that issue is fixed. (I could do so, unless @benpicco is faster ;-))

gschorcht · 2020-02-07T11:14:08Z

ACK from my side, please squash.

kaspar030 · 2020-02-07T11:20:01Z

Some boards have to little RAM to compile the test

yup, added them to Makefile.ci.

there is still a bug in the LPC2387 CPU (ISR_STACKSIZE is defined as ((unsigned) &__stack_irq_size), which is a super strange construct; why should an address of a variable be considered to be a size?)

Yeah, the linker can only provide addresses, but arbitrary values in those... I've adde arch_arm7 to FEATURES_BLACKLIST.

kaspar030 · 2020-02-07T11:21:44Z

squashed, adressed last comments

gschorcht · 2020-02-07T12:29:24Z

All lights are green, let's go.

gschorcht · 2020-02-07T12:36:20Z

@kaspar030 Thanks for the contribution. @maribu We should restart with the process you tried in PR #12486.

kaspar030 · 2020-02-07T21:29:23Z

Thanks everyone!

haukepetersen mentioned this pull request Oct 17, 2019

sys/tasklet: rename irq_handler to tasklet #12459

Closed

haukepetersen mentioned this pull request Oct 17, 2019

demonstrator: take on shared event handler threads #12480

Closed

maribu reviewed Oct 17, 2019

View reviewed changes

sys/auto_init/auto_init.c Show resolved Hide resolved

gschorcht reviewed Oct 17, 2019

View reviewed changes

maribu mentioned this pull request Oct 17, 2019

sys: Removed irq_handler #12486

Closed

This was referenced Nov 13, 2019

RFC: lower network stack rework #12688

Closed

[POC]: netif rework #12741

Closed

jia200x mentioned this pull request Nov 29, 2019

Extensebility of netdev_driver_t API #12469

Closed

kaspar030 force-pushed the add_event_threads branch 2 times, most recently from 0eaf8bd to 3581aef Compare January 9, 2020 14:06

kaspar030 changed the title ~~sys: add shared event threads (WIP)~~ sys: add shared event threads Jan 9, 2020

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 Jan 16, 2020

jia200x reviewed Jan 27, 2020

View reviewed changes

maribu approved these changes Feb 7, 2020

View reviewed changes

kaspar030 added 2 commits February 7, 2020 12:17

sys/event: add shared event threads

e01ad86

tests/event_threads: initial event threads test application

9a93805

kaspar030 force-pushed the add_event_threads branch from c968784 to 9a93805 Compare February 7, 2020 11:18

gschorcht merged commit 3514eec into RIOT-OS:master Feb 7, 2020

benpicco mentioned this pull request Feb 7, 2020

tests/event_threads: remove arch_arm7 from blacklist #13317

Merged

jia200x mentioned this pull request Feb 7, 2020

core/tasklet: add initial support #12420

Closed

kaspar030 deleted the add_event_threads branch February 7, 2020 22:03

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

		@@ -0,0 +1,5 @@
		include ../Makefile.tests_common

		USEMODULE += event_thread_highest event_thread_medium event_thread_lowest

Conversation

kaspar030 commented Oct 16, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kaspar030 commented Oct 16, 2019

Uh oh!

jia200x commented Oct 16, 2019

Uh oh!

kaspar030 commented Oct 16, 2019

Uh oh!

haukepetersen commented Oct 17, 2019

Uh oh!

maribu commented Oct 17, 2019

Uh oh!

maribu commented Oct 17, 2019

Uh oh!

haukepetersen commented Oct 17, 2019

Uh oh!

maribu commented Oct 17, 2019

Uh oh!

haukepetersen commented Oct 17, 2019

Uh oh!

Uh oh!

maribu commented Oct 17, 2019

Uh oh!

jia200x commented Oct 17, 2019

Uh oh!

kaspar030 commented Oct 17, 2019

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!

jia200x commented Oct 21, 2019

Uh oh!

jia200x commented Nov 1, 2019

Uh oh!

kaspar030 commented Jan 9, 2020

Uh oh!

kaspar030 commented Jan 9, 2020

Uh oh!

kaspar030 commented Jan 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jia200x commented Jan 27, 2020

Uh oh!

kaspar030 commented Feb 6, 2020

Uh oh!

jia200x commented Feb 7, 2020

Uh oh!

kaspar030 commented Feb 7, 2020

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

gschorcht commented Feb 7, 2020

Uh oh!

kaspar030 commented Feb 7, 2020

Uh oh!

kaspar030 commented Feb 7, 2020

Uh oh!

gschorcht commented Feb 7, 2020

Uh oh!

gschorcht commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Feb 7, 2020

gschorcht commented Feb 7, 2020 •

edited

Loading