Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/tasklet: add initial support #12420

Closed
wants to merge 2 commits into from
Closed

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Oct 10, 2019

Contribution description

This PR adds kernel support of tasklets, similar to the ones in Linux

Tasklets are snippets of code that are ran as soon as the OS finds a safe time. They can be scheduled several time but will only run once (they can be re-scheduled again after ran).

Scheduling tasklets is a thread safe operation and can be done from ISR context.

They are extremely useful for doing ISR offloading. E.g handling radios from the OS instead of the network stack. It's possible to change:

static void _irq_handler(void *arg)
{
    dev->event_callback(dev, NETDEV_EVENT_ISR); /* The stack offloads the ISR */
}

into

static void _irq_handler(void *arg)
{
    tasklet_schedule(&radio_isr_tasklet); /* The kernel offloads the ISR */
}

Since the device drivers registers the tasklet, it might not be necessary to have a dev->driver->isr netdev function with this method. And also, it ensures packet reception is preemptive since the tasklet run in the highest priority (see #11256).

Also, drivers or components that need to offload ISR wouldn't require to register a thread (e.g recv thread in some applications)

I think there was a comment somewhere from @gschorcht with a similar idea for offloading ISRs. It's also inspired in the work of @gebart in #9326.
The current implementation doesn't support priorities nor deadlines (for real-time constraints), but they could be added in follow ups.

Testing procedure

  • Check the documentation builds with make doc
  • Test with make -C tests/unittests tests-tasklet

Issues/PRs references

None so far, but it's would benefit the PHY & MAC rework

@jia200x jia200x added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Oct 10, 2019
@jia200x
Copy link
Member Author

jia200x commented Oct 10, 2019

I removed the receive thread from the driver_at86rf2xx using tasklets.
I applied this patch in the HEAD of this PR.

It works OK on iotlab-m3 and the ROM and RAM consumption went from:

   text    data     bss     dec     hex filename
  23628     508    4552   28688    7010 /home/jialamos/Development/RIOT/tests/driver_at86rf2xx/bin/iotlab-m3/tests_driver_at86rf2xx.elf

to

   text    data     bss     dec     hex filename
  23216     512    4056   27784    6c88 /home/jialamos/Development/RIOT/tests/driver_at86rf2xx/bin/iotlab-m3/tests_driver_at86rf2xx.elf

EDIT: Please ignore current results in RAM consumption, since the tasklet thread was initialized with THREAD_STACKSIZE_DEFAULT

@kaspar030
Copy link
Contributor

I think this could be easily modeled using event_callback and a thread running the event loop.

@kaspar030
Copy link
Contributor

@haukepetersen has a PoC here: https://github.com/haukepetersen/RIOT/tree/add_eventhandlerthread

Looking at the code, the struct used for tasklet is basically identical to event_callback. The tasklet list handling is also very similar. The main difference would be that event allows multiple queues, whereas tasklet as is would only allow one.

@gschorcht
Copy link
Contributor

gschorcht commented Oct 10, 2019

It seems to be exactly the same as we realized with the interrupt handler thread in PR #10555. The only difference is that the interrupt handler thread is realized on top of event queues. Although the name suggests that it is only suitable for interrupts, it could be used for other functions as well.

I suggest that we only implement one mechanism for the same problem. Otherwise, an application might have different threads that do the same thing. For example, a driver for an I2C device processes interrupts by using the interrupt handler thread while the application is using with the tasklets thread.

We could give the interrupt event handler a more general name, and if neded extend it with additional functions.

@gschorcht
Copy link
Contributor

The main difference would be that event allows multiple queues, whereas tasklet as is would only allow one.

@kaspar At the moment, we have the same problem with the interrupt handler thread.

@jia200x
Copy link
Member Author

jia200x commented Oct 11, 2019

Looking at the code, the struct used for tasklet is basically identical to event_callback. The tasklet list handling is also very similar. The main difference would be that event allows multiple queues, whereas tasklet as is would only allow one.

I'm aware this whole concept can be implemented with event queues. The only reason why I didn't do it was to not mix sys dependencies with core, but maybe it's better to stick to event_loops :)

It seems to be exactly the same as we realized with the interrupt handler thread in PR #10555. The only difference is that the interrupt handler thread is realized on top of event queues. Although the name suggests that it is only suitable for interrupts, it could be used for other functions as well.

I suggest that we only implement one mechanism for the same problem. Otherwise, an application might have different threads that do the same thing. For example, a driver for an I2C device processes interrupts by using the interrupt handler thread while the application is using with the tasklets thread.

That was exactly the purpose I was aiming for. I couldn't find the original references for the IRQ handler proposal and I was not aware of this feature. I totally agree that we should only aim to have one solution for this.

We could give the interrupt event handler a more general name, and if neded extend it with additional functions.

Well, the ISR handler thread is basically a tasklet mechanism. Any opinions on closing this PR and renaming the ISR thread handler concept to "tasklet"? I think it will be easier to find in the documentation.

@jia200x
Copy link
Member Author

jia200x commented Oct 11, 2019

@kaspar At the moment, we have the same problem with the interrupt handler thread.

As described in #10555, the only way to have preemptive events is with several threads. In Linux for instances, 2 are used for tasklets (tasklet_schedule() and tasklet_hi_schedule())

@gschorcht
Copy link
Contributor

gschorcht commented Oct 11, 2019

@kaspar At the moment, we have the same problem with the interrupt handler thread.

As described in #10555, the only way to have preemptive events is with several threads. In Linux for instances, 2 are used for tasklets (tasklet_schedule() and tasklet_hi_schedule())

@jia200x At the moment, the interrupt handler thread is realized as singleton. This has two advantages. First, there is no need to create the thread explicitly. It's done when it's used the first time. Second, as long as the irq_handler functions are not used, it doesn't occupy any memory since the compiler optimizes out the stack variable.

What are the options?

  1. Sure, we could extend API functions by adding an additional parameter of type kernel_pid_t *pid that is then used for adding irq_events/tasklets to the queue of a certain thread. It would still create the tasks implicitly, when the given pid is KERNEL_PID_UNDEF. However, the big question would be, who defines the pids as global variables? For example, device drivers wouldn't know what handler thread exist.

  2. We make it configurable how many threads are created. The thread pid variable would then be an array instead of a scalar. The minimum number of such threads would be 1. The priority of threads would have to be decreasing.

@gschorcht
Copy link
Contributor

Well, the ISR handler thread is basically a tasklet mechanism. Any opinions on closing this PR and renaming the ISR thread handler concept to "tasklet"? I think it will be easier to find in the documentation.

I would be fine if we would expand and rename the irq_handler. It's name was simply derived from the initial problem.

@jia200x
Copy link
Member Author

jia200x commented Oct 11, 2019

hi @gschorcht

@jia200x At the moment, the interrupt handler thread is realized as singleton. This has two advantages. First, there is no need to instantiate the thread explicitly. It's done when it's used the first time. Second, as long as the interrupt handler thread isn't used, it doesn't occupy any memory.

I'm a little bit confused with this statement that leads to the first proposal. Isn't the stack already allocated in RAM even before creating the thread? I get that if the function is never called, then the stack won't be allocated because of compiler optimizations, but in any other case the RAM usage would be the same. Or am I missing something?

We make it configurable how many threads are created. The thread pid variable would then be an array instead of a scalar. The minimum number of such threads would be 1. The priority of threads would have to be decreasing.

I think this is a good solution!

I would be fine if we would expand and rename the irq_handler. It's name was simply derived from the initial problem.

Ok, I can do it, no problem. Then, I can close this PR

@gschorcht
Copy link
Contributor

@jia200x At the moment, the interrupt handler thread is realized as singleton. This has two advantages. First, there is no need to instantiate the thread explicitly. It's done when it's used the first time. Second, as long as the interrupt handler thread isn't used, it doesn't occupy any memory.

I'm a little bit confused with this statement that leads to the first proposal. Isn't the stack already allocated in RAM even before creating the thread? I get that if the function is never called, then the stack won't be allocated because of compiler optimizations, but in any other case the RAM usage would be the same.

Yes, I expressed myself wrong, my fault. That is exactly what I meant. If you don't use the function the stack is optimized out. For example, the sizes for an Cortex-M with

   text	   data	    bss	    dec	    hex	filename
  11208	    180	   5228	  16616	   40e8	

and without calling the function.

   text	   data	    bss	    dec	    hex	filename
  10340	    180	   4152	  14672	   3950	

The difference in .bss results mainly from the default stack size of 1024 bytes.

Ok, I can do it, no problem. Then, I can close this PR

Great.

@gschorcht
Copy link
Contributor

@kaspar030 Any feedback from your side to the proposed approach?

@gschorcht
Copy link
Contributor

@maribu It might be interesting for you.

@maribu
Copy link
Member

maribu commented Oct 11, 2019

@maribu It might be interesting for you.

Yes, indeed. Especially the possibility of sharing one thread to handling the ISR callback between different network devices is nice for our testbed nodes with plenty of netdevs.

To me it appeared that the conclusion that irq_handler does already solve the problem the tasklets try to address and that effort should be focused there instead, right?

@jia200x
Copy link
Member Author

jia200x commented Oct 11, 2019

To me it appeared that the conclusion that irq_handler does already solve the problem the tasklets try to address and that effort should be focused there instead, right?

Basically the irq_handler is a tasklets implementation. So, yes.
I proposed to rename it to "tasklets", since it's a common concept and it would be easier to find it in the documentation.

@jia200x
Copy link
Member Author

jia200x commented Oct 15, 2019

#12459 is there

@jia200x
Copy link
Member Author

jia200x commented Feb 7, 2020

since #12474 got merged, I will just close this one

@jia200x jia200x closed this Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants