[lttng-dev] [RFC PATCH v2 00/13] Sessiond teardown overhaul

[lttng-dev] [RFC PATCH v2 00/13] Sessiond teardown overhaul

[Jonathan Rajotte]

Hi,

This series objective is to review how the sessiond teardown is conducted. This is
the result of stress testing the teardown code path using [1] and the discussion
following previous proposed fixes [2][3].

The main problematic is that applications could hang on
application-initiated communication via the notify socket. This would end up
causing multiple deadlock scenarios where sessiond would wait on communications
for an already stuck application. This is caused by the absence of certain
threads at the moment the application is communicating and data handling race
during the teardown. I propose to impose a teardown order for thread to prevent
such issue.

The teardown mechanisms were revolving around the use of the
thread_quit_pipe. Most thread would listen on this pipe for the go signal of
termination. This result is a non-deterministic teardown code path and result in
scenarios quite hard to reproduce.

This series mostly introduce specialized quit pipes offering better control
over the lifetime of important/interdependent(data-wise) threads.

It also includes fixes to problems found along the way.

[1] https://github.com/PSRCode/lttng-stress
[2] https://lists.lttng.org/pipermail/lttng-dev/2017-August/027366.html
[3] https://lists.lttng.org/pipermail/lttng-dev/2017-August/027365.html

[lttng-dev] [RFC PATCH v2 01/13] Extend health thread lifetime

[Jonathan Rajotte]

Allow easier control over the thread by providing a dedicated quit pipe.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 74 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 19 deletions(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 03f695ec..5d7df744 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -204,6 +204,7 @@ static int kernel_poll_pipe[2] = { -1, -1 };
  * for all threads when receiving an event on the pipe.
  */
 static int thread_quit_pipe[2] = { -1, -1 };
+static int thread_health_teardown_trigger_pipe[2] = { -1, -1 };
 
 /*
  * This pipe is used to inform the thread managing application communication
@@ -477,6 +478,11 @@ static int init_thread_quit_pipe(void)
 	return __init_thread_quit_pipe(thread_quit_pipe);
 }
 
+static int init_thread_health_teardown_trigger_pipe(void)
+{
+	return __init_thread_quit_pipe(thread_health_teardown_trigger_pipe);
+}
+
 /*
  * Stop all threads by closing the thread quit pipe.
  */
@@ -4297,11 +4303,13 @@ static void *thread_manage_health(void *data)
 		goto error;
 	}
 
-	/*
-	 * Pass 2 as size here for the thread quit pipe and client_sock. Nothing
-	 * more will be added to this poll set.
-	 */
-	ret = sessiond_set_thread_pollset(&events, 2);
+	ret = lttng_poll_create(&events, 2, LTTNG_CLOEXEC);
+	if (ret < 0) {
+		goto error;
+	}
+
+	/* Add teardown trigger */
+	ret = lttng_poll_add(&events, thread_health_teardown_trigger_pipe[0], LPOLLIN | LPOLLERR);
 	if (ret < 0) {
 		goto error;
 	}
@@ -4343,10 +4351,18 @@ restart:
 			}
 
 			/* Thread quit pipe has been closed. Killing thread. */
-			ret = sessiond_check_thread_quit_pipe(pollfd, revents);
-			if (ret) {
-				err = 0;
-				goto exit;
+			if (pollfd == thread_health_teardown_trigger_pipe[0]) {
+				if (revents & LPOLLIN) {
+					/* Normal exit */
+					err = 0;
+					goto exit;
+				} else if (revents & LPOLLERR) {
+					ERR("Health teardown poll error");
+					goto error;
+				} else {
+					ERR("Unexpected poll events %u for teardown sock", revents);
+					goto error;
+				}
 			}
 
 			/* Event on the registration socket */
@@ -4412,8 +4428,13 @@ restart:
 		}
 	}
 
-exit:
 error:
+	/*
+	 * Perform stop_thread only on error path since in a normal teardown the
+	 * health thread is in the last thread to terminate.
+	 */
+	stop_threads();
+exit:
 	if (err) {
 		ERR("Health error occurred in %s", __func__);
 	}
@@ -4425,9 +4446,7 @@ error:
 			PERROR("close");
 		}
 	}
-
 	lttng_poll_clean(&events);
-	stop_threads();
 	rcu_unregister_thread();
 	return NULL;
 }
@@ -5631,6 +5650,7 @@ int main(int argc, char **argv)
 			*ust64_channel_monitor_pipe = NULL,
 			*kernel_channel_monitor_pipe = NULL;
 	bool notification_thread_running = false;
+	bool health_thread_running = false;
 
 	init_kernel_workarounds();
 
@@ -5717,6 +5737,11 @@ int main(int argc, char **argv)
 		goto exit_init_data;
 	}
 
+	if (init_thread_health_teardown_trigger_pipe()) {
+		retval = -1;
+		goto exit_init_data;
+	}
+
 	/* Check if daemon is UID = 0 */
 	is_root = !getuid();
 
@@ -6119,6 +6144,7 @@ int main(int argc, char **argv)
 		retval = -1;
 		goto exit_health;
 	}
+	health_thread_running = true;
 
 	/* notification_thread_data acquires the pipes' read side. */
 	notification_thread_handle = notification_thread_handle_create(
@@ -6311,13 +6337,6 @@ exit_dispatch:
 
 exit_client:
 exit_notification:
-	ret = pthread_join(health_thread, &status);
-	if (ret) {
-		errno = ret;
-		PERROR("pthread_join health thread");
-		retval = -1;
-	}
-
 exit_health:
 exit_init_data:
 	/*
@@ -6359,6 +6378,20 @@ exit_init_data:
 		notification_thread_handle_destroy(notification_thread_handle);
 	}
 
+	if (health_thread_running) {
+		ret = notify_thread_pipe(thread_health_teardown_trigger_pipe[1]);
+		if (ret < 0) {
+			ERR("write error on thread quit pipe");
+		}
+
+		ret = pthread_join(health_thread, &status);
+		if (ret) {
+			errno = ret;
+			PERROR("pthread_join health thread");
+			retval = -1;
+		}
+	}
+
 	rcu_thread_offline();
 	rcu_unregister_thread();
 
@@ -6366,6 +6399,9 @@ exit_init_data:
 	if (ret) {
 		retval = -1;
 	}
+
+	/* Health thread teardown pipe cleanup */
+	utils_close_pipe(thread_health_teardown_trigger_pipe);
 	lttng_pipe_destroy(ust32_channel_monitor_pipe);
 	lttng_pipe_destroy(ust64_channel_monitor_pipe);
 	lttng_pipe_destroy(kernel_channel_monitor_pipe);
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 02/13] Reorder pthread_join for easier ordering later on

