From f613e3e6ea34dec9fe9232056bc02f6d80d95965 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Mon, 15 Jul 2013 11:26:06 -0400 Subject: [PATCH] Fix: allow get/put subbuf in loop for metadata stream data_pending check can trigger the following sequence: - get_next_subbuf - put_subbuf - get_next_subbuf - put_subbuf - get_next_subbuf - put_subbuf ... and then finally a thread would consume the data: - get_next_subbuf - put_next_subbuf However, we don't want to populate data from the metadata cache into the stream until put_next_subbuf is issued. Add a check to ensure that it is not populated until required. Also, disallow get_subbuf() ioctl on metadata channel: its random-access semantic does not play well with serialization of the metadata cache on demand. Reviewed-by: Julien Desfossez Signed-off-by: Mathieu Desnoyers --- lttng-abi.c | 65 +++++++++++++++++++++++++++++++++++++++++++------- lttng-events.c | 14 ++++++++--- lttng-events.h | 3 ++- 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/lttng-abi.c b/lttng-abi.c index 45d78d97..ba8362b1 100644 --- a/lttng-abi.c +++ b/lttng-abi.c @@ -539,7 +539,7 @@ unsigned int lttng_metadata_ring_buffer_poll(struct file *filp, mask |= POLLHUP; if (stream->metadata_cache->metadata_written > - stream->metadata_cache_read) + stream->metadata_out) mask |= POLLIN; } @@ -547,7 +547,7 @@ unsigned int lttng_metadata_ring_buffer_poll(struct file *filp, } static -int lttng_metadata_ring_buffer_ioctl_get_subbuf(struct file *filp, +int lttng_metadata_ring_buffer_ioctl_get_next_subbuf(struct file *filp, unsigned int cmd, unsigned long arg) { struct lttng_metadata_stream *stream = filp->private_data; @@ -564,6 +564,15 @@ int lttng_metadata_ring_buffer_ioctl_get_subbuf(struct file *filp, return ret; } +static +void lttng_metadata_ring_buffer_ioctl_put_next_subbuf(struct file *filp, + unsigned int cmd, unsigned long arg) +{ + struct lttng_metadata_stream *stream = filp->private_data; + + stream->metadata_out = stream->metadata_in; +} + static long lttng_metadata_ring_buffer_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) @@ -573,21 +582,41 @@ long lttng_metadata_ring_buffer_ioctl(struct file *filp, struct lib_ring_buffer *buf = stream->priv; switch (cmd) { - case RING_BUFFER_GET_SUBBUF: case RING_BUFFER_GET_NEXT_SUBBUF: { - ret = lttng_metadata_ring_buffer_ioctl_get_subbuf(filp, + ret = lttng_metadata_ring_buffer_ioctl_get_next_subbuf(filp, cmd, arg); if (ret < 0) goto err; break; } + case RING_BUFFER_GET_SUBBUF: + { + /* + * Random access is not allowed for metadata channel. + */ + return -ENOSYS; + } default: break; } + /* PUT_SUBBUF is the one from lib ring buffer, unmodified. */ + /* Performing lib ring buffer ioctl after our own. */ - return lib_ring_buffer_ioctl(filp, cmd, arg, buf); + ret = lib_ring_buffer_ioctl(filp, cmd, arg, buf); + if (ret < 0) + goto err; + switch (cmd) { + case RING_BUFFER_PUT_NEXT_SUBBUF: + { + lttng_metadata_ring_buffer_ioctl_put_next_subbuf(filp, + cmd, arg); + break; + } + default: + break; + } err: return ret; } @@ -602,21 +631,41 @@ long lttng_metadata_ring_buffer_compat_ioctl(struct file *filp, struct lib_ring_buffer *buf = stream->priv; switch (cmd) { - case RING_BUFFER_GET_SUBBUF: case RING_BUFFER_GET_NEXT_SUBBUF: { - ret = lttng_metadata_ring_buffer_ioctl_get_subbuf(filp, + ret = lttng_metadata_ring_buffer_ioctl_get_next_subbuf(filp, cmd, arg); if (ret < 0) goto err; break; } + case RING_BUFFER_GET_SUBBUF: + { + /* + * Random access is not allowed for metadata channel. + */ + return -ENOSYS; + } default: break; } + /* PUT_SUBBUF is the one from lib ring buffer, unmodified. */ + /* Performing lib ring buffer ioctl after our own. */ - return lib_ring_buffer_compat_ioctl(filp, cmd, arg, buf); + ret = lib_ring_buffer_compat_ioctl(filp, cmd, arg, buf); + if (ret < 0) + goto err; + switch (cmd) { + case RING_BUFFER_PUT_NEXT_SUBBUF: + { + lttng_metadata_ring_buffer_ioctl_put_next_subbuf(filp, + cmd, arg); + break; + } + default: + break; + } err: return ret; } diff --git a/lttng-events.c b/lttng-events.c index 27a8f864..567df65a 100644 --- a/lttng-events.c +++ b/lttng-events.c @@ -561,8 +561,16 @@ int lttng_metadata_output_channel(struct lttng_channel *chan, int ret = 0; size_t len, reserve_len; + /* + * Ensure we support mutiple get_next / put sequences followed + * by put_next. + */ + WARN_ON(stream->metadata_in < stream->metadata_out); + if (stream->metadata_in != stream->metadata_out) + return 0; + len = stream->metadata_cache->metadata_written - - stream->metadata_cache_read; + stream->metadata_in; if (!len) return 0; reserve_len = min_t(size_t, @@ -579,10 +587,10 @@ int lttng_metadata_output_channel(struct lttng_channel *chan, goto end; } chan->ops->event_write(&ctx, - stream->metadata_cache->data + stream->metadata_cache_read, + stream->metadata_cache->data + stream->metadata_in, reserve_len); chan->ops->event_commit(&ctx); - stream->metadata_cache_read += reserve_len; + stream->metadata_in += reserve_len; ret = reserve_len; end: diff --git a/lttng-events.h b/lttng-events.h index 46d80bcd..4c1f3228 100644 --- a/lttng-events.h +++ b/lttng-events.h @@ -278,7 +278,8 @@ struct lttng_channel { struct lttng_metadata_stream { void *priv; /* Ring buffer private data */ struct lttng_metadata_cache *metadata_cache; - unsigned int metadata_cache_read; /* Bytes read from the cache */ + unsigned int metadata_in; /* Bytes read from the cache */ + unsigned int metadata_out; /* Bytes consumed from stream */ int finalized; /* Has channel been finalized */ wait_queue_head_t read_wait; /* Reader buffer-level wait queue */ struct list_head list; /* Stream list */ -- 2.34.1