Fix problem with threaded Zephyr and overflow handling in QEMU emulation #159
Conversation
| @@ -367,7 +374,8 @@ int lf_available_cores() { | |||
| int lf_thread_create(lf_thread_t* thread, void *(*lf_thread) (void *), void* arguments) { | |||
| // Use static id to map each created thread to a | |||
| static int tid = 0; | |||
|
|
|||
|
|
|||
| // Make sure we dont try to create too many threads | |||
There was a problem hiding this comment.
If lf_thread_create is called in user code, then the number of workers isn't really relevant, is it? Does this really belong here?
There was a problem hiding this comment.
I think it is bad practice to do too much dynamic spawning of threads in a microcontroller setting. You want to avoid dynamic memory allocation if you can. Therefore the stacks and thread control blocks are statically allocated. This puts an upper limit to how many times you can call lf_thread_t. The user can use ordinary calls to zephyrs API to create other threads.
There was a problem hiding this comment.
While I don't disagree with you on this, AFAIK, the Zephyr implementation is the only one that does this. My point of criticism has to do with the fact that the function is lf_thread_create not lf_worker_create. Threads and workers are two different things, and you're now looking at the number of workers in the thread creation function, which I think is odd...
There was a problem hiding this comment.
Hm, I would argue that reactor-c is not a threading library and that user-code should not use the LF threading API. That would make the runtime more attractive for use in these embedded systems where people are wary of using dynamic memory allocation. There is already a lot of that in the runtime though, but theoretically, most of it can be replaced by static allocations.
There was a problem hiding this comment.
The function does not have a leading underscore, so it is part of the public API...
There was a problem hiding this comment.
Zephyr has a POSIX implementation that we could use temporarily, or we can just turn of threaded support for Zephyr, that is fine with me. With the bodiless reactions we have to make a decision about what is actually going to be part of the user-facing API since those includes must be code-generated. So let's make the right decision there and revisit this later
There was a problem hiding this comment.
I'm not sure I understand your proposal. Here are some possible solutions that crossed my mind:
- have a define
MAX_THREAD_COUNTand check against that - update the docs and have all implementations check against
NUMBER_OF_WORKERS - rename to
_lf_thread_create - introduce a
lf_worker_createthat does the check and then just invokeslf_thread_create - ...?
| @@ -1031,7 +1031,9 @@ void start_threads() { | |||
| LF_PRINT_LOG("Starting %u worker threads.", _lf_number_of_workers); | |||
| _lf_thread_ids = (lf_thread_t*)malloc(_lf_number_of_workers * sizeof(lf_thread_t)); | |||
| for (unsigned int i = 0; i < _lf_number_of_workers; i++) { | |||
| lf_thread_create(&_lf_thread_ids[i], worker, NULL); | |||
| if (lf_thread_create(&_lf_thread_ids[i], worker, NULL) != 0) { | |||
There was a problem hiding this comment.
It seems that the check implemented in lf_thread_create should be done here, at the call site, before even invoking lf_thread_create...
There was a problem hiding this comment.
Since I have statically allocated the stacks for the threads I have to check it inside lf_thread_create to make sure that it is not called from somewhere else
erlingrj
changed the title
This PR addresses various bugs that I discovered when debugging the QEMU emulation of the threaded runtime.
lf_thread_create. Fixed by turning off threading support until we know whetherlf_thread_createshould be a user-facing API.#inlude "lf_types.h"at the top.lf_clockgettimeimplementetion for the hi-res timer.lf_clock_gettimefor the low-res timer (which is what is used by QEMU), did not handle overflows which caused the emulation to stop after some