[Jonathan Rajotte]

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 96 ++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 51 deletions(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 5d7df744..45c0270e 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -6170,15 +6170,26 @@ int main(int argc, char **argv)
 	}
 	notification_thread_running = true;
 
-	/* Create thread to manage the client socket */
-	ret = pthread_create(&client_thread, default_pthread_attr(),
-			thread_manage_clients, (void *) NULL);
+	/* Create thread to manage application notify socket */
+	ret = pthread_create(&apps_notify_thread, default_pthread_attr(),
+			ust_thread_manage_notify, (void *) NULL);
 	if (ret) {
 		errno = ret;
-		PERROR("pthread_create clients");
+		PERROR("pthread_create notify");
 		retval = -1;
 		stop_threads();
-		goto exit_client;
+		goto exit_apps_notify;
+	}
+
+	/* Create thread to manage application socket */
+	ret = pthread_create(&apps_thread, default_pthread_attr(),
+			thread_manage_apps, (void *) NULL);
+	if (ret) {
+		errno = ret;
+		PERROR("pthread_create apps");
+		retval = -1;
+		stop_threads();
+		goto exit_apps;
 	}
 
 	/* Create thread to dispatch registration */
@@ -6203,26 +6214,15 @@ int main(int argc, char **argv)
 		goto exit_reg_apps;
 	}
 
-	/* Create thread to manage application socket */
-	ret = pthread_create(&apps_thread, default_pthread_attr(),
-			thread_manage_apps, (void *) NULL);
+	/* Create thread to manage the client socket */
+	ret = pthread_create(&client_thread, default_pthread_attr(),
+			thread_manage_clients, (void *) NULL);
 	if (ret) {
 		errno = ret;
-		PERROR("pthread_create apps");
+		PERROR("pthread_create clients");
 		retval = -1;
 		stop_threads();
-		goto exit_apps;
-	}
-
-	/* Create thread to manage application notify socket */
-	ret = pthread_create(&apps_notify_thread, default_pthread_attr(),
-			ust_thread_manage_notify, (void *) NULL);
-	if (ret) {
-		errno = ret;
-		PERROR("pthread_create notify");
-		retval = -1;
-		stop_threads();
-		goto exit_apps_notify;
+		goto exit_client;
 	}
 
 	/* Create agent registration thread. */
@@ -6278,12 +6278,12 @@ exit_load_session:
 		ret = pthread_join(kernel_thread, &status);
 		if (ret) {
 			errno = ret;
-			PERROR("pthread_join");
+			PERROR("pthread_join kernel");
 			retval = -1;
 		}
 	}
-exit_kernel:
 
+exit_kernel:
 	ret = pthread_join(agent_reg_thread, &status);
 	if (ret) {
 		errno = ret;
@@ -6291,51 +6291,45 @@ exit_kernel:
 		retval = -1;
 	}
 exit_agent_reg:
-
-	ret = pthread_join(apps_notify_thread, &status);
+	ret = pthread_join(client_thread, &status);
 	if (ret) {
 		errno = ret;
-		PERROR("pthread_join apps notify");
+		PERROR("pthread_join client");
 		retval = -1;
 	}
-exit_apps_notify:
 
+exit_client:
+	ret = pthread_join(reg_apps_thread, &status);
+	if (ret) {
+		errno = ret;
+		PERROR("pthread_join registration app" );
+		retval = -1;
+	}
+exit_reg_apps:
+	ret = pthread_join(dispatch_thread, &status);
+	if (ret) {
+		errno = ret;
+		PERROR("pthread_join dispatch");
+		retval = -1;
+	}
+
+exit_dispatch:
 	ret = pthread_join(apps_thread, &status);
 	if (ret) {
 		errno = ret;
 		PERROR("pthread_join apps");
 		retval = -1;
 	}
+
 exit_apps:
-
-	ret = pthread_join(reg_apps_thread, &status);
+	ret = pthread_join(apps_notify_thread, &status);
 	if (ret) {
 		errno = ret;
-		PERROR("pthread_join");
-		retval = -1;
-	}
-exit_reg_apps:
-
-	/*
-	 * Join dispatch thread after joining reg_apps_thread to ensure
-	 * we don't leak applications in the queue.
-	 */
-	ret = pthread_join(dispatch_thread, &status);
-	if (ret) {
-		errno = ret;
-		PERROR("pthread_join");
-		retval = -1;
-	}
-exit_dispatch:
-
-	ret = pthread_join(client_thread, &status);
-	if (ret) {
-		errno = ret;
-		PERROR("pthread_join");
+		PERROR("pthread_join apps notify");
 		retval = -1;
 	}
 
-exit_client:
+exit_apps_notify:
 exit_notification:
 exit_health:
 exit_init_data:
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 03/13] Terminate dispatch thread after reg_apps_thread is terminated

[Jonathan Rajotte]

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 45c0270e..0ca72ec8 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -497,9 +497,6 @@ static void stop_threads(void)
 		ERR("write error on thread quit pipe");
 	}
 
-	/* Dispatch thread */
-	CMM_STORE_SHARED(dispatch_thread_exit, 1);
-	futex_nto1_wake(&ust_cmd_queue.futex);
 }
 
 /*
@@ -6306,6 +6303,10 @@ exit_client:
 		retval = -1;
 	}
 exit_reg_apps:
+	/* Instruct the dispatch thread to quit */
+	CMM_STORE_SHARED(dispatch_thread_exit, 1);
+	futex_nto1_wake(&ust_cmd_queue.futex);
+
 	ret = pthread_join(dispatch_thread, &status);
 	if (ret) {
 		errno = ret;
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 04/13] Order termination of thread_manage_apps after dispatch_thread

