[lttng-ust,v2] Fix: wait for initial statedump before proceeding to the main program
diff mbox series

Message ID 20190729220535.26748-1-gabriel.pollo-guilbert@efficios.com
State New
Headers show
Series
  • [lttng-ust,v2] Fix: wait for initial statedump before proceeding to the main program
Related show

Commit Message

Gabriel-Andrew Pollo-Guilbert July 29, 2019, 10:05 p.m. UTC
In the case of short lived applications, the application may exit before
the initial statedump has completed.

Higher-level trace analysis features such as translating addresses to
symbols rely on statedump. That information is required for those
analyses to work on such short-lived applications.

Force the statedump to occur before handing the control to the
application.

Fixes #1190

Signed-off-by: Gabriel-Andrew Pollo-Guilbert <gabriel.pollo-guilbert at efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
---
v2:
	* renamed constructor_sem_posted -> registration_done
	* add sem_count_initial_value
	* add assert in decrement_sem_count()
	* edit documentation for handle_pending_statedump
	* reset registration_done and initial_statedump_done in cleanup_sock_info

 liblttng-ust/lttng-ust-comm.c | 88 ++++++++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 21 deletions(-)

Comments

Mathieu Desnoyers July 30, 2019, 2:13 p.m. UTC | #1
Merged into master, 2.11, 2.10, 2.9, thanks!

Mathieu

----- On Jul 29, 2019, at 6:05 PM, Gabriel-Andrew Pollo-Guilbert gabriel.pollo-guilbert at efficios.com wrote:

