[lttng-dev] [RFC PATCH 2/2] Fix: order termination of ust_thread_manage_notify
Jonathan Rajotte-Julien
jonathan.rajotte-julien at efficios.com
Mon Aug 28 18:25:49 UTC 2017
On Fri, Aug 25, 2017 at 07:08:09PM +0000, Mathieu Desnoyers wrote:
> ----- On Aug 24, 2017, at 1:15 PM, Jonathan Rajotte jonathan.rajotte-julien at efficios.com wrote:
>
> > The following scenario lead to a deadlock:
> > - An application is being registered sessiond side
> > (thread_dispatch_ust_registration).
> > - ust_thread_manage_notify is already terminated via quit pipe, hence
> > not listening on the notify socket.
> > - The application is trying to register an event
> > (lttng_event_create->ustcomm_register_event) via the notify socket.
> > - The notify socket is never closed.
> >
> > The application hang on the communication via the notify socket since
> > no one is responding, sessiond is waiting for a reply on the
> > ustctl_register_done call.
> >
> > Forcing an order of termination on ust_thread_manage_notify and
> > thread_dispatch_ust_registration guarantees that the socket will be
> > closed and allow for regular execution. This guarantee is provided by
> > the fact that no further registration can happen in the absence of the
> > registration dispatch thread and all current applications will be present
> > when the ust_thread_manage_notify perform its cleanup.
> >
> > This can be reproduced using the sessiond_teardown_active_session
> > scenario provided by [1].
>
> I think this fix is probably incomplete. There are other scenarios where the
> sessiond can be initiating a command to an application that can cause
> this kind of deadlock scenario as well, e.g. enable-event command being
> handled by process client message.
These scenarios are correctly handled by the previous patch of this series given
that this patch is bundled with it.
These deadlocks are created because the actor (sessiond)
getting teared down does not closes its end of the communication channel on where
it's acting as a responding actor.
This patch try to address the fact that we accept an application even when
ust_thread_manage_notify is terminated and effectively closed all socket known
at this point in time. This results in a dangling open socket causing the
application to hang on notify socket communication which is basically what the
previous patch addresses. I was hesitant to merge those two patches since they
resolve different concerns even if the end problem is similar.
> > This fix should not focus on the specific case of interdependency
> between register done command vs notification thread, but rather
> fix all incorrect teardown dependency order between threads involved
> with UST applications.
Ordering of termination is necessary between those threads to ensure that no
application will be further registered, *notify socket accepted but never
closed*, and that the notify socket hash table is complete when we iterate over
it (previous patch) during termination of the notify thread.
"register done" happens to be the command illustrating the problem.
Also found another deadlock involving call_rcu that would prevent the closing of
the notify sock given that we do it inside a call_rcu call.
- Sessiond is trying to communicate (ust_app_register_done) holding the
session_lock_list.
- A call_rcu to delete_ust_app is waiting on the session_lock_list
- A call_rcu to close the notify socket, causing the hang, was queued.
- The app is waiting on a response on the notify sock which will never be
closed.
Holding a lock during communication is a hole other topics.
This bring me to propose that we order the termination of the
thread_dispatch_ust_registration followed by the termination of
ust_thread_manage_notify combined with a direct call to close() on the notify
sockets during the termination of the ust_thread_manage_notify.
- The ordering ensure that no further addition will be made to the hash tables.
- Allows to bypass the initial reason why the close is done via call_rcu [1].
- The direct close() ensure that the notify socket will effectively be closed.
Resulting in application-side progression (error handling related to the notify
socket closing) and allowing sessiond-side progression.
Thought?
Thanks
[1] src/bin/lttng-sessiond/ust-app.c +5884
---------
/*
* Close socket after a grace period to avoid for the socket to be reused
* before the application object is freed creating potential race between
* threads trying to add unique in the global hash table.
*/
if (!err_enomem) {
call_rcu(&obj->head, close_notify_sock_rcu);
}
>
> Thanks,
>
> Mathieu
>
> >
> > [1] https://github.com/PSRCode/lttng-stress
> >
> > Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
> > ---
> > src/bin/lttng-sessiond/lttng-sessiond.h | 7 +++++++
> > src/bin/lttng-sessiond/main.c | 24 +++++++++++++++++++++++-
> > src/bin/lttng-sessiond/ust-thread.c | 16 ++++++++++++----
> > 3 files changed, 42 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/bin/lttng-sessiond/lttng-sessiond.h
> > b/src/bin/lttng-sessiond/lttng-sessiond.h
> > index 74552db6..a805704e 100644
> > --- a/src/bin/lttng-sessiond/lttng-sessiond.h
> > +++ b/src/bin/lttng-sessiond/lttng-sessiond.h
> > @@ -95,6 +95,13 @@ struct ust_reg_wait_node {
> > extern int apps_cmd_notify_pipe[2];
> >
> > /*
> > + * This pipe is used to inform the ust_thread_manage_notify thread that the
> > + * thread_dispatch_ust_registration thread is terminated. Allowing the
> > + * ust_thread_manage_notify to perform its termination.
> > + */
> > +extern int thread_dispatch_ust_registration_teardown_complete_pipe[2];
> > +
> > +/*
> > * Used to notify that a hash table needs to be destroyed by dedicated
> > * thread. Required by design because we don't want to move destroy
> > * paths outside of large RCU read-side lock paths, and destroy cannot
> > diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
> > index 03f695ec..5298b1a1 100644
> > --- a/src/bin/lttng-sessiond/main.c
> > +++ b/src/bin/lttng-sessiond/main.c
> > @@ -206,6 +206,13 @@ static int kernel_poll_pipe[2] = { -1, -1 };
> > static int thread_quit_pipe[2] = { -1, -1 };
> >
> > /*
> > + * This pipe is used to inform the ust_thread_manage_notify thread that the
> > + * thread_dispatch_ust_registration thread is terminated. Allowing the
> > + * ust_thread_manage_notify to perform its termination.
> > + */
> > +int thread_dispatch_ust_registration_teardown_complete_pipe[2] = { -1, -1 };
> > +
> > +/*
> > * This pipe is used to inform the thread managing application communication
> > * that a command is queued and ready to be processed.
> > */
> > @@ -477,6 +484,11 @@ static int init_thread_quit_pipe(void)
> > return __init_thread_quit_pipe(thread_quit_pipe);
> > }
> >
> > +static int init_thread_dispatch_ust_registration_teardown_complete_pipe(void)
> > +{
> > + return
> > __init_thread_quit_pipe(thread_dispatch_ust_registration_teardown_complete_pipe);
> > +}
> > +
> > /*
> > * Stop all threads by closing the thread quit pipe.
> > */
> > @@ -623,6 +635,7 @@ static void sessiond_cleanup(void)
> > * since we are now called.
> > */
> > utils_close_pipe(thread_quit_pipe);
> > + utils_close_pipe(thread_dispatch_ust_registration_teardown_complete_pipe);
> >
> > /*
> > * If opt_pidfile is undefined, the default file will be wiped when
> > @@ -2128,6 +2141,11 @@ static void *thread_dispatch_ust_registration(void *data)
> > err = 0;
> >
> > error:
> > + ret =
> > notify_thread_pipe(thread_dispatch_ust_registration_teardown_complete_pipe[1]);
> > + if (ret < 0) {
> > + ERR("write error on thread
> > thread_dispatch_ust_registration_teardown_complete_pipe");
> > + }
> > +
> > /* Clean up wait queue. */
> > cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
> > &wait_queue.head, head) {
> > @@ -5711,12 +5729,16 @@ int main(int argc, char **argv)
> > goto exit_ht_cleanup;
> > }
> >
> > - /* Create thread quit pipe */
> > if (init_thread_quit_pipe()) {
> > retval = -1;
> > goto exit_init_data;
> > }
> >
> > + if (init_thread_dispatch_ust_registration_teardown_complete_pipe()) {
> > + retval = -1;
> > + goto exit_init_data;
> > + }
> > +
> > /* Check if daemon is UID = 0 */
> > is_root = !getuid();
> >
> > diff --git a/src/bin/lttng-sessiond/ust-thread.c
> > b/src/bin/lttng-sessiond/ust-thread.c
> > index e605703a..c8d5ffdf 100644
> > --- a/src/bin/lttng-sessiond/ust-thread.c
> > +++ b/src/bin/lttng-sessiond/ust-thread.c
> > @@ -27,6 +27,11 @@
> > #include "health-sessiond.h"
> > #include "testpoint.h"
> >
> > +static
> > +int thread_should_quit(int pollfd, uint32_t revents)
> > +{
> > + return (pollfd == thread_dispatch_ust_registration_teardown_complete_pipe[0]
> > && (revents & LPOLLIN)) ? 1 : 0;
> > +}
> >
> > static
> > void notify_sock_unregister_all()
> > @@ -64,11 +69,16 @@ void *ust_thread_manage_notify(void *data)
> >
> > health_code_update();
> >
> > - ret = sessiond_set_thread_pollset(&events, 2);
> > + ret = lttng_poll_create(&events, 2, LTTNG_CLOEXEC);
> > if (ret < 0) {
> > goto error_poll_create;
> > }
> >
> > + ret = lttng_poll_add(&events,
> > thread_dispatch_ust_registration_teardown_complete_pipe[0], LPOLLIN |
> > LPOLLERR);
> > + if (ret < 0) {
> > + goto error;
> > + }
> > +
> > /* Add notify pipe to the pollset. */
> > ret = lttng_poll_add(&events, apps_cmd_notify_pipe[0],
> > LPOLLIN | LPOLLERR | LPOLLHUP | LPOLLRDHUP);
> > @@ -112,9 +122,7 @@ restart:
> > continue;
> > }
> >
> > - /* Thread quit pipe has been closed. Killing thread. */
> > - ret = sessiond_check_thread_quit_pipe(pollfd, revents);
> > - if (ret) {
> > + if (thread_should_quit(pollfd, revents)) {
> > err = 0;
> > goto exit;
> > }
> > --
> > 2.11.0
> >
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev at lists.lttng.org
> > https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
--
Jonathan Rajotte-Julien
EfficiOS
More information about the lttng-dev
mailing list