[Jonathan Rajotte]

This ensure that no registration are ongoing when thread_manage_apps
perform it's cleanup and termination.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 46 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 0ca72ec8..8d9b14b5 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -205,6 +205,7 @@ static int kernel_poll_pipe[2] = { -1, -1 };
  */
 static int thread_quit_pipe[2] = { -1, -1 };
 static int thread_health_teardown_trigger_pipe[2] = { -1, -1 };
+static int thread_apps_teardown_trigger_pipe[2] = { -1, -1 };
 
 /*
  * This pipe is used to inform the thread managing application communication
@@ -483,6 +484,11 @@ static int init_thread_health_teardown_trigger_pipe(void)
 	return __init_thread_quit_pipe(thread_health_teardown_trigger_pipe);
 }
 
+static int init_thread_apps_teardown_trigger_pipe(void)
+{
+	return __init_thread_quit_pipe(thread_apps_teardown_trigger_pipe);
+}
+
 /*
  * Stop all threads by closing the thread quit pipe.
  */
@@ -496,7 +502,6 @@ static void stop_threads(void)
 	if (ret < 0) {
 		ERR("write error on thread quit pipe");
 	}
-
 }
 
 /*
@@ -1634,11 +1639,17 @@ static void *thread_manage_apps(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;
 	}
 
+	/* Add quit pipe */
+	ret = lttng_poll_add(&events, thread_apps_teardown_trigger_pipe[0], LPOLLIN | LPOLLERR);
+	if (ret < 0) {
+		goto error;
+	}
+
 	ret = lttng_poll_add(&events, apps_cmd_pipe[0], LPOLLIN | LPOLLRDHUP);
 	if (ret < 0) {
 		goto error;
@@ -1685,10 +1696,18 @@ static void *thread_manage_apps(void *data)
 			}
 
 			/* Thread quit pipe has been closed. Killing thread. */
-			ret = sessiond_check_thread_quit_pipe(pollfd, revents);
-			if (ret) {
-				err = 0;
-				goto exit;
+			if (pollfd == thread_apps_teardown_trigger_pipe[0]) {
+				if (revents & LPOLLIN) {
+					/* Normal exit */
+					err = 0;
+					goto exit;
+				} else if (revents & LPOLLERR) {
+					ERR("Quit pipe error");
+					goto error;
+				} else {
+					ERR("Unknown poll events %u for quit pipe", revents);
+					goto error;
+				}
 			}
 
 			/* Inspect the apps cmd pipe */
@@ -5739,6 +5758,11 @@ int main(int argc, char **argv)
 		goto exit_init_data;
 	}
 
+	if (init_thread_apps_teardown_trigger_pipe()) {
+		retval = -1;
+		goto exit_init_data;
+	}
+
 	/* Check if daemon is UID = 0 */
 	is_root = !getuid();
 
@@ -6302,6 +6326,7 @@ exit_client:
 		PERROR("pthread_join registration app" );
 		retval = -1;
 	}
+
 exit_reg_apps:
 	/* Instruct the dispatch thread to quit */
 	CMM_STORE_SHARED(dispatch_thread_exit, 1);
@@ -6315,6 +6340,12 @@ exit_reg_apps:
 	}
 
 exit_dispatch:
+	/* Instruct the apps thread to quit */
+	ret = notify_thread_pipe(thread_apps_teardown_trigger_pipe[1]);
+	if (ret < 0) {
+		ERR("write error on thread quit pipe");
+	}
+
 	ret = pthread_join(apps_thread, &status);
 	if (ret) {
 		errno = ret;
@@ -6397,6 +6428,9 @@ exit_init_data:
 
 	/* Health thread teardown pipe cleanup */
 	utils_close_pipe(thread_health_teardown_trigger_pipe);
+	/* Apps thread teardown pipe cleanup */
+	utils_close_pipe(thread_apps_teardown_trigger_pipe);
+
 	lttng_pipe_destroy(ust32_channel_monitor_pipe);
 	lttng_pipe_destroy(ust64_channel_monitor_pipe);
 	lttng_pipe_destroy(kernel_channel_monitor_pipe);
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 05/13] Control thread_apps_notify lifetime with specialized quit pipe

[Jonathan Rajotte]

Listen to application longer and quit only when most other thread are
terterminated. This simplifies data interaction with other threads.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/lttng-sessiond.h |  2 ++
 src/bin/lttng-sessiond/main.c           | 20 +++++++++++++++++++-
 src/bin/lttng-sessiond/ust-thread.c     | 27 ++++++++++++++++++++-------
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/src/bin/lttng-sessiond/lttng-sessiond.h b/src/bin/lttng-sessiond/lttng-sessiond.h
index 74552db6..8fbcac60 100644
--- a/src/bin/lttng-sessiond/lttng-sessiond.h
+++ b/src/bin/lttng-sessiond/lttng-sessiond.h
@@ -94,6 +94,8 @@ struct ust_reg_wait_node {
  */
 extern int apps_cmd_notify_pipe[2];
 
+extern int thread_apps_notify_teardown_trigger_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
diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 8d9b14b5..3596d7e3 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -206,6 +206,7 @@ static int kernel_poll_pipe[2] = { -1, -1 };
 static int thread_quit_pipe[2] = { -1, -1 };
 static int thread_health_teardown_trigger_pipe[2] = { -1, -1 };
 static int thread_apps_teardown_trigger_pipe[2] = { -1, -1 };
+int thread_apps_notify_teardown_trigger_pipe[2] = { -1, -1 };
 
 /*
  * This pipe is used to inform the thread managing application communication
@@ -489,6 +490,11 @@ static int init_thread_apps_teardown_trigger_pipe(void)
 	return __init_thread_quit_pipe(thread_apps_teardown_trigger_pipe);
 }
 
+static int init_thread_apps_notify_teardown_trigger_pipe(void)
+{
+	return __init_thread_quit_pipe(thread_apps_notify_teardown_trigger_pipe);
+}
+
 /*
  * Stop all threads by closing the thread quit pipe.
  */
@@ -5763,6 +5769,11 @@ int main(int argc, char **argv)
 		goto exit_init_data;
 	}
 
