[ltt-dev] [UST PATCH 1/3] ust-consumerd: fix exit race crashes

[ltt-dev] [UST PATCH 1/3] ust-consumerd: fix exit race crashes

[Jason Wessel]

The ust-consumerd gets shutdown by the SIGTERM signal and a number of
places in the ust-consumerd did not properly deal with the case where
a system call returns EINTR in errno as a result of a signal to the
process.  The failure to handle EINTR properly was leading to some
data corruption in the buffer code and causing some random "victim"
crashes in lowlevel.c

The way all the offending functions were tracked down was to
temporarily add an abort() in the SIGTERM signal handler.  Then it was
a matter of looking at what threads were blocked on system calls at
the time outside of the thread that received the signal.

Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
---
 libustconsumer/libustconsumer.c |   25 +++++++++++++++++++------
 ust-consumerd/ust-consumerd.c   |   11 ++++++++++-
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/libustconsumer/libustconsumer.c b/libustconsumer/libustconsumer.c
index c5acffa..6f6d4bb 100644
--- a/libustconsumer/libustconsumer.c
+++ b/libustconsumer/libustconsumer.c
@@ -477,6 +477,8 @@ int consumer_loop(struct ustconsumer_instance *instance, struct buffer_info *buf
 			DBG("App died while being traced");
 			finish_consuming_dead_subbuffer(instance->callbacks, buf);
 			break;
+		} else if (read_result == -1 && errno == EINTR) {
+			continue;
 		}
 
 		if(instance->callbacks->on_read_subbuffer)
@@ -783,8 +785,11 @@ int ustconsumer_stop_instance(struct ustconsumer_instance *instance, int send_ms
 
 	struct sockaddr_un addr;
 
+socket_again:
 	result = fd = socket(PF_UNIX, SOCK_STREAM, 0);
 	if(result == -1) {
+		if (errno == EINTR)
+			goto socket_again;
 		PERROR("socket");
 		return 1;
 	}
@@ -794,13 +799,21 @@ int ustconsumer_stop_instance(struct ustconsumer_instance *instance, int send_ms
 	strncpy(addr.sun_path, instance->sock_path, UNIX_PATH_MAX);
 	addr.sun_path[UNIX_PATH_MAX-1] = '\0';
 
-	result = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
-	if(result == -1) {
-		PERROR("connect");
-	}
+connect_again:
+		result = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
+		if(result == -1) {
+			if (errno == EINTR)
+				goto connect_again;
+			PERROR("connect");
+		}
 
-	while(bytes != sizeof(msg))
-		bytes += send(fd, msg, sizeof(msg), 0);
+	while(bytes != sizeof(msg)) {
+		int inc = send(fd, msg, sizeof(msg), 0);
+		if (inc < 0 && errno != EINTR)
+			break;
+		else
+			bytes += inc;
+	}
 
 	close(fd);
 
diff --git a/ust-consumerd/ust-consumerd.c b/ust-consumerd/ust-consumerd.c
index ce2ee40..c961394 100644
--- a/ust-consumerd/ust-consumerd.c
+++ b/ust-consumerd/ust-consumerd.c
@@ -210,7 +210,11 @@ int on_open_buffer(struct ustconsumer_callbacks *data, struct buffer_info *buf)
 		    trace_path, buf->pid, buf->pidunique, buf->name);
 		return 1;
 	}
+again:
 	result = fd = open(tmp, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL, 00600);
