-
Notifications
You must be signed in to change notification settings - Fork 24
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
Runtime support for watchdogs #177
Changes from 30 commits
d28b56d
37a9604
11dc26d
96da894
90a0a9b
050abfa
0820da9
92160a8
04173e9
7fd67a5
6dff526
0f28759
59dfc4f
9a40303
b4b70c5
c974d4d
3358cd1
4677a37
38aec13
c643234
b97d084
c00691a
8a8827f
50ef0d2
742f294
faaf56b
4fe27cc
f5825b4
82afe9b
ff3c84c
8ca094f
10c275b
e516564
b62683e
24c0dc7
e24e3bc
ccb0a3c
9b6c7dc
7dcdb7d
9ba9817
2c7a514
05b6e96
795d38b
24e1dc0
cee63e1
a48b022
02cbf70
2f51b30
436decc
c985c6a
775b27a
eede0fa
aeff26c
670f634
9ae8632
5427b5d
4265119
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1316,18 +1316,70 @@ bool _lf_check_deadline(self_base_t* self, bool invoke_deadline_handler) { | |
return false; | ||
} | ||
|
||
#ifdef LF_THREADED | ||
|
||
void* run_watchdog(watchdog_t* watchdog) { | ||
watchdog->thread_active = true; | ||
self_base_t* base = watchdog->base; | ||
|
||
while (lf_time_physical() < watchdog->expiration) { | ||
interval_t T = watchdog->expiration - lf_time_physical(); | ||
lf_mutex_unlock(&(base->watchdog_mutex)); | ||
lf_nanosleep(T); | ||
lf_mutex_lock(&(base->watchdog_mutex)); | ||
} | ||
|
||
|
||
watchdog_function_t watchdog_func = watchdog->watchdog_function; | ||
(*watchdog_func)(watchdog); | ||
lf_mutex_unlock(&(base->watchdog_mutex)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you return here, the thread_active field is still true. So there wont be another thread created. Also, shouldnt the thread keep looping inside this function and wait for a watchdog reset? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This has been changed so that the thread_active field is set to false.
Calling lf_watchdog_start should restart the watchdog and create another thread after the previous one has ended. If we want to have a watchdog that keeps running without a restart after being executed, I can put the function inside the loop and change the conditions so that it continues looping, but that was not my understanding of the intended behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am pretty sure we wouldnt want that. Creating a thread has a significant overhead. It would be better to have a single thread per watchdog. |
||
} | ||
|
||
void _lf_watchdog_start(watchdog_t* watchdog, interval_t additional_timeout) { | ||
self_base_t* base = watchdog->base; | ||
|
||
watchdog->expiration = lf_time_logical() + watchdog->min_expiration + additional_timeout; | ||
|
||
|
||
if (!watchdog->thread_active) { | ||
lf_thread_create(&(watchdog->thread_id), run_watchdog, watchdog); | ||
} | ||
|
||
watchdog->thread_active = false; | ||
erlingrj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
//FIXME: modif4watchdogs | ||
void _lf_watchdog_stop(watchdog_t* watchdog) { | ||
|
||
} | ||
#endif | ||
|
||
/** | ||
* Invoke the given reaction | ||
* | ||
* @param reaction The reaction that has just executed. | ||
* @param worker The thread number of the worker thread or 0 for unthreaded execution (for tracing). | ||
*/ | ||
void _lf_invoke_reaction(reaction_t* reaction, int worker) { | ||
|
||
#ifdef LF_THREADED | ||
if (&(((self_base_t*) reaction->self)->watchdog_mutex) != NULL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesnt seem right. I think you rather want to test the value of that field, not its address. The address of the watchdog_mutex field is some offset + the address of the reaction->self struct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that was actually causing an issue once I fixed the race condition associated with 'thread_active' field race condition mentioned below. I now use a boolean field added to the self_base_t struct indicating whether or not the mutex has been initialized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think have a |
||
lf_mutex_lock(&(((self_base_t*) reaction->self)->watchdog_mutex)); | ||
} | ||
#endif | ||
|
||
tracepoint_reaction_starts(reaction, worker); | ||
((self_base_t*) reaction->self)->executing_reaction = reaction; | ||
reaction->function(reaction->self); | ||
((self_base_t*) reaction->self)->executing_reaction = NULL; | ||
tracepoint_reaction_ends(reaction, worker); | ||
|
||
|
||
#ifdef LF_THREADED | ||
if (&(((self_base_t*) reaction->self)->watchdog_mutex) != NULL) { | ||
lf_mutex_unlock(&(((self_base_t*) reaction->self)->watchdog_mutex)); | ||
} | ||
#endif | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
master | ||
watchdogs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesnt seem right. The first iteration of this while loop you are not holding the mutex and can thus not unlock it. Please document how the interaction between this thread running a single watchdog and the worker threads executing reactions should look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, should be fixed now.
Not sure if there is a better way of drawing this out:
_lf_invoke_reaction
locks the mutex if the reaction is part of a reactor that contains a watchdog_lf_invoke_reaction
unlocks the mutex after invoking the reaction_lf_watchdog_start
unlocks the mutex only after creating the thread for the watchdogrun_watchdog
locks the mutex that was just unlocked by_lf_watchdog_start
run_watchdog
unlocks->locks the mutex inside the while loop so that other reactions in the reactor can run while the thread is asleep.run_watchdog
unlocks the mutex after executing the watchdog functionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have it represented as a sequence. In what order to you expect this to happen. I think there is some flaw in your logic here. The watchdog mutex should be held whenever a reaction within that reactor is executed. I don't think it makes sense to unlock that mutex when you call
_lf_watchdog_start
from inside a reaction. Remember that a mutex has to unlocked by the same thread that locks it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. My thought process was the following:
Thread 1: If reaction is in a reactor that contains a watchdog, lock mutex -
lf_invoke_reaction
.Thread 1: Execute reaction.
Thread 1: If reaction body calls
_lf_watchdog_start
, start thread 2.Thread 1: Inside
_lf_watchdog_start
, unlock mutex.Thread 2: acquire mutex, set expiration, and unlock mutex
Thread 2: sleep
Thread 1: Inside
_lf_watchdog_start
- if the mutex has been unlocked, lock mutexThread 1: Unlock mutex inside
lf_invoke_reaction
.Thread 2: Once done sleeping, lock mutex, and either reset expiration or execute watchdog function.
In hindsight, I'm now realizing that I need to reset the sleep each time, or I will get undesired behavior past the first watchdog function call.