+	if (init_thread_apps_notify_teardown_trigger_pipe()) {
+		retval = -1;
+		goto exit_init_data;
+	}
+
 	/* Check if daemon is UID = 0 */
 	is_root = !getuid();
 
@@ -6354,6 +6365,12 @@ exit_dispatch:
 	}
 
 exit_apps:
+	/* Instruct the apps_notify thread to quit */
+	ret = notify_thread_pipe(thread_apps_notify_teardown_trigger_pipe[1]);
+	if (ret < 0) {
+		ERR("write error on thread quit pipe");
+	}
+
 	ret = pthread_join(apps_notify_thread, &status);
 	if (ret) {
 		errno = ret;
@@ -6430,7 +6447,8 @@ exit_init_data:
 	utils_close_pipe(thread_health_teardown_trigger_pipe);
 	/* Apps thread teardown pipe cleanup */
 	utils_close_pipe(thread_apps_teardown_trigger_pipe);
-
+	/* Apps notify thread teardown pipe cleanup */
+	utils_close_pipe(thread_apps_notify_teardown_trigger_pipe);
 	lttng_pipe_destroy(ust32_channel_monitor_pipe);
 	lttng_pipe_destroy(ust64_channel_monitor_pipe);
 	lttng_pipe_destroy(kernel_channel_monitor_pipe);
diff --git a/src/bin/lttng-sessiond/ust-thread.c b/src/bin/lttng-sessiond/ust-thread.c
index 7fb18a78..1e7a8229 100644
--- a/src/bin/lttng-sessiond/ust-thread.c
+++ b/src/bin/lttng-sessiond/ust-thread.c
@@ -51,9 +51,15 @@ 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;
+		goto error;
+	}
+
+	/* Add quit pipe */
+	ret = lttng_poll_add(&events, thread_apps_notify_teardown_trigger_pipe[0], LPOLLIN | LPOLLERR);
+	if (ret < 0) {
+		goto error;
 	}
 
 	/* Add notify pipe to the pollset. */
@@ -99,11 +105,18 @@ restart:
 				continue;
 			}
 
-			/* Thread quit pipe has been closed. Killing thread. */
-			ret = sessiond_check_thread_quit_pipe(pollfd, revents);
-			if (ret) {
-				err = 0;
-				goto exit;
+			if (pollfd == thread_apps_notify_teardown_trigger_pipe[0]) {
+				if (revents & LPOLLIN) {
+					/* Normal exit */
+					err = 0;
+					goto exit;
+				} else if (revents & LPOLLERR) {
+					ERR("Apps notify quit error");
+					goto error;
+				} else {
+					ERR("Unexpected poll events %u for quit pipe", revents);
+					goto error;
+				}
 			}
 
 			/* Inspect the apps cmd pipe */
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 06/13] Fix: unregister app notify socket on sessiond tear down

[Jonathan Rajotte]

A race between the sessiond tear down and applications initialization
can lead to a deadlock.

Applications try to communicate via the notify sockets while sessiond
does not listen anymore on these sockets since the thread responsible
for reception/response is terminated (ust_thread_manage_notify). These
sockets are never closed hence an application could hang on
communication.

Sessiond hang happen during call to cmd_destroy_session during
sessiond_cleanup. Sessiond is trying to communicate with the app while
the app is waiting for a response on the app notification socket.

To prevent this situation a call to ust_app_notify_sock_unregister is
performed on all entry of the ust_app_ht_by_notify_sock hash table at
the time of termination. This ensure that any pending communication
initiated by the application will be terminated since all sockets will
be closed at the end of the grace period via call_rcu inside
ust_app_notify_sock_unregister. The use of ust_app_ht_by_notify_sock
instead of the ust_app_ht prevent a double call_rcu since entries are
removed from ust_app_ht_by_notify_sock during ust_app_notify_sock_unregister.

This can be reproduced using the sessiond_teardown_active_session
scenario provided by [1].

[1] https://github.com/PSRCode/lttng-stress

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/ust-thread.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/bin/lttng-sessiond/ust-thread.c b/src/bin/lttng-sessiond/ust-thread.c
index 1e7a8229..8f11133a 100644
--- a/src/bin/lttng-sessiond/ust-thread.c
+++ b/src/bin/lttng-sessiond/ust-thread.c
@@ -27,6 +27,19 @@
 #include "health-sessiond.h"
 #include "testpoint.h"
 
+
+static
+void notify_sock_unregister_all()
+{
+	struct lttng_ht_iter iter;
+	struct ust_app *app;
+	rcu_read_lock();
+	cds_lfht_for_each_entry(ust_app_ht_by_notify_sock->ht, &iter.iter, app, notify_sock_n.node) {
+		ust_app_notify_sock_unregister(app->notify_sock);
+	}
+	rcu_read_unlock();
+}
+
 /*
  * This thread manage application notify communication.
  */
@@ -53,7 +66,7 @@ void *ust_thread_manage_notify(void *data)
 
 	ret = lttng_poll_create(&events, 2, LTTNG_CLOEXEC);
 	if (ret < 0) {
-		goto error;
+		goto error_poll_create;
 	}
 
 	/* Add quit pipe */
@@ -197,6 +210,8 @@ error_poll_create:
 error_testpoint:
 	utils_close_pipe(apps_cmd_notify_pipe);
 	apps_cmd_notify_pipe[0] = apps_cmd_notify_pipe[1] = -1;
+	notify_sock_unregister_all();
+
 	DBG("Application notify communication apps thread cleanup complete");
 	if (err) {
 		health_error();
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 07/13] Always reply to an inquiring app

