| Message ID | 20210419074814.488933-1-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pctl: add synchronisation to wait for threads init | expand |
On 19.04.21 09:48, Dominique Martinet wrote: > Count when we start threads and add helpers to wait for threads > to have finished their init phase before moving on and starting > subprocesses. > > This helps making sure children processes can immediately send logs > to the parent swupdate process or connect to swupdate control sockets > immediately without waiting and retrying. > > This also fixes a hang with musl libc that occured when another thread > was using stdio functions while fork happens. > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > > Note for traces in network_initializer: v0 of the patch added a > first-iteration bool to only signal thread ready once within the loop > after the print statement, but that is a bit ugly. > There actually already is another trace just before the end of the for > loop (sleep again message) so having the message within the loop brings > no information. > OTOH there was no message that update started so I added one, but I > guess it is not really required and am happy to remove it. > > core/network_thread.c | 2 ++ > core/notifier.c | 1 + > core/pctl.c | 35 +++++++++++++++++++++++++++++++++++ > core/progress_thread.c | 1 + > core/stream_interface.c | 4 +++- > core/swupdate.c | 3 +++ > include/pctl.h | 3 +++ > 7 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/core/network_thread.c b/core/network_thread.c > index dbfdf0800fec..b21d983680be 100644 > --- a/core/network_thread.c > +++ b/core/network_thread.c > @@ -333,6 +333,7 @@ static void handle_subprocess_ipc(struct subprocess_msg_elem *subprocess_msg) > static void *subprocess_thread (void *data) > { > (void)data; > + thread_ready(); > pthread_mutex_lock(&subprocess_msg_lock); > > while(1) { > @@ -394,6 +395,7 @@ void *network_thread (void *data) > get_ctrl_socket()); > } > > + thread_ready(); > do { > clilen = sizeof(cliaddr); > if ( (ctrlconnfd = accept(ctrllisten, (struct sockaddr *) &cliaddr, &clilen)) < 0) { > diff --git a/core/notifier.c b/core/notifier.c > index 5aa5435a2c72..1052dbda0fd8 100644 > --- a/core/notifier.c > +++ b/core/notifier.c > @@ -448,6 +448,7 @@ static void *notifier_thread (void __attribute__ ((__unused__)) *data) > break; > } while (1); > > + thread_ready(); > do { > len = recvfrom(serverfd, &msg, sizeof(msg), 0, NULL, NULL); > /* > diff --git a/core/pctl.c b/core/pctl.c > index 517be2dddc22..9c2d53443c1f 100644 > --- a/core/pctl.c > +++ b/core/pctl.c > @@ -51,6 +51,13 @@ int pid = 0; > > int sw_sockfd = -1; > > +/* > + * This allows waiting for initial threads to be ready before spawning subprocesses > + */ > +static int threads_towait = 0; > +static pthread_mutex_t threads_towait_lock = PTHREAD_MUTEX_INITIALIZER; > +static pthread_cond_t threads_towait_cond = PTHREAD_COND_INITIALIZER; > + > #if defined(__linux__) > static void parent_dead_handler(int __attribute__ ((__unused__)) dummy) > { > @@ -70,6 +77,10 @@ pthread_t start_thread(void *(* start_routine) (void *), void *arg) > pthread_attr_init(&attr); > pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); > > + pthread_mutex_lock(&threads_towait_lock); > + threads_towait++; > + pthread_mutex_unlock(&threads_towait_lock); > + > ret = pthread_create(&id, &attr, start_routine, arg); > if (ret) { > exit(1); > @@ -77,6 +88,30 @@ pthread_t start_thread(void *(* start_routine) (void *), void *arg) > return id; > } > > +/* > + * Internal threads should signal they are ready if internal subprocesses > + * can be spawned after them > + */ > +void thread_ready(void) > +{ > + pthread_mutex_lock(&threads_towait_lock); > + threads_towait--; > + if (threads_towait == 0) > + pthread_cond_broadcast(&threads_towait_cond); > + pthread_mutex_unlock(&threads_towait_lock); > +} > + > +/* > + * Wait for all threads to have signaled they're ready > + */ > +void wait_threads_ready(void) > +{ > + pthread_mutex_lock(&threads_towait_lock); > + while (threads_towait != 0) > + pthread_cond_wait(&threads_towait_cond, &threads_towait_lock); > + pthread_mutex_unlock(&threads_towait_lock); > +} > + > /* > * spawn_process forks and start a new process > * under a new user > diff --git a/core/progress_thread.c b/core/progress_thread.c > index aaca3d52d936..e46a00ed713d 100644 > --- a/core/progress_thread.c > +++ b/core/progress_thread.c > @@ -260,6 +260,7 @@ void *progress_bar_thread (void __attribute__ ((__unused__)) *data) > get_prog_socket()); > } > > + thread_ready(); > do { > clilen = sizeof(cliaddr); > if ( (connfd = accept(listen, (struct sockaddr *) &cliaddr, &clilen)) < 0) { > diff --git a/core/stream_interface.c b/core/stream_interface.c > index 0631abd756ba..e87fc7185964 100644 > --- a/core/stream_interface.c > +++ b/core/stream_interface.c > @@ -509,10 +509,11 @@ void *network_initializer(void *data) > /* fork off the local dialogs and network service */ > network_thread_id = start_thread(network_thread, &inst); > > + TRACE("Main loop daemon"); > + thread_ready(); > /* handle installation requests (from either source) */ > while (1) { > ret = 0; > - TRACE("Main loop Daemon"); > > /* wait for someone to issue an install request */ > pthread_mutex_lock(&stream_mutex); > @@ -520,6 +521,7 @@ void *network_initializer(void *data) > inst.status = RUN; > pthread_mutex_unlock(&stream_mutex); > notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software Update started !"); > + TRACE("Software update started"); > > req = &inst.req; > > diff --git a/core/swupdate.c b/core/swupdate.c > index 2d114650a102..b39cff9a0dcc 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -856,6 +856,9 @@ int main(int argc, char **argv) > > start_thread(progress_bar_thread, NULL); > > + /* wait for threads to be done before starting children */ > + wait_threads_ready(); > + > /* Start embedded web server */ > #if defined(CONFIG_MONGOOSE) > if (opt_w) { > diff --git a/include/pctl.h b/include/pctl.h > index 8767864e165b..82b6e638cccb 100644 > --- a/include/pctl.h > +++ b/include/pctl.h > @@ -27,6 +27,9 @@ struct swupdate_task { > > pthread_t start_thread(void *(* start_routine) (void *), void *arg); > > +void thread_ready(void); > +void wait_threads_ready(void); > + > typedef int (*swupdate_process)(const char *cfgname, int argc, char **argv); > > void start_subprocess(sourcetype type, const char *name, > Applied to -master, thanks ! Best regards, Stefano Babic
diff --git a/core/network_thread.c b/core/network_thread.c index dbfdf0800fec..b21d983680be 100644 --- a/core/network_thread.c +++ b/core/network_thread.c @@ -333,6 +333,7 @@ static void handle_subprocess_ipc(struct subprocess_msg_elem *subprocess_msg) static void *subprocess_thread (void *data) { (void)data; + thread_ready(); pthread_mutex_lock(&subprocess_msg_lock); while(1) { @@ -394,6 +395,7 @@ void *network_thread (void *data) get_ctrl_socket()); } + thread_ready(); do { clilen = sizeof(cliaddr); if ( (ctrlconnfd = accept(ctrllisten, (struct sockaddr *) &cliaddr, &clilen)) < 0) { diff --git a/core/notifier.c b/core/notifier.c index 5aa5435a2c72..1052dbda0fd8 100644 --- a/core/notifier.c +++ b/core/notifier.c @@ -448,6 +448,7 @@ static void *notifier_thread (void __attribute__ ((__unused__)) *data) break; } while (1); + thread_ready(); do { len = recvfrom(serverfd, &msg, sizeof(msg), 0, NULL, NULL); /* diff --git a/core/pctl.c b/core/pctl.c index 517be2dddc22..9c2d53443c1f 100644 --- a/core/pctl.c +++ b/core/pctl.c @@ -51,6 +51,13 @@ int pid = 0; int sw_sockfd = -1; +/* + * This allows waiting for initial threads to be ready before spawning subprocesses + */ +static int threads_towait = 0; +static pthread_mutex_t threads_towait_lock = PTHREAD_MUTEX_INITIALIZER; +static pthread_cond_t threads_towait_cond = PTHREAD_COND_INITIALIZER; + #if defined(__linux__) static void parent_dead_handler(int __attribute__ ((__unused__)) dummy) { @@ -70,6 +77,10 @@ pthread_t start_thread(void *(* start_routine) (void *), void *arg) pthread_attr_init(&attr); pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); + pthread_mutex_lock(&threads_towait_lock); + threads_towait++; + pthread_mutex_unlock(&threads_towait_lock); + ret = pthread_create(&id, &attr, start_routine, arg); if (ret) { exit(1); @@ -77,6 +88,30 @@ pthread_t start_thread(void *(* start_routine) (void *), void *arg) return id; } +/* + * Internal threads should signal they are ready if internal subprocesses + * can be spawned after them + */ +void thread_ready(void) +{ + pthread_mutex_lock(&threads_towait_lock); + threads_towait--; + if (threads_towait == 0) + pthread_cond_broadcast(&threads_towait_cond); + pthread_mutex_unlock(&threads_towait_lock); +} + +/* + * Wait for all threads to have signaled they're ready + */ +void wait_threads_ready(void) +{ + pthread_mutex_lock(&threads_towait_lock); + while (threads_towait != 0) + pthread_cond_wait(&threads_towait_cond, &threads_towait_lock); + pthread_mutex_unlock(&threads_towait_lock); +} + /* * spawn_process forks and start a new process * under a new user diff --git a/core/progress_thread.c b/core/progress_thread.c index aaca3d52d936..e46a00ed713d 100644 --- a/core/progress_thread.c +++ b/core/progress_thread.c @@ -260,6 +260,7 @@ void *progress_bar_thread (void __attribute__ ((__unused__)) *data) get_prog_socket()); } + thread_ready(); do { clilen = sizeof(cliaddr); if ( (connfd = accept(listen, (struct sockaddr *) &cliaddr, &clilen)) < 0) { diff --git a/core/stream_interface.c b/core/stream_interface.c index 0631abd756ba..e87fc7185964 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -509,10 +509,11 @@ void *network_initializer(void *data) /* fork off the local dialogs and network service */ network_thread_id = start_thread(network_thread, &inst); + TRACE("Main loop daemon"); + thread_ready(); /* handle installation requests (from either source) */ while (1) { ret = 0; - TRACE("Main loop Daemon"); /* wait for someone to issue an install request */ pthread_mutex_lock(&stream_mutex); @@ -520,6 +521,7 @@ void *network_initializer(void *data) inst.status = RUN; pthread_mutex_unlock(&stream_mutex); notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software Update started !"); + TRACE("Software update started"); req = &inst.req; diff --git a/core/swupdate.c b/core/swupdate.c index 2d114650a102..b39cff9a0dcc 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -856,6 +856,9 @@ int main(int argc, char **argv) start_thread(progress_bar_thread, NULL); + /* wait for threads to be done before starting children */ + wait_threads_ready(); + /* Start embedded web server */ #if defined(CONFIG_MONGOOSE) if (opt_w) { diff --git a/include/pctl.h b/include/pctl.h index 8767864e165b..82b6e638cccb 100644 --- a/include/pctl.h +++ b/include/pctl.h @@ -27,6 +27,9 @@ struct swupdate_task { pthread_t start_thread(void *(* start_routine) (void *), void *arg); +void thread_ready(void); +void wait_threads_ready(void); + typedef int (*swupdate_process)(const char *cfgname, int argc, char **argv); void start_subprocess(sourcetype type, const char *name,
Count when we start threads and add helpers to wait for threads to have finished their init phase before moving on and starting subprocesses. This helps making sure children processes can immediately send logs to the parent swupdate process or connect to swupdate control sockets immediately without waiting and retrying. This also fixes a hang with musl libc that occured when another thread was using stdio functions while fork happens. Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- Note for traces in network_initializer: v0 of the patch added a first-iteration bool to only signal thread ready once within the loop after the print statement, but that is a bit ugly. There actually already is another trace just before the end of the for loop (sleep again message) so having the message within the loop brings no information. OTOH there was no message that update started so I added one, but I guess it is not really required and am happy to remove it. core/network_thread.c | 2 ++ core/notifier.c | 1 + core/pctl.c | 35 +++++++++++++++++++++++++++++++++++ core/progress_thread.c | 1 + core/stream_interface.c | 4 +++- core/swupdate.c | 3 +++ include/pctl.h | 3 +++ 7 files changed, 48 insertions(+), 1 deletion(-)