On 18 September 2017 at 18:52, Jonathan Rajotte <[email protected]> wrote: > Previous work on thread termination ordering permits the dismissal of > the following comment: > > We don't clean the UST app hash table here since already registered > applications can still be controlled so let them be until the session > daemon dies or the applications stop. > > At the moment of termination control thread are already terminated.
It's not clear why this patch is needed. I think what you are worried about here is the application notification sockets never being closed on shutdown? That seems to be taken care of in sessiond_cleanup() [1] which cleans the various global HTs. More specifically, if we look in ust_app_clean_list [2], delete_ust_app_rcu() is invoked on all ust_app_ht entries, which eventually results in delete_ust_app() being called, which closes the application socket [3]. Looking at the rest of your patch set, I'm sure there is something I am missing, but it's not clear. Read on for other comments. [1] https://github.com/lttng/lttng-tools/blob/master/src/bin/lttng-sessiond/main.c#L579 [2] https://github.com/lttng/lttng-tools/blob/master/src/bin/lttng-sessiond/ust-app.c#L3880 [3] https://github.com/lttng/lttng-tools/blob/master/src/bin/lttng-sessiond/ust-app.c#L922 > > Signed-off-by: Jonathan Rajotte <[email protected]> > --- > src/bin/lttng-sessiond/main.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c > index 8cffa6f6..4a2a661f 100644 > --- a/src/bin/lttng-sessiond/main.c > +++ b/src/bin/lttng-sessiond/main.c > @@ -1780,11 +1780,15 @@ error_testpoint: > utils_close_pipe(apps_cmd_pipe); > apps_cmd_pipe[0] = apps_cmd_pipe[1] = -1; > > - /* > - * We don't clean the UST app hash table here since already registered > - * applications can still be controlled so let them be until the > session > - * daemon dies or the applications stop. > - */ > + { > + struct lttng_ht_iter iter; > + struct ust_app *app; Missing blank line here. > + rcu_read_lock(); > + cds_lfht_for_each_entry(ust_app_ht->ht, &iter.iter, app, > pid_n.node) { > + ust_app_unregister(app->sock); This part deserves an explanation in your commit message. Jérémie > + } > + rcu_read_unlock(); > + } > > if (err) { > health_error(); > -- > 2.11.0 > -- Jérémie Galarneau EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list [email protected] https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
