[RFC,v2,09/13] Perform ust_app_unregister on thread_manage_apps teardown
Commit Message
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.
Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
src/bin/lttng-sessiond/main.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
Comments
On 18 September 2017 at 18:52, Jonathan Rajotte
<jonathan.rajotte-julien at efficios.com> 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 <jonathan.rajotte-julien at efficios.com>
> ---
> 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
>
@@ -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;
+ rcu_read_lock();
+ cds_lfht_for_each_entry(ust_app_ht->ht, &iter.iter, app, pid_n.node) {
+ ust_app_unregister(app->sock);
+ }
+ rcu_read_unlock();
+ }
if (err) {
health_error();