[Jonathan Rajotte]

Reply to the app on errors to prevent an app-side receive hang.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/ust-app.c | 64 ++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c
index e79455b0..20ac469b 100644
--- a/src/bin/lttng-sessiond/ust-app.c
+++ b/src/bin/lttng-sessiond/ust-app.c
@@ -5394,13 +5394,13 @@ static int reply_ust_register_channel(int sock, int sobjd, int cobjd,
 		size_t nr_fields, struct ustctl_field *fields)
 {
 	int ret, ret_code = 0;
-	uint32_t chan_id, reg_count;
-	uint64_t chan_reg_key;
-	enum ustctl_channel_header type;
+	uint32_t chan_id = 0, reg_count = 0;
+	uint64_t chan_reg_key = 0;
+	enum ustctl_channel_header type = USTCTL_CHANNEL_HEADER_UNKNOWN;
 	struct ust_app *app;
 	struct ust_app_channel *ua_chan;
 	struct ust_app_session *ua_sess;
-	struct ust_registry_session *registry;
+	struct ust_registry_session *registry = NULL;
 	struct ust_registry_channel *chan_reg;
 
 	rcu_read_lock();
@@ -5410,16 +5410,16 @@ static int reply_ust_register_channel(int sock, int sobjd, int cobjd,
 	if (!app) {
 		DBG("Application socket %d is being torn down. Abort event notify",
 				sock);
-		ret = 0;
-		goto error_rcu_unlock;
+		ret_code = -1;
+		goto reply;
 	}
 
 	/* Lookup channel by UST object descriptor. */
 	ua_chan = find_channel_by_objd(app, cobjd);
 	if (!ua_chan) {
 		DBG("Application channel is being torn down. Abort event notify");
-		ret = 0;
-		goto error_rcu_unlock;
+		ret_code = -1;
+		goto reply;
 	}
 
 	assert(ua_chan->session);
@@ -5429,8 +5429,8 @@ static int reply_ust_register_channel(int sock, int sobjd, int cobjd,
 	registry = get_session_registry(ua_sess);
 	if (!registry) {
 		DBG("Application session is being torn down. Abort event notify");
-		ret = 0;
-		goto error_rcu_unlock;
+		ret_code = -1;
+		goto reply;
 	};
 
 	/* Depending on the buffer type, a different channel key is used. */
@@ -5469,10 +5469,13 @@ static int reply_ust_register_channel(int sock, int sobjd, int cobjd,
 		ret_code = ust_metadata_channel_statedump(registry, chan_reg);
 		if (ret_code) {
 			ERR("Error appending channel metadata (errno = %d)", ret_code);
-			goto reply;
+			goto unlock_reply;
 		}
 	}
 
+unlock_reply:
+	pthread_mutex_unlock(&registry->lock);
+
 reply:
 	DBG3("UST app replying to register channel key %" PRIu64
 			" with id %u, type: %d, ret: %d", chan_reg_key, chan_id, type,
@@ -5488,12 +5491,13 @@ reply:
 		goto error;
 	}
 
-	/* This channel registry registration is completed. */
+	if (ret_code < 0) {
+		goto error;
+	}
+
 	chan_reg->register_done = 1;
 
 error:
-	pthread_mutex_unlock(&registry->lock);
-error_rcu_unlock:
 	rcu_read_unlock();
 	free(fields);
 	return ret;
@@ -5527,16 +5531,16 @@ static int add_event_ust_registry(int sock, int sobjd, int cobjd, char *name,
 	if (!app) {
 		DBG("Application socket %d is being torn down. Abort event notify",
 				sock);
-		ret = 0;
-		goto error_rcu_unlock;
+		ret_code = -1;
+		goto reply;
 	}
 
 	/* Lookup channel by UST object descriptor. */
 	ua_chan = find_channel_by_objd(app, cobjd);
 	if (!ua_chan) {
 		DBG("Application channel is being torn down. Abort event notify");
-		ret = 0;
-		goto error_rcu_unlock;
+		ret_code = -1;
+		goto reply;
 	}
 
 	assert(ua_chan->session);
@@ -5545,8 +5549,8 @@ static int add_event_ust_registry(int sock, int sobjd, int cobjd, char *name,
 	registry = get_session_registry(ua_sess);
 	if (!registry) {
 		DBG("Application session is being torn down. Abort event notify");
-		ret = 0;
-		goto error_rcu_unlock;
+		ret_code = -1;
+		goto reply;
 	}
 
 	if (ua_sess->buffer_type == LTTNG_BUFFER_PER_UID) {
@@ -5570,6 +5574,9 @@ static int add_event_ust_registry(int sock, int sobjd, int cobjd, char *name,
 	fields = NULL;
 	model_emf_uri = NULL;
 
+	pthread_mutex_unlock(&registry->lock);
+
+reply:
 	/*
 	 * The return value is returned to ustctl so in case of an error, the
 	 * application can be notified. In case of an error, it's important not to
@@ -5593,8 +5600,6 @@ static int add_event_ust_registry(int sock, int sobjd, int cobjd, char *name,
 			name, event_id);
 
 error:
-	pthread_mutex_unlock(&registry->lock);
-error_rcu_unlock:
 	rcu_read_unlock();
 	free(sig);
 	free(fields);
@@ -5627,8 +5632,9 @@ static int add_enum_ust_registry(int sock, int sobjd, char *name,
 		/* Return an error since this is not an error */
 		DBG("Application socket %d is being torn down. Aborting enum registration",
 				sock);
+		ret_code = -1;
 		free(entries);
-		goto error_rcu_unlock;
+		goto reply;
 	}
 
 	/* Lookup session by UST object descriptor. */
@@ -5637,14 +5643,16 @@ static int add_enum_ust_registry(int sock, int sobjd, char *name,
 		/* Return an error since this is not an error */
 		DBG("Application session is being torn down (session not found). Aborting enum registration.");
 		free(entries);
-		goto error_rcu_unlock;
+		ret_code = -1;
+		goto reply;
 	}
 
 	registry = get_session_registry(ua_sess);
 	if (!registry) {
 		DBG("Application session is being torn down (registry not found). Aborting enum registration.");
 		free(entries);
-		goto error_rcu_unlock;
+		ret_code = -1;
+		goto reply;
 	}
 
 	pthread_mutex_lock(&registry->lock);
@@ -5658,6 +5666,9 @@ static int add_enum_ust_registry(int sock, int sobjd, char *name,
 			entries, nr_entries, &enum_id);
 	entries = NULL;
 
+	pthread_mutex_unlock(&registry->lock);
+
+reply:
 	/*
 	 * The return value is returned to ustctl so in case of an error, the
 	 * application can be notified. In case of an error, it's important not to
@@ -5678,10 +5689,7 @@ static int add_enum_ust_registry(int sock, int sobjd, char *name,
 	}
 
 	DBG3("UST registry enum %s added successfully or already found", name);
-
 error:
-	pthread_mutex_unlock(&registry->lock);
-error_rcu_unlock:
 	rcu_read_unlock();
 	return ret;
 }
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 08/13] Fix: perform lookup then test for condition

[Jonathan Rajotte]

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 3596d7e3..8cffa6f6 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -1899,8 +1899,11 @@ static void sanitize_wait_queue(struct ust_reg_wait_queue *wait_queue)
 
 		cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
 				&wait_queue->head, head) {
-			if (pollfd == wait_node->app->sock &&
-					(revents & (LPOLLHUP | LPOLLERR))) {
+			if (pollfd != wait_node->app->sock) {
+				continue;
+			}
+
+			if (revents & (LPOLLHUP | LPOLLERR)) {
 				cds_list_del(&wait_node->head);
 				wait_queue->count--;
 				ust_app_destroy(wait_node->app);
-- 
2.11.0

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

[Jonathan Rajotte]

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(-)

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;
+		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();
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 10/13] Teardown apps_notify_thread before thread_manage_apps

[Jonathan Rajotte]

RFC:
This is necessary since we use the ust_app_ht_by_notify_sock to
perform the cleanup operation. Since both ust_app_unregister and
ust_app_ust_app_notify_unregister perform a remove of the app on the
ust_ust_app_by_notify_sock but only ust_app_notify_unregister actually
end up performing a close(call_rcu) on the socket.

Other way to do fix this problem?

Could we simply not remove it on a ust_app_unregister? And always defer
to the apps_notify_thread for cleanup?

Update the value in the hash table to -1 and emit a close and remove
from the hash table if the value is -1?

We could also keep a local list of fd in apps_notify_thread and use it for
cleanup instead of relying on ust_ust_app_by_notify_sock.

I'm not sure what is the best/elegant solution here. I am not a fan of
the current solution but it working.

Obviously this commit will be reworded and modified accordingly.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 55 ++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 4a2a661f..216d0da6 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -6209,16 +6209,6 @@ int main(int argc, char **argv)
 	}
 	notification_thread_running = true;
 
-	/* Create thread to manage application notify socket */
-	ret = pthread_create(&apps_notify_thread, default_pthread_attr(),
-			ust_thread_manage_notify, (void *) NULL);
-	if (ret) {
-		errno = ret;
-		PERROR("pthread_create notify");
-		retval = -1;
-		stop_threads();
-		goto exit_apps_notify;
-	}
 
 	/* Create thread to manage application socket */
 	ret = pthread_create(&apps_thread, default_pthread_attr(),
@@ -6231,6 +6221,17 @@ int main(int argc, char **argv)
 		goto exit_apps;
 	}
 
+	/* Create thread to manage application notify socket */
+	ret = pthread_create(&apps_notify_thread, default_pthread_attr(),
+			ust_thread_manage_notify, (void *) NULL);
+	if (ret) {
+		errno = ret;
+		PERROR("pthread_create notify");
+		retval = -1;
+		stop_threads();
+		goto exit_apps_notify;
+	}
+
 	/* Create thread to dispatch registration */
 	ret = pthread_create(&dispatch_thread, default_pthread_attr(),
 			thread_dispatch_ust_registration, (void *) NULL);
@@ -6358,20 +6359,6 @@ exit_reg_apps:
 	}
 
 exit_dispatch:
-	/* Instruct the apps thread to quit */
-	ret = notify_thread_pipe(thread_apps_teardown_trigger_pipe[1]);
-	if (ret < 0) {
-		ERR("write error on thread quit pipe");
-	}
-
-	ret = pthread_join(apps_thread, &status);
-	if (ret) {
-		errno = ret;
-		PERROR("pthread_join apps");
-		retval = -1;
-	}
-
-exit_apps:
 	/* Instruct the apps_notify thread to quit */
 	ret = notify_thread_pipe(thread_apps_notify_teardown_trigger_pipe[1]);
 	if (ret < 0) {
@@ -6386,6 +6373,26 @@ exit_apps:
 	}
 
 exit_apps_notify:
+	/*
+	 * The barrier ensure that all previous resources, notify sockets in
+	 * particular, are freed/closed.
+	 */
+	rcu_barrier();
+
+	/* Instruct the apps thread to quit */
+	ret = notify_thread_pipe(thread_apps_teardown_trigger_pipe[1]);
+	if (ret < 0) {
+		ERR("write error on thread quit pipe");
+	}
+
+	ret = pthread_join(apps_thread, &status);
+	if (ret) {
+		errno = ret;
+		PERROR("pthread_join apps");
+		retval = -1;
+	}
+
+exit_apps:
 exit_notification:
 exit_health:
 exit_init_data:
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 11/13] Comments update

[Jonathan Rajotte]

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 216d0da6..a840e8de 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -496,14 +496,18 @@ static int init_thread_apps_notify_teardown_trigger_pipe(void)
 }
 
 /*
- * Stop all threads by closing the thread quit pipe.
+ * Stop first wave threads by closing the thread quit pipe.
+ *  - kernel thread
+ *  - client thread
+ *  - agent thread
+ *  - reg apps thread
  */
 static void stop_threads(void)
 {
 	int ret;
 
-	/* Stopping all threads */
-	DBG("Terminating all threads");
+	/* Stopping first wave threads */
+	DBG("Terminating first wave threads");
 	ret = notify_thread_pipe(thread_quit_pipe[1]);
 	if (ret < 0) {
 		ERR("write error on thread quit pipe");
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 12/13] Fix: delay termination on consumerd to allow metadata flushing

[Jonathan Rajotte]

Move consumerd ownership to thread_manage_consumer to scope the lifetime
on the consumerd to its manager thread.

"thread_manage_consumer" is responsible for signaling and waiting the
termination of its consumerd.

All thread_manage_consumer threads now wait on a unique quit pipe
different from the global thread quit pipe. This allow control over its
lifetime.

The termination notification is sent during sessiond_cleanup after the
destroy session command to ensure that no session are still active at
the moment the consumerds are terminated.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 174 +++++++++++++++++++++++++++---------------
 1 file changed, 112 insertions(+), 62 deletions(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index a840e8de..fb58ab4b 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -206,6 +206,7 @@ static int kernel_poll_pipe[2] = { -1, -1 };
 static int thread_quit_pipe[2] = { -1, -1 };
 static int thread_health_teardown_trigger_pipe[2] = { -1, -1 };
 static int thread_apps_teardown_trigger_pipe[2] = { -1, -1 };
+static int thread_consumers_teardown_trigger_pipe[2] = { -1, -1 };
 int thread_apps_notify_teardown_trigger_pipe[2] = { -1, -1 };
 
 /*
@@ -495,6 +496,11 @@ static int init_thread_apps_notify_teardown_trigger_pipe(void)
 	return __init_thread_quit_pipe(thread_apps_notify_teardown_trigger_pipe);
 }
 
+static int init_thread_consumers_teardown_trigger_pipe(void)
+{
+	return __init_thread_quit_pipe(thread_consumers_teardown_trigger_pipe);
+}
+
 /*
  * Stop first wave threads by closing the thread quit pipe.
  *  - kernel thread
@@ -601,14 +607,15 @@ static int generate_lock_file_path(char *path, size_t len)
 /*
  * Wait on consumer process termination.
  *
- * Need to be called with the consumer data lock held or from a context
- * ensuring no concurrent access to data (e.g: cleanup).
+ * Need to be called with the consumer data lock held.
  */
 static void wait_consumer(struct consumer_data *consumer_data)
 {
 	pid_t ret;
 	int status;
 
+	assert(consumer_data);
+
 	if (consumer_data->pid <= 0) {
 		return;
 	}
@@ -626,6 +633,52 @@ static void wait_consumer(struct consumer_data *consumer_data)
 }
 
 /*
+ * Signal to the consumer process to terminate.
+ *
+ * Need to be called with the consumer data lock held.
+ */
+static void kill_consumer(struct consumer_data *consumer_data)
+{
+	int ret;
+
+	assert(consumer_data);
+
+	/* Consumer pid must be a real one. */
+	if (consumer_data->pid <= 0) {
+		goto end;
+	}
+
+	ret = kill(consumer_data->pid, SIGTERM);
+	if (ret) {
+		PERROR("Error killing consumer daemon");
+		goto end;
+	}
+end:
+	return;
+}
+
+static int join_thread_consumer(struct consumer_data *consumer_data)
+{
+	int ret;
+	void *status;
+
+	assert(consumer_data);
+
+	/* Consumer pid must be a real one. */
+	if (consumer_data->pid <= 0) {
+		ret = 0;
+		goto end;
+	}
+
+	ret = pthread_join(consumer_data->thread, &status);
+	if (ret) {
+		ERR("Joining consumer thread pid %d", consumer_data->pid);
+	}
+end:
+	return ret;
+}
+
+/*
  * Cleanup the session daemon's data structures.
  */
 static void sessiond_cleanup(void)
@@ -707,7 +760,6 @@ static void sessiond_cleanup(void)
 	(void) rmdir(path);
 
 	DBG("Cleaning up all sessions");
-
 	/* Destroy session list mutex */
 	if (session_list_ptr != NULL) {
 		pthread_mutex_destroy(&session_list_ptr->lock);
@@ -719,9 +771,35 @@ static void sessiond_cleanup(void)
 		}
 	}
 
-	wait_consumer(&kconsumer_data);
-	wait_consumer(&ustconsumer64_data);
-	wait_consumer(&ustconsumer32_data);
+	/*
+	 * Delay the termination of manage_consumer_thread threads to allow
+	 * proper metadata flushing, following the session destroy. Use a
+	 * barrier to ensure that all call_rcu are executed at this point.
+	 */
+	DBG("Teardown consurmer thread");
+	rcu_barrier();
+	ret = notify_thread_pipe(thread_consumers_teardown_trigger_pipe[1]);
+	if (ret < 0) {
+		ERR("write error on thread consumer quit pipe");
+	}
+
+	ret = join_thread_consumer(&kconsumer_data);
+	if (ret) {
+		errno = ret;
+		PERROR("join_consumer kernel");
+	}
+
+	ret = join_thread_consumer(&ustconsumer32_data);
+	if (ret) {
+		errno = ret;
+		PERROR("join_consumer ust32");
+	}
+
+	ret = join_thread_consumer(&ustconsumer64_data);
+	if (ret) {
+		errno = ret;
+		PERROR("join_consumer ust64");
+	}
 
 	DBG("Cleaning up all agent apps");
 	agent_app_ht_clean();
@@ -1289,14 +1367,20 @@ static void *thread_manage_consumer(void *data)
 	health_code_update();
 
 	/*
-	 * Pass 3 as size here for the thread quit pipe, consumerd_err_sock and the
+	 * Pass 3 as size here for the thread consumer quit pipe, consumerd_err_sock and the
 	 * metadata_sock. Nothing more will be added to this poll set.
 	 */
-	ret = sessiond_set_thread_pollset(&events, 3);
+	ret = lttng_poll_create(&events, 3, LTTNG_CLOEXEC);
 	if (ret < 0) {
 		goto error_poll;
 	}
 
+	/* Add quit pipe */
+	ret = lttng_poll_add(&events, thread_consumers_teardown_trigger_pipe[0], LPOLLIN | LPOLLERR);
+	if (ret < 0) {
+		goto error;
+	}
+
 	/*
 	 * The error socket here is already in a listening state which was done
 	 * just before spawning this thread to avoid a race between the consumer
@@ -1344,7 +1428,7 @@ restart:
 		}
 
 		/* Thread quit pipe has been closed. Killing thread. */
-		ret = sessiond_check_thread_quit_pipe(pollfd, revents);
+		ret = (pollfd == thread_consumers_teardown_trigger_pipe[0] && (revents & LPOLLIN)) ? 1 : 0;
 		if (ret) {
 			err = 0;
 			goto exit;
@@ -1509,7 +1593,7 @@ restart_poll:
 			 * but continue the current loop to handle potential data from
 			 * consumer.
 			 */
-			should_quit = sessiond_check_thread_quit_pipe(pollfd, revents);
+			should_quit = (pollfd == thread_consumers_teardown_trigger_pipe[0] && (revents & LPOLLIN)) ? 1 : 0;
 
 			if (pollfd == sock) {
 				/* Event on the consumerd socket */
@@ -1552,11 +1636,6 @@ restart_poll:
 
 exit:
 error:
-	/*
-	 * We lock here because we are about to close the sockets and some other
-	 * thread might be using them so get exclusive access which will abort all
-	 * other consumer command by other threads.
-	 */
 	pthread_mutex_lock(&consumer_data->lock);
 
 	/* Immediately set the consumerd state to stopped */
@@ -1570,6 +1649,13 @@ error:
 		assert(0);
 	}
 
+	/*
+	 * This thread is responsible for its consumerd. Make sure the
+	 * consumerd teardown is complete before proceding.
+	 */
+	kill_consumer(consumer_data);
+	wait_consumer(consumer_data);
+
 	if (consumer_data->err_sock >= 0) {
 		ret = close(consumer_data->err_sock);
 		if (ret) {
@@ -1600,13 +1686,15 @@ error:
 
 	unlink(consumer_data->err_unix_sock_path);
 	unlink(consumer_data->cmd_unix_sock_path);
-	pthread_mutex_unlock(&consumer_data->lock);
 
 	/* Cleanup metadata socket mutex. */
 	if (consumer_data->metadata_sock.lock) {
 		pthread_mutex_destroy(consumer_data->metadata_sock.lock);
 		free(consumer_data->metadata_sock.lock);
 	}
+
+	pthread_mutex_unlock(&consumer_data->lock);
+
 	lttng_poll_clean(&events);
 
 	if (cmd_socket_wrapper) {
@@ -2560,27 +2648,6 @@ error:
 }
 
 /*
- * Join consumer thread
- */
-static int join_consumer_thread(struct consumer_data *consumer_data)
-{
-	void *status;
-
-	/* Consumer pid must be a real one. */
-	if (consumer_data->pid > 0) {
-		int ret;
-		ret = kill(consumer_data->pid, SIGTERM);
-		if (ret) {
-			PERROR("Error killing consumer daemon");
-			return ret;
-		}
-		return pthread_join(consumer_data->thread, &status);
-	} else {
-		return 0;
-	}
-}
-
-/*
  * Fork and exec a consumer daemon (consumerd).
  *
  * Return pid if successful else -1.
@@ -4741,27 +4808,6 @@ error_create_poll:
 
 	rcu_unregister_thread();
 
-	/*
-	 * Since we are creating the consumer threads, we own them, so we need
-	 * to join them before our thread exits.
-	 */
-	ret = join_consumer_thread(&kconsumer_data);
-	if (ret) {
-		errno = ret;
-		PERROR("join_consumer");
-	}
-
-	ret = join_consumer_thread(&ustconsumer32_data);
-	if (ret) {
-		errno = ret;
-		PERROR("join_consumer ust32");
-	}
-
-	ret = join_consumer_thread(&ustconsumer64_data);
-	if (ret) {
-		errno = ret;
-		PERROR("join_consumer ust64");
-	}
 	return NULL;
 }
 
@@ -5785,6 +5831,11 @@ int main(int argc, char **argv)
 		goto exit_init_data;
 	}
 
+	if (init_thread_consumers_teardown_trigger_pipe()) {
+		retval = -1;
+		goto exit_init_data;
+	}
+
 	/* Check if daemon is UID = 0 */
 	is_root = !getuid();
 
@@ -6406,11 +6457,8 @@ exit_init_data:
 	 * perform lookups in those structures.
 	 */
 	rcu_barrier();
-	/*
-	 * sessiond_cleanup() is called when no other thread is running, except
-	 * the ht_cleanup thread, which is needed to destroy the hash tables.
-	 */
 	rcu_thread_online();
+
 	sessiond_cleanup();
 
 	/*
@@ -6461,6 +6509,8 @@ exit_init_data:
 		retval = -1;
 	}
 
+	/* Consumers thread teardown pipe cleanup */
+	utils_close_pipe(thread_consumers_teardown_trigger_pipe);
 	/* Health thread teardown pipe cleanup */
 	utils_close_pipe(thread_health_teardown_trigger_pipe);
 	/* Apps thread teardown pipe cleanup */
-- 
2.11.0

[lttng-dev] [RFC PATCH v2 13/13] Fix: quit early if instructed to

[Jonathan Rajotte]

If the wait_queue size is considerable, not checking if the thread should
quit delays the termination (3+ seconds during stress testing).

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index fb58ab4b..0475c5a3 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -2244,7 +2244,7 @@ static void *thread_dispatch_ust_registration(void *data)
 				rcu_read_unlock();
 				session_unlock_list();
 			}
-		} while (node != NULL);
+		} while (node != NULL && !CMM_LOAD_SHARED(dispatch_thread_exit));
 
 		health_poll_entry();
 		/* Futex wait on queue. Blocking call on futex() */
-- 
2.11.0