> In the case of short lived applications, the application may exit before
> the initial statedump has completed.
> 
> Higher-level trace analysis features such as translating addresses to
> symbols rely on statedump. That information is required for those
> analyses to work on such short-lived applications.
> 
> Force the statedump to occur before handing the control to the
> application.
> 
> Fixes #1190
> 
> Signed-off-by: Gabriel-Andrew Pollo-Guilbert
> <gabriel.pollo-guilbert at efficios.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> ---
> v2:
>	* renamed constructor_sem_posted -> registration_done
>	* add sem_count_initial_value
>	* add assert in decrement_sem_count()
>	* edit documentation for handle_pending_statedump
>	* reset registration_done and initial_statedump_done in cleanup_sock_info
> 
> liblttng-ust/lttng-ust-comm.c | 88 ++++++++++++++++++++++++++---------
> 1 file changed, 67 insertions(+), 21 deletions(-)
> 
> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
> index a299ba84..0c6db9fe 100644
> --- a/liblttng-ust/lttng-ust-comm.c
> +++ b/liblttng-ust/lttng-ust-comm.c
> @@ -228,7 +228,11 @@ static sem_t constructor_wait;
> /*
>  * Doing this for both the global and local sessiond.
>  */
> -static int sem_count = { 2 };
> +enum {
> +	sem_count_initial_value = 4,
> +};
> +
> +static int sem_count = sem_count_initial_value;
> 
> /*
>  * Counting nesting within lttng-ust. Used to ensure that calling fork()
> @@ -243,7 +247,7 @@ struct sock_info {
> 	const char *name;
> 	pthread_t ust_listener;	/* listener thread */
> 	int root_handle;
> -	int constructor_sem_posted;
> +	int registration_done;
> 	int allowed;
> 	int global;
> 	int thread_active;
> @@ -256,6 +260,7 @@ struct sock_info {
> 	char *wait_shm_mmap;
> 	/* Keep track of lazy state dump not performed yet. */
> 	int statedump_pending;
> +	int initial_statedump_done;
> };
> 
> /* Socket from app (connect) to session daemon (listen) for communication */
> @@ -264,6 +269,7 @@ struct sock_info global_apps = {
> 	.global = 1,
> 
> 	.root_handle = -1,
> +	.registration_done = 0,
> 	.allowed = 0,
> 	.thread_active = 0,
> 
> @@ -274,6 +280,7 @@ struct sock_info global_apps = {
> 	.wait_shm_path = "/" LTTNG_UST_WAIT_FILENAME,
> 
> 	.statedump_pending = 0,
> +	.initial_statedump_done = 0,
> };
> 
> /* TODO: allow global_apps_sock_path override */
> @@ -282,6 +289,7 @@ struct sock_info local_apps = {
> 	.name = "local",
> 	.global = 0,
> 	.root_handle = -1,
> +	.registration_done = 0,
> 	.allowed = 0,	/* Check setuid bit first */
> 	.thread_active = 0,
> 
> @@ -289,6 +297,7 @@ struct sock_info local_apps = {
> 	.notify_socket = -1,
> 
> 	.statedump_pending = 0,
> +	.initial_statedump_done = 0,
> };
> 
> static int wait_poll_fallback;
> @@ -621,45 +630,81 @@ int send_reply(int sock, struct ustcomm_ust_reply *lur)
> }
> 
> static
> -int handle_register_done(struct sock_info *sock_info)
> +void decrement_sem_count(unsigned int count)
> {
> 	int ret;
> 
> -	if (sock_info->constructor_sem_posted)
> -		return 0;
> -	sock_info->constructor_sem_posted = 1;
> +	assert(uatomic_read(&sem_count) >= count);
> +
> 	if (uatomic_read(&sem_count) <= 0) {
> -		return 0;
> +		return;
> 	}
> -	ret = uatomic_add_return(&sem_count, -1);
> +
> +	ret = uatomic_add_return(&sem_count, -count);
> 	if (ret == 0) {
> 		ret = sem_post(&constructor_wait);
> 		assert(!ret);
> 	}
> +}
> +
> +static
> +int handle_register_done(struct sock_info *sock_info)
> +{
> +	if (sock_info->registration_done)
> +		return 0;
> +	sock_info->registration_done = 1;
> +
> +	decrement_sem_count(1);
> +
> +	return 0;
> +}
> +
> +static
> +int handle_register_failed(struct sock_info *sock_info)
> +{
> +	if (sock_info->registration_done)
> +		return 0;
> +	sock_info->registration_done = 1;
> +	sock_info->initial_statedump_done = 1;
> +
> +	decrement_sem_count(2);
> +
> 	return 0;
> }
> 
> /*
>  * Only execute pending statedump after the constructor semaphore has
> - * been posted by each listener thread. This means statedump will only
> - * be performed after the "registration done" command is received from
> - * each session daemon the application is connected to.
> + * been posted by the current listener thread. This means statedump will
> + * only be performed after the "registration done" command is received
> + * from this thread's session daemon.
>  *
>  * This ensures we don't run into deadlock issues with the dynamic
>  * loader mutex, which is held while the constructor is called and
>  * waiting on the constructor semaphore. All operations requiring this
>  * dynamic loader lock need to be postponed using this mechanism.
> + *
> + * In a scenario with two session daemons connected to the application,
> + * it is possible that the first listener thread which receives the
> + * registration done command issues its statedump while the dynamic
> + * loader lock is still held by the application constructor waiting on
> + * the semaphore. It will however be allowed to proceed when the
> + * second session daemon sends the registration done command to the
> + * second listener thread. This situation therefore does not produce
> + * a deadlock.
>  */
> static
> void handle_pending_statedump(struct sock_info *sock_info)
> {
> -	int ctor_passed = sock_info->constructor_sem_posted;
> -
> -	if (ctor_passed && sock_info->statedump_pending) {
> +	if (sock_info->registration_done && sock_info->statedump_pending) {
> 		sock_info->statedump_pending = 0;
> 		pthread_mutex_lock(&ust_fork_mutex);
> 		lttng_handle_pending_statedump(sock_info);
> 		pthread_mutex_unlock(&ust_fork_mutex);
> +
> +		if (!sock_info->initial_statedump_done) {
> +			sock_info->initial_statedump_done = 1;
> +			decrement_sem_count(1);
> +		}
> 	}
> }
> 
> @@ -1069,7 +1114,8 @@ void cleanup_sock_info(struct sock_info *sock_info, int
> exiting)
> 		}
> 		sock_info->root_handle = -1;
> 	}
> -	sock_info->constructor_sem_posted = 0;
> +	sock_info->registration_done = 0;
> +	sock_info->initial_statedump_done = 0;
> 
> 	/*
> 	 * wait_shm_mmap, socket and notify socket are used by listener
> @@ -1477,7 +1523,7 @@ restart:
> 		 * If we cannot find the sessiond daemon, don't delay
> 		 * constructor execution.
> 		 */
> -		ret = handle_register_done(sock_info);
> +		ret = handle_register_failed(sock_info);
> 		assert(!ret);
> 		ust_unlock();
> 		goto restart;
> @@ -1531,7 +1577,7 @@ restart:
> 		 * If we cannot register to the sessiond daemon, don't
> 		 * delay constructor execution.
> 		 */
> -		ret = handle_register_done(sock_info);
> +		ret = handle_register_failed(sock_info);
> 		assert(!ret);
> 		ust_unlock();
> 		goto restart;
> @@ -1560,7 +1606,7 @@ restart:
> 		 * If we cannot find the sessiond daemon, don't delay
> 		 * constructor execution.
> 		 */
> -		ret = handle_register_done(sock_info);
> +		ret = handle_register_failed(sock_info);
> 		assert(!ret);
> 		ust_unlock();
> 		goto restart;
> @@ -1624,7 +1670,7 @@ restart:
> 		 * If we cannot register to the sessiond daemon, don't
> 		 * delay constructor execution.
> 		 */
> -		ret = handle_register_done(sock_info);
> +		ret = handle_register_failed(sock_info);
> 		assert(!ret);
> 		ust_unlock();
> 		goto restart;
> @@ -1654,7 +1700,7 @@ restart:
> 			 * If we cannot register to the sessiond daemon, don't
> 			 * delay constructor execution.
> 			 */
> -			ret = handle_register_done(sock_info);
> +			ret = handle_register_failed(sock_info);
> 			assert(!ret);
> 			ust_unlock();
> 			goto end;
> @@ -1923,7 +1969,7 @@ void lttng_ust_cleanup(int exiting)
> 	exit_tracepoint();
> 	if (!exiting) {
> 		/* Reinitialize values for fork */
> -		sem_count = 2;
> +		sem_count = sem_count_initial_value;
> 		lttng_ust_comm_should_quit = 0;
> 		initialized = 0;
> 	}
> --
> 2.22.0

