From: David Goulet <dgoulet@efficios.com>
Date: Tue, 4 Dec 2012 23:10:45 +0000 (-0500)
Subject: Fix: locking order between consumer and stream
X-Git-Tag: v2.1.0~79
X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=74251bb8269c4609c776e28fcd137628225e6ca7

Fix: locking order between consumer and stream

Also, lock the stream BEFORE calling the read subbuffer so not to race
with the data pending command.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: David Goulet <dgoulet@efficios.com>
---

diff --git a/src/common/consumer.c b/src/common/consumer.c
index 260779ae6..0e73bc9b8 100644
--- a/src/common/consumer.c
+++ b/src/common/consumer.c
@@ -348,8 +348,8 @@ void consumer_del_stream(struct lttng_consumer_stream *stream,
 		goto free_stream;
 	}
 
-	pthread_mutex_lock(&stream->lock);
 	pthread_mutex_lock(&consumer_data.lock);
+	pthread_mutex_lock(&stream->lock);
 
 	switch (consumer_data.type) {
 	case LTTNG_CONSUMER_KERNEL:
@@ -441,8 +441,8 @@ void consumer_del_stream(struct lttng_consumer_stream *stream,
 
 end:
 	consumer_data.need_update = 1;
-	pthread_mutex_unlock(&consumer_data.lock);
 	pthread_mutex_unlock(&stream->lock);
+	pthread_mutex_unlock(&consumer_data.lock);
 
 	if (free_chan) {
 		consumer_del_channel(free_chan);
@@ -1280,8 +1280,6 @@ ssize_t lttng_consumer_on_read_subbuffer_mmap(
 	/* RCU lock for the relayd pointer */
 	rcu_read_lock();
 
-	pthread_mutex_lock(&stream->lock);
-
 	/* Flag that the current stream if set for network streaming. */
 	if (stream->net_seq_idx != -1) {
 		relayd = consumer_find_relayd(stream->net_seq_idx);
@@ -1406,7 +1404,6 @@ end:
 	if (relayd && stream->metadata_flag) {
 		pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
 	}
-	pthread_mutex_unlock(&stream->lock);
 
 	rcu_read_unlock();
 	return written;
@@ -1447,8 +1444,6 @@ ssize_t lttng_consumer_on_read_subbuffer_splice(
 	/* RCU lock for the relayd pointer */
 	rcu_read_lock();
 
-	pthread_mutex_lock(&stream->lock);
-
 	/* Flag that the current stream if set for network streaming. */
 	if (stream->net_seq_idx != -1) {
 		relayd = consumer_find_relayd(stream->net_seq_idx);
@@ -1616,7 +1611,6 @@ end:
 	if (relayd && stream->metadata_flag) {
 		pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
 	}
-	pthread_mutex_unlock(&stream->lock);
 
 	rcu_read_unlock();
 	return written;
@@ -1762,9 +1756,9 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream,
 		goto free_stream;
 	}
 
+	pthread_mutex_lock(&consumer_data.lock);
 	pthread_mutex_lock(&stream->lock);
 
-	pthread_mutex_lock(&consumer_data.lock);
 	switch (consumer_data.type) {
 	case LTTNG_CONSUMER_KERNEL:
 		if (stream->mmap_base != NULL) {
@@ -1854,8 +1848,8 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream,
 	}
 
 end:
-	pthread_mutex_unlock(&consumer_data.lock);
 	pthread_mutex_unlock(&stream->lock);
+	pthread_mutex_unlock(&consumer_data.lock);
 
 	if (free_chan) {
 		consumer_del_channel(free_chan);
@@ -2559,17 +2553,27 @@ end:
 ssize_t lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream,
 		struct lttng_consumer_local_data *ctx)
 {
+	ssize_t ret;
+
+	pthread_mutex_lock(&stream->lock);
+
 	switch (consumer_data.type) {
 	case LTTNG_CONSUMER_KERNEL:
-		return lttng_kconsumer_read_subbuffer(stream, ctx);
+		ret = lttng_kconsumer_read_subbuffer(stream, ctx);
+		break;
 	case LTTNG_CONSUMER32_UST:
 	case LTTNG_CONSUMER64_UST:
-		return lttng_ustconsumer_read_subbuffer(stream, ctx);
+		ret = lttng_ustconsumer_read_subbuffer(stream, ctx);
+		break;
 	default:
 		ERR("Unknown consumer_data type");
 		assert(0);
-		return -ENOSYS;
+		ret = -ENOSYS;
+		break;
 	}
+
+	pthread_mutex_unlock(&stream->lock);
+	return ret;
 }
 
 int lttng_consumer_on_recv_stream(struct lttng_consumer_stream *stream)
diff --git a/src/common/consumer.h b/src/common/consumer.h
index 8411ff964..87ba490d7 100644
--- a/src/common/consumer.h
+++ b/src/common/consumer.h
@@ -132,10 +132,10 @@ struct lttng_consumer_stream {
 	/* Next sequence number to use for trace packet */
 	uint64_t next_net_seq_num;
 	/*
-	 * Lock to use the stream FDs since they are used between threads. Using
-	 * this lock with network streaming, when using the control mutex of a
-	 * consumer_relayd_sock_pair, make sure to acquire this lock BEFORE locking
-	 * it and releasing it AFTER the control mutex unlock.
+	 * Lock to use the stream FDs since they are used between threads.
+	 *
+	 * This is nested INSIDE the consumer_data lock.
+	 * This is nested OUTSIDE consumer_relayd_sock_pair lock.
 	 */
 	pthread_mutex_t lock;
 	/* Tracing session id */
@@ -170,6 +170,9 @@ struct consumer_relayd_sock_pair {
 	 * between threads sending data to the relayd. Since metadata data is sent
 	 * over that socket, at least two sendmsg() are needed (header + data)
 	 * creating a race for packets to overlap between threads using it.
+	 *
+	 * This is nested INSIDE the consumer_data lock.
+	 * This is nested INSIDE the stream lock.
 	 */
 	pthread_mutex_t ctrl_sock_mutex;
 
@@ -257,8 +260,8 @@ struct lttng_consumer_global_data {
 	 * and number of element in the hash table. It's also a protection for
 	 * concurrent read/write between threads.
 	 *
-	 * XXX: We need to see if this lock is still needed with the lockless RCU
-	 * hash tables.
+	 * This is nested OUTSIDE the stream lock.
+	 * This is nested OUTSIDE the consumer_relayd_sock_pair lock.
 	 */
 	pthread_mutex_t lock;