diff mbox series

[RFC,v2,01/13] Extend health thread lifetime

Message ID 20170918225206.17725-2-jonathan.rajotte-julien@efficios.com
State Superseded, archived
Delegated to: Jérémie Galarneau
Headers show
Series Sessiond teardown overhaul | expand

Commit Message

Jonathan Rajotte Sept. 18, 2017, 10:51 p.m. UTC
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(-)

Comments

Jérémie Galarneau Dec. 14, 2017, 1:54 a.m. UTC | #1
Hi,

I rebased this patch set on master.

https://github.com/jgalar/lttng-tools/tree/teardown-rebased

Most patches still applied cleanly, but git really messed this first one up
when rebasing. The patch applied and the code compiled, but
I realized hunks from thread_manage_health() and thread_manage_clients()
ended up mixed-up... *Fun* times.

See comments below and on the other patches.

Thanks!
Jérémie

On 18 September 2017 at 18:51, Jonathan Rajotte
<jonathan.rajotte-julien at efficios.com> wrote:
> Allow easier control over the thread by providing a dedicated quit pipe.

over the health thread's lifetime

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

Please add a comment about the "2" being used here.

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

teardown socket -> pipe

> +                                       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.

is 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
>
diff mbox series

Patch

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