Patch
diff mbox series

diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
index a299ba84..0c6db9fe 100644
--- a/liblttng-ust/lttng-ust-comm.c
+++ b/liblttng-ust/lttng-ust-comm.c
@@ -228,7 +228,11 @@  static sem_t constructor_wait;
 /*
  * Doing this for both the global and local sessiond.
  */
-static int sem_count = { 2 };
+enum {
+	sem_count_initial_value = 4,
+};
+
+static int sem_count = sem_count_initial_value;
 
 /*
  * Counting nesting within lttng-ust. Used to ensure that calling fork()
@@ -243,7 +247,7 @@  struct sock_info {
 	const char *name;
 	pthread_t ust_listener;	/* listener thread */
 	int root_handle;
-	int constructor_sem_posted;
+	int registration_done;
 	int allowed;
 	int global;
 	int thread_active;
@@ -256,6 +260,7 @@  struct sock_info {
 	char *wait_shm_mmap;
 	/* Keep track of lazy state dump not performed yet. */
 	int statedump_pending;
+	int initial_statedump_done;
 };
 
 /* Socket from app (connect) to session daemon (listen) for communication */
@@ -264,6 +269,7 @@  struct sock_info global_apps = {
 	.global = 1,
 
 	.root_handle = -1,
+	.registration_done = 0,
 	.allowed = 0,
 	.thread_active = 0,
 
@@ -274,6 +280,7 @@  struct sock_info global_apps = {
 	.wait_shm_path = "/" LTTNG_UST_WAIT_FILENAME,
 
 	.statedump_pending = 0,
+	.initial_statedump_done = 0,
 };
 
 /* TODO: allow global_apps_sock_path override */
@@ -282,6 +289,7 @@  struct sock_info local_apps = {
 	.name = "local",
 	.global = 0,
 	.root_handle = -1,
+	.registration_done = 0,
 	.allowed = 0,	/* Check setuid bit first */
 	.thread_active = 0,
 
@@ -289,6 +297,7 @@  struct sock_info local_apps = {
 	.notify_socket = -1,
 
 	.statedump_pending = 0,
+	.initial_statedump_done = 0,
 };
 
 static int wait_poll_fallback;
@@ -621,45 +630,81 @@  int send_reply(int sock, struct ustcomm_ust_reply *lur)
 }
 
 static
-int handle_register_done(struct sock_info *sock_info)
+void decrement_sem_count(unsigned int count)
 {
 	int ret;
 
-	if (sock_info->constructor_sem_posted)
-		return 0;
-	sock_info->constructor_sem_posted = 1;
+	assert(uatomic_read(&sem_count) >= count);
+
 	if (uatomic_read(&sem_count) <= 0) {
-		return 0;
+		return;
 	}
-	ret = uatomic_add_return(&sem_count, -1);
+
+	ret = uatomic_add_return(&sem_count, -count);
 	if (ret == 0) {
 		ret = sem_post(&constructor_wait);
 		assert(!ret);
 	}
+}
+
+static
+int handle_register_done(struct sock_info *sock_info)
+{
+	if (sock_info->registration_done)
+		return 0;
+	sock_info->registration_done = 1;
+
+	decrement_sem_count(1);
+
+	return 0;
+}
+
+static
+int handle_register_failed(struct sock_info *sock_info)
+{
+	if (sock_info->registration_done)
+		return 0;
+	sock_info->registration_done = 1;
+	sock_info->initial_statedump_done = 1;
+
+	decrement_sem_count(2);
+
 	return 0;
 }
 
 /*
  * Only execute pending statedump after the constructor semaphore has
- * been posted by each listener thread. This means statedump will only
- * be performed after the "registration done" command is received from
- * each session daemon the application is connected to.
+ * been posted by the current listener thread. This means statedump will
+ * only be performed after the "registration done" command is received
+ * from this thread's session daemon.
  *
  * This ensures we don't run into deadlock issues with the dynamic
  * loader mutex, which is held while the constructor is called and
  * waiting on the constructor semaphore. All operations requiring this
  * dynamic loader lock need to be postponed using this mechanism.
+ *
+ * In a scenario with two session daemons connected to the application,
+ * it is possible that the first listener thread which receives the
+ * registration done command issues its statedump while the dynamic
+ * loader lock is still held by the application constructor waiting on
+ * the semaphore. It will however be allowed to proceed when the
+ * second session daemon sends the registration done command to the
+ * second listener thread. This situation therefore does not produce
+ * a deadlock.
  */
 static
 void handle_pending_statedump(struct sock_info *sock_info)
 {
-	int ctor_passed = sock_info->constructor_sem_posted;
-
-	if (ctor_passed && sock_info->statedump_pending) {
+	if (sock_info->registration_done && sock_info->statedump_pending) {
 		sock_info->statedump_pending = 0;
 		pthread_mutex_lock(&ust_fork_mutex);
 		lttng_handle_pending_statedump(sock_info);
 		pthread_mutex_unlock(&ust_fork_mutex);
+
+		if (!sock_info->initial_statedump_done) {
+			sock_info->initial_statedump_done = 1;
+			decrement_sem_count(1);
+		}
 	}
 }
 
@@ -1069,7 +1114,8 @@  void cleanup_sock_info(struct sock_info *sock_info, int exiting)
 		}
 		sock_info->root_handle = -1;
 	}
