[lttng-dev] [RFC PATCH v2 09/13] Perform ust_app_unregister on thread_manage_apps teardown

[Jérémie Galarneau]

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
>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com