From 591918c399b848d99f29f8d4043a88a749152efc Mon Sep 17 00:00:00 2001
From: David Goulet <dgoulet@efficios.com>
Date: Tue, 4 Dec 2012 18:10:45 -0500
Subject: [PATCH 1/2] 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: David Goulet <dgoulet@efficios.com>
---
 src/common/consumer.c |   27 ++++++++++++++-------------
 src/common/consumer.h |   15 +++++++++------
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/src/common/consumer.c b/src/common/consumer.c
index dbec177..7acdda2 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);
@@ -1278,8 +1278,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);
@@ -1404,7 +1402,6 @@ end:
 	if (relayd && stream->metadata_flag) {
 		pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
 	}
-	pthread_mutex_unlock(&stream->lock);
 
 	rcu_read_unlock();
 	return written;
@@ -1445,8 +1442,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);
@@ -1614,7 +1609,6 @@ end:
 	if (relayd && stream->metadata_flag) {
 		pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
 	}
-	pthread_mutex_unlock(&stream->lock);
 
 	rcu_read_unlock();
 	return written;
@@ -1760,9 +1754,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) {
@@ -1852,8 +1846,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);
@@ -2547,17 +2541,24 @@ end:
 ssize_t lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream,
 		struct lttng_consumer_local_data *ctx)
 {
+	int 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);
 	case LTTNG_CONSUMER32_UST:
 	case LTTNG_CONSUMER64_UST:
-		return lttng_ustconsumer_read_subbuffer(stream, ctx);
+		ret = lttng_ustconsumer_read_subbuffer(stream, ctx);
 	default:
 		ERR("Unknown consumer_data type");
 		assert(0);
-		return -ENOSYS;
+		ret = -ENOSYS;
 	}
+
+	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 8411ff9..87ba490 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;
 
-- 
1.7.10.4