-	sock_info->constructor_sem_posted = 0;
+	sock_info->registration_done = 0;
+	sock_info->initial_statedump_done = 0;
 
 	/*
 	 * wait_shm_mmap, socket and notify socket are used by listener
@@ -1477,7 +1523,7 @@  restart:
 		 * If we cannot find the sessiond daemon, don't delay
 		 * constructor execution.
 		 */
-		ret = handle_register_done(sock_info);
+		ret = handle_register_failed(sock_info);
 		assert(!ret);
 		ust_unlock();
 		goto restart;
@@ -1531,7 +1577,7 @@  restart:
 		 * If we cannot register to the sessiond daemon, don't
 		 * delay constructor execution.
 		 */
-		ret = handle_register_done(sock_info);
+		ret = handle_register_failed(sock_info);
 		assert(!ret);
 		ust_unlock();
 		goto restart;
@@ -1560,7 +1606,7 @@  restart:
 		 * If we cannot find the sessiond daemon, don't delay
 		 * constructor execution.
 		 */
-		ret = handle_register_done(sock_info);
+		ret = handle_register_failed(sock_info);
 		assert(!ret);
 		ust_unlock();
 		goto restart;
@@ -1624,7 +1670,7 @@  restart:
 		 * If we cannot register to the sessiond daemon, don't
 		 * delay constructor execution.
 		 */
-		ret = handle_register_done(sock_info);
+		ret = handle_register_failed(sock_info);
 		assert(!ret);
 		ust_unlock();
 		goto restart;
@@ -1654,7 +1700,7 @@  restart:
 			 * If we cannot register to the sessiond daemon, don't
 			 * delay constructor execution.
 			 */
-			ret = handle_register_done(sock_info);
+			ret = handle_register_failed(sock_info);
 			assert(!ret);
 			ust_unlock();
 			goto end;
@@ -1923,7 +1969,7 @@  void lttng_ust_cleanup(int exiting)
 	exit_tracepoint();
 	if (!exiting) {
 		/* Reinitialize values for fork */
-		sem_count = 2;
+		sem_count = sem_count_initial_value;
 		lttng_ust_comm_should_quit = 0;
 		initialized = 0;
 	}