Thanks for getting to the bottom of it! Merged in master, stable-2.8 and stable-2.9.
Jérémie On 9 January 2017 at 11:23, Mathieu Desnoyers <mathieu.desnoy...@efficios.com> wrote: > The locking order comment in consumer.h is incorrect. First, its > description of locking order is not in sync with the comment found in > consumer-metadata-cache.h. The comment in struct consumer_metadata_cache > only states that the metadata cache lock nests inside the consumer_data > lock, and does not mention the stream lock, which implies that the > metadata cache lock does NOT nest inside the stream lock. But let's > investigate further to confirm: > > * lttng_consumer_read_subbuffer() acquires the stream lock, and then > calls lttng_ustconsumer_read_subbuffer() with stream lock held, > and then invokes commin_one_metadata_packet(), which acquires the > metadata cache lock. > > * lttng_ustconsumer_sync_metadata() acquires the metadata stream lock, > and calls commit_one_metadata_packet(), which takes the metadata cache > lock. > > Therefore, update the comment in consumer.h to state that the metadata > cache lock nests INSIDE the stream lock, and update > consumer_del_metadata_stream() accordingly. > > This should take care of fixing the locking order reversal found by > Coverity. > > CID 1368314 (#1 of 1): Thread deadlock (ORDER_REVERSAL) > CID 1368319: Program hangs (ORDER_REVERSAL) > > Fixes: 5feafd4130 "Fix: protect the channel's metadata stream using the > metadata cache lock" > Fixes: 1ea6cc572b "Fix: lock nesting order reversed" > Fixes: fb549e7ac2 "Fix: reverse channel and metadata cache lock nesting order" > Reported-by: Coverity Scan > Signed-off-by: Mathieu Desnoyers <mathieu.desnoy...@efficios.com> > --- > src/common/consumer/consumer.c | 4 ++-- > src/common/consumer/consumer.h | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c > index 89dad59..e379171 100644 > --- a/src/common/consumer/consumer.c > +++ b/src/common/consumer/consumer.c > @@ -2052,11 +2052,11 @@ void consumer_del_metadata_stream(struct > lttng_consumer_stream *stream, > > pthread_mutex_lock(&consumer_data.lock); > pthread_mutex_lock(&stream->chan->lock); > + pthread_mutex_lock(&stream->lock); > if (stream->chan->metadata_cache) { > /* Only applicable to userspace consumers. */ > pthread_mutex_lock(&stream->chan->metadata_cache->lock); > } > - pthread_mutex_lock(&stream->lock); > > /* Remove any reference to that stream. */ > consumer_stream_delete(stream, ht); > @@ -2080,10 +2080,10 @@ void consumer_del_metadata_stream(struct > lttng_consumer_stream *stream, > */ > stream->chan->metadata_stream = NULL; > > - pthread_mutex_unlock(&stream->lock); > if (stream->chan->metadata_cache) { > pthread_mutex_unlock(&stream->chan->metadata_cache->lock); > } > + pthread_mutex_unlock(&stream->lock); > pthread_mutex_unlock(&stream->chan->lock); > pthread_mutex_unlock(&consumer_data.lock); > > diff --git a/src/common/consumer/consumer.h b/src/common/consumer/consumer.h > index acefacb..37adecb 100644 > --- a/src/common/consumer/consumer.h > +++ b/src/common/consumer/consumer.h > @@ -337,9 +337,9 @@ struct lttng_consumer_stream { > * Lock to use the stream FDs since they are used between threads. > * > * This is nested INSIDE the consumer_data lock. > - * This is nested INSIDE the metadata cache lock. > * This is nested INSIDE the channel lock. > * This is nested INSIDE the channel timer lock. > + * This is nested OUTSIDE the metadata cache lock. > * This is nested OUTSIDE consumer_relayd_sock_pair lock. > */ > pthread_mutex_t lock; > -- > 2.1.4 > -- Jérémie Galarneau EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev