| Message ID | 1520479170-31989-1-git-send-email-alison@peloton-tech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ipc: protect against pthread_cond_wait() spurious wakeups | expand |
Hi Alison, On 08/03/2018 04:19, alison@peloton-tech.com wrote: > From: Alison Chaiken <alison@peloton-tech.com> > > pthread_cond_wait() is subject to spurious wakeups. Make certain that > the network_initializer() thread is genuinely ready to run by checking > that inst.fd has been set by network_thread(). Otherwise > network_initializer() has no work to perform. > Does this really happened or it just to prevent issues ? Spurious wakeups (as far as I know) can happen when several threads are involved. For design, SWUpdate has just two threads, the network thread and the consumer (the installer). > Suggested-by: Brian Silverman <brian@peloton-tech.com> > Signed-off-by: Alison Chaiken <alison@peloton-tech.com> > --- > corelib/stream_interface.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c > index 91f523f..9e31b19 100644 > --- a/corelib/stream_interface.c > +++ b/corelib/stream_interface.c > @@ -301,7 +301,11 @@ void *network_initializer(void *data) > > /* wait for someone to issue an install request */ > pthread_mutex_lock(&stream_mutex); > - pthread_cond_wait(&stream_wkup, &stream_mutex); > + /* Guard against spurious wakeups of pthread_cond_wait() by checking that the > + installer thread has a file descriptor to read. */ > + while (inst.fd == -1) { > + pthread_cond_wait(&stream_wkup, &stream_mutex); > + } I do not like this. It realizes a sort of endless loop, without any information if something is going wrong. IMHO nothing "dangerous" should happen in case of spurious wakeup. The fd is invalid and the installer returns soon when it tries to read the stream. The side-effect should be just that an error is reported, while we had really nothing to install. But hardware remains untouched and nothing is written or damaged. If we want to add such a protection, it should flow into the main loop, something like: pthread_cond_wait(&stream_wkup, &stream_mutex); if (inst.fd == -1) { pthread_mutex_unlock(&stream_mutex); INFO(<tell that happened !>); continue; } Best regards, Stefano Babic
diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c index 91f523f..9e31b19 100644 --- a/corelib/stream_interface.c +++ b/corelib/stream_interface.c @@ -301,7 +301,11 @@ void *network_initializer(void *data) /* wait for someone to issue an install request */ pthread_mutex_lock(&stream_mutex); - pthread_cond_wait(&stream_wkup, &stream_mutex); + /* Guard against spurious wakeups of pthread_cond_wait() by checking that the + installer thread has a file descriptor to read. */ + while (inst.fd == -1) { + pthread_cond_wait(&stream_wkup, &stream_mutex); + } inst.status = RUN; pthread_mutex_unlock(&stream_mutex); notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software Update started !");