+	if (result == -1 && errno == EINTR)
+		goto again;
+
 	if(result == -1) {
 		PERROR("open");
 		ERR("failed opening trace file %s", tmp);
@@ -225,7 +229,12 @@ int on_open_buffer(struct ustconsumer_callbacks *data, struct buffer_info *buf)
 int on_close_buffer(struct ustconsumer_callbacks *data, struct buffer_info *buf)
 {
 	struct buffer_info_local *buf_local = buf->user_data;
-	int result = close(buf_local->file_fd);
+	int result;
+
+again:
+	result = close(buf_local->file_fd);
+	if (result == -1 && errno == EINTR)
+		goto again;
 	free(buf_local);
 	if(result == -1) {
 		PERROR("close");
-- 
1.7.1

[ltt-dev] [UST PATCH 2/3] ust-consumerd: fix exit race log corruption

[Jason Wessel]

In the following scenario on an SMP system the ust-consumerd can end
up not properly closing out file handles which leads to log
corruption:
  * usttrace -m -l small_quick_app_lots_of_malloc_and_free
  * The app completes and usttrace sees and sends the SIGTERM to ust-consumerd
  * The ust-consumerd main thread will exit and the _exit() handlers
    kills off the remaining pthreads without everything getting closed out

The solution to the problem is to introduce an active_thread count for
the private ustconsumer_instance.  This counter will be zeroed out
when it is safe to completely shutdown the main thread, which will
subsequently run the _exit() handlers.

Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
---
 include/ust/ustconsumer.h       |    1 +
 libustconsumer/libustconsumer.c |   10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/include/ust/ustconsumer.h b/include/ust/ustconsumer.h
index f99f8f1..cde8440 100644
--- a/include/ust/ustconsumer.h
+++ b/include/ust/ustconsumer.h
@@ -89,6 +89,7 @@ struct ustconsumer_instance {
 	char *sock_path;
 	pthread_mutex_t mutex;
 	int active_buffers;
+	int active_threads;
 };
 
 /**
diff --git a/libustconsumer/libustconsumer.c b/libustconsumer/libustconsumer.c
index 6f6d4bb..a723540 100644
--- a/libustconsumer/libustconsumer.c
+++ b/libustconsumer/libustconsumer.c
@@ -543,6 +543,10 @@ void *consumer_thread(void *arg)
 	int result;
 	sigset_t sigset;
 
+	pthread_mutex_lock(&args->instance->mutex);
+	args->instance->active_threads++;
+	pthread_mutex_unlock(&args->instance->mutex);
+
 	if(args->instance->callbacks->on_new_thread)
 		args->instance->callbacks->on_new_thread(args->instance->callbacks);
 
@@ -584,6 +588,10 @@ void *consumer_thread(void *arg)
 	if(args->instance->callbacks->on_close_thread)
 		args->instance->callbacks->on_close_thread(args->instance->callbacks);
 
+	pthread_mutex_lock(&args->instance->mutex);
+	args->instance->active_threads--;
+	pthread_mutex_unlock(&args->instance->mutex);
+
 	free((void *)args->channel);
 	free(args);
 	return NULL;
@@ -735,7 +743,7 @@ int ustconsumer_start_instance(struct ustconsumer_instance *instance)
 
 		if (instance->quit_program) {
 			pthread_mutex_lock(&instance->mutex);
-			if(instance->active_buffers == 0) {
+			if(instance->active_buffers == 0 && instance->active_threads == 0) {
 				pthread_mutex_unlock(&instance->mutex);
 				break;
 			}
-- 
1.7.1

[ltt-dev] [UST PATCH 3/3] support busybox for manual trace tests

[Jason Wessel]

The busybox version of find does not support using -L, and it does not
appear to be needed in order to pass the test.  In the interest of
being able to run internal UST tests on a busybox based rootfs,
appropriately detect if -L is available as an argument to find and use
it when it is available.

Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
---
 tests/manual_mode_tracing.sh |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/tests/manual_mode_tracing.sh b/tests/manual_mode_tracing.sh
index 19044ba..f9bc793 100755
--- a/tests/manual_mode_tracing.sh
+++ b/tests/manual_mode_tracing.sh
@@ -43,7 +43,14 @@ UST_CONSUMERD_PID="$(<$pidfilepath)"
 
 LIB_UST="$TESTDIR/../libust/.libs/libust.so.0.0.0"
 LIB_UST_MALLOC="$TESTDIR/../libustinstr-malloc/.libs/libustinstr-malloc.so"
-LD_PRELOAD="$LIB_UST:$LIB_UST_MALLOC" find -L / >/dev/null 2>&1 &
+# Check to see if find supports -L
+find . -maxdepth 0 -L > /dev/null 2>&1
+if [ $? = 0 ] ; then
+   USE_L="-L"
+else
+   USE_L=""
+fi
+LD_PRELOAD="$LIB_UST:$LIB_UST_MALLOC" find $USE_L / >/dev/null 2>&1 &
 PID=$!
 TRACE=auto
 USTCTL="$TESTDIR/../ustctl/ustctl"
-- 
1.7.1

[ltt-dev] [UST PATCH 1/3] ust-consumerd: fix exit race crashes

[Nils Carlson]

merged.

On Wed, 27 Apr 2011, Jason Wessel wrote:

> The ust-consumerd gets shutdown by the SIGTERM signal and a number of
> places in the ust-consumerd did not properly deal with the case where
> a system call returns EINTR in errno as a result of a signal to the
> process.  The failure to handle EINTR properly was leading to some
> data corruption in the buffer code and causing some random "victim"
> crashes in lowlevel.c
>
> The way all the offending functions were tracked down was to
> temporarily add an abort() in the SIGTERM signal handler.  Then it was
> a matter of looking at what threads were blocked on system calls at
> the time outside of the thread that received the signal.
>
> Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
> ---
> libustconsumer/libustconsumer.c |   25 +++++++++++++++++++------
> ust-consumerd/ust-consumerd.c   |   11 ++++++++++-
> 2 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/libustconsumer/libustconsumer.c b/libustconsumer/libustconsumer.c
> index c5acffa..6f6d4bb 100644
> --- a/libustconsumer/libustconsumer.c
> +++ b/libustconsumer/libustconsumer.c
> @@ -477,6 +477,8 @@ int consumer_loop(struct ustconsumer_instance *instance, struct buffer_info *buf
> 			DBG("App died while being traced");
> 			finish_consuming_dead_subbuffer(instance->callbacks, buf);
> 			break;
> +		} else if (read_result == -1 && errno == EINTR) {
> +			continue;
> 		}
>
> 		if(instance->callbacks->on_read_subbuffer)
> @@ -783,8 +785,11 @@ int ustconsumer_stop_instance(struct ustconsumer_instance *instance, int send_ms
>
> 	struct sockaddr_un addr;
>
> +socket_again:
> 	result = fd = socket(PF_UNIX, SOCK_STREAM, 0);
> 	if(result == -1) {
> +		if (errno == EINTR)
> +			goto socket_again;
> 		PERROR("socket");
> 		return 1;
> 	}
> @@ -794,13 +799,21 @@ int ustconsumer_stop_instance(struct ustconsumer_instance *instance, int send_ms
> 	strncpy(addr.sun_path, instance->sock_path, UNIX_PATH_MAX);
> 	addr.sun_path[UNIX_PATH_MAX-1] = '\0';
>
> -	result = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
> -	if(result == -1) {
> -		PERROR("connect");
> -	}
> +connect_again:
> +		result = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
> +		if(result == -1) {
> +			if (errno == EINTR)
> +				goto connect_again;
> +			PERROR("connect");
> +		}
>
> -	while(bytes != sizeof(msg))
> -		bytes += send(fd, msg, sizeof(msg), 0);
> +	while(bytes != sizeof(msg)) {
> +		int inc = send(fd, msg, sizeof(msg), 0);
> +		if (inc < 0 && errno != EINTR)
> +			break;
> +		else
> +			bytes += inc;
> +	}
>
> 	close(fd);
>
> diff --git a/ust-consumerd/ust-consumerd.c b/ust-consumerd/ust-consumerd.c
> index ce2ee40..c961394 100644
> --- a/ust-consumerd/ust-consumerd.c
> +++ b/ust-consumerd/ust-consumerd.c
> @@ -210,7 +210,11 @@ int on_open_buffer(struct ustconsumer_callbacks *data, struct buffer_info *buf)
> 		    trace_path, buf->pid, buf->pidunique, buf->name);
> 		return 1;
> 	}
> +again:
> 	result = fd = open(tmp, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL, 00600);
> +	if (result == -1 && errno == EINTR)
> +		goto again;
> +
> 	if(result == -1) {
> 		PERROR("open");
> 		ERR("failed opening trace file %s", tmp);
> @@ -225,7 +229,12 @@ int on_open_buffer(struct ustconsumer_callbacks *data, struct buffer_info *buf)
> int on_close_buffer(struct ustconsumer_callbacks *data, struct buffer_info *buf)
> {
> 	struct buffer_info_local *buf_local = buf->user_data;
> -	int result = close(buf_local->file_fd);
> +	int result;
> +
> +again:
> +	result = close(buf_local->file_fd);
> +	if (result == -1 && errno == EINTR)
> +		goto again;
> 	free(buf_local);
> 	if(result == -1) {
> 		PERROR("close");
> -- 
> 1.7.1
>
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>

[ltt-dev] [UST PATCH 2/3] ust-consumerd: fix exit race log corruption

[Nils Carlson]

merged.

On Wed, 27 Apr 2011, Jason Wessel wrote:

> In the following scenario on an SMP system the ust-consumerd can end
> up not properly closing out file handles which leads to log
> corruption:
>  * usttrace -m -l small_quick_app_lots_of_malloc_and_free
>  * The app completes and usttrace sees and sends the SIGTERM to ust-consumerd
>  * The ust-consumerd main thread will exit and the _exit() handlers
>    kills off the remaining pthreads without everything getting closed out
>
> The solution to the problem is to introduce an active_thread count for
> the private ustconsumer_instance.  This counter will be zeroed out
> when it is safe to completely shutdown the main thread, which will
> subsequently run the _exit() handlers.
>
> Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
> ---
> include/ust/ustconsumer.h       |    1 +
> libustconsumer/libustconsumer.c |   10 +++++++++-
> 2 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/include/ust/ustconsumer.h b/include/ust/ustconsumer.h
> index f99f8f1..cde8440 100644
> --- a/include/ust/ustconsumer.h
> +++ b/include/ust/ustconsumer.h
> @@ -89,6 +89,7 @@ struct ustconsumer_instance {
> 	char *sock_path;
> 	pthread_mutex_t mutex;
> 	int active_buffers;
> +	int active_threads;
> };
>
> /**
> diff --git a/libustconsumer/libustconsumer.c b/libustconsumer/libustconsumer.c
> index 6f6d4bb..a723540 100644
> --- a/libustconsumer/libustconsumer.c
> +++ b/libustconsumer/libustconsumer.c
> @@ -543,6 +543,10 @@ void *consumer_thread(void *arg)
> 	int result;
> 	sigset_t sigset;
>
> +	pthread_mutex_lock(&args->instance->mutex);
> +	args->instance->active_threads++;
> +	pthread_mutex_unlock(&args->instance->mutex);
> +
> 	if(args->instance->callbacks->on_new_thread)
> 		args->instance->callbacks->on_new_thread(args->instance->callbacks);
>
> @@ -584,6 +588,10 @@ void *consumer_thread(void *arg)
> 	if(args->instance->callbacks->on_close_thread)
> 		args->instance->callbacks->on_close_thread(args->instance->callbacks);
>
> +	pthread_mutex_lock(&args->instance->mutex);
> +	args->instance->active_threads--;
> +	pthread_mutex_unlock(&args->instance->mutex);
> +
> 	free((void *)args->channel);
> 	free(args);
> 	return NULL;
> @@ -735,7 +743,7 @@ int ustconsumer_start_instance(struct ustconsumer_instance *instance)
>
> 		if (instance->quit_program) {
> 			pthread_mutex_lock(&instance->mutex);
> -			if(instance->active_buffers == 0) {
> +			if(instance->active_buffers == 0 && instance->active_threads == 0) {
> 				pthread_mutex_unlock(&instance->mutex);
> 				break;
> 			}
> -- 
> 1.7.1
>
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>

[ltt-dev] [UST PATCH 3/3] support busybox for manual trace tests

[Nils Carlson]

merged.

On Wed, 27 Apr 2011, Jason Wessel wrote:

> The busybox version of find does not support using -L, and it does not
> appear to be needed in order to pass the test.  In the interest of
> being able to run internal UST tests on a busybox based rootfs,
> appropriately detect if -L is available as an argument to find and use
> it when it is available.
>
> Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
> ---
> tests/manual_mode_tracing.sh |    9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/tests/manual_mode_tracing.sh b/tests/manual_mode_tracing.sh
> index 19044ba..f9bc793 100755
> --- a/tests/manual_mode_tracing.sh
> +++ b/tests/manual_mode_tracing.sh
> @@ -43,7 +43,14 @@ UST_CONSUMERD_PID="$(<$pidfilepath)"
>
> LIB_UST="$TESTDIR/../libust/.libs/libust.so.0.0.0"
> LIB_UST_MALLOC="$TESTDIR/../libustinstr-malloc/.libs/libustinstr-malloc.so"
> -LD_PRELOAD="$LIB_UST:$LIB_UST_MALLOC" find -L / >/dev/null 2>&1 &
> +# Check to see if find supports -L
> +find . -maxdepth 0 -L > /dev/null 2>&1
> +if [ $? = 0 ] ; then
> +   USE_L="-L"
> +else
> +   USE_L=""
> +fi
> +LD_PRELOAD="$LIB_UST:$LIB_UST_MALLOC" find $USE_L / >/dev/null 2>&1 &
> PID=$!
> TRACE=auto
> USTCTL="$TESTDIR/../ustctl/ustctl"
> -- 
> 1.7.1
>
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>

[ltt-dev] [UST PATCH 2/3] ust-consumerd: fix exit race log corruption

[Mathieu Desnoyers]

* Nils Carlson (nils.carlson at ericsson.com) wrote:
> merged.

This is a step in the right direction, but we should not stop there: I
recommend to:

1 - create a linked list of active threads. list_add should be called
    after each pthread_create.
2 - remove the call to pthread_detach().
3 - before exit, iterate on each thread of the list, calling
    pthread_join for each of them.
4 - then we can remove the counter-based scheme proposed by this patch.

Because with this scheme, we still race between exit() and thread memory
free/teardown. It won't cause much problem in practice, but valgrind is
likely to complain.

Thanks,

Mathieu

>
> On Wed, 27 Apr 2011, Jason Wessel wrote:
>
>> In the following scenario on an SMP system the ust-consumerd can end
>> up not properly closing out file handles which leads to log
>> corruption:
>>  * usttrace -m -l small_quick_app_lots_of_malloc_and_free
>>  * The app completes and usttrace sees and sends the SIGTERM to ust-consumerd
>>  * The ust-consumerd main thread will exit and the _exit() handlers
>>    kills off the remaining pthreads without everything getting closed out
>>
>> The solution to the problem is to introduce an active_thread count for
>> the private ustconsumer_instance.  This counter will be zeroed out
>> when it is safe to completely shutdown the main thread, which will
>> subsequently run the _exit() handlers.
>>
>> Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
>> ---
>> include/ust/ustconsumer.h       |    1 +
>> libustconsumer/libustconsumer.c |   10 +++++++++-
>> 2 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/ust/ustconsumer.h b/include/ust/ustconsumer.h
>> index f99f8f1..cde8440 100644
>> --- a/include/ust/ustconsumer.h
>> +++ b/include/ust/ustconsumer.h
>> @@ -89,6 +89,7 @@ struct ustconsumer_instance {
>> 	char *sock_path;
>> 	pthread_mutex_t mutex;
>> 	int active_buffers;
>> +	int active_threads;
>> };
>>
>> /**
>> diff --git a/libustconsumer/libustconsumer.c b/libustconsumer/libustconsumer.c
>> index 6f6d4bb..a723540 100644
>> --- a/libustconsumer/libustconsumer.c
>> +++ b/libustconsumer/libustconsumer.c
>> @@ -543,6 +543,10 @@ void *consumer_thread(void *arg)
>> 	int result;
>> 	sigset_t sigset;
>>
>> +	pthread_mutex_lock(&args->instance->mutex);
>> +	args->instance->active_threads++;
>> +	pthread_mutex_unlock(&args->instance->mutex);
>> +
>> 	if(args->instance->callbacks->on_new_thread)
>> 		args->instance->callbacks->on_new_thread(args->instance->callbacks);
>>
>> @@ -584,6 +588,10 @@ void *consumer_thread(void *arg)
>> 	if(args->instance->callbacks->on_close_thread)
>> 		args->instance->callbacks->on_close_thread(args->instance->callbacks);
>>
>> +	pthread_mutex_lock(&args->instance->mutex);
>> +	args->instance->active_threads--;
>> +	pthread_mutex_unlock(&args->instance->mutex);
>> +
>> 	free((void *)args->channel);
>> 	free(args);
>> 	return NULL;
>> @@ -735,7 +743,7 @@ int ustconsumer_start_instance(struct ustconsumer_instance *instance)
>>
>> 		if (instance->quit_program) {
>> 			pthread_mutex_lock(&instance->mutex);
>> -			if(instance->active_buffers == 0) {
>> +			if(instance->active_buffers == 0 && instance->active_threads == 0) {
>> 				pthread_mutex_unlock(&instance->mutex);
>> 				break;
>> 			}
>> -- 
>> 1.7.1
>>
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com