diff mbox series

[babeltrace] Fix: lttng-live discarded event count after inactivity

Message ID 1515535974-8377-1-git-send-email-jdesfossez@efficios.com
State Accepted, archived
Headers show
Series [babeltrace] Fix: lttng-live discarded event count after inactivity | expand

Commit Message

Julien Desfossez Jan. 9, 2018, 10:12 p.m. UTC
When a stream is inactive, the consumer produces fake indexes which are
beacons to let the viewer know a stream has not produced any data up to
a certain timestamp. These beacon are actually real packet indexes with
all the fields set to 0 except for the timestamp_end. Currently we keep
these beacons just like we keep real indexes. The problem is that when
we switch packet, we compare the events_discarded field in the index we
just received with the same field in the previous index. In the case
where a stream has been inactive, we have received inactivity beacons,
and set the discarded_event field to 0, so the difference with the next
real index might be wrong.

In fact, since the inactivity beacons are only used to push the
timestamp end of a stream, we don't need to keep them and we actually
need to keep most of the data from the real previous index. So we now
copy the entire prev_index into the cur_index when we receive an
inactivity beacon. We could refactor the code to avoid performing the
pointer swap of cur and prev indexes, but this implies a redesign of
much of the packet switching code which would affect other code paths.

Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
---
 formats/lttng-live/lttng-live-comm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Jonathan Rajotte Jan. 9, 2018, 10:21 p.m. UTC | #1
Hi,

Julien forgot to mention that this is for the stable 1.5 branch.

Cheers

On Tue, Jan 09, 2018 at 05:12:54PM -0500, Julien Desfossez wrote:
> When a stream is inactive, the consumer produces fake indexes which are
> beacons to let the viewer know a stream has not produced any data up to
> a certain timestamp. These beacon are actually real packet indexes with
> all the fields set to 0 except for the timestamp_end. Currently we keep
> these beacons just like we keep real indexes. The problem is that when
> we switch packet, we compare the events_discarded field in the index we
> just received with the same field in the previous index. In the case
> where a stream has been inactive, we have received inactivity beacons,
> and set the discarded_event field to 0, so the difference with the next
> real index might be wrong.
> 
> In fact, since the inactivity beacons are only used to push the
> timestamp end of a stream, we don't need to keep them and we actually
> need to keep most of the data from the real previous index. So we now
> copy the entire prev_index into the cur_index when we receive an
> inactivity beacon. We could refactor the code to avoid performing the
> pointer swap of cur and prev indexes, but this implies a redesign of
> much of the packet switching code which would affect other code paths.
> 
> Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
> ---
>  formats/lttng-live/lttng-live-comm.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/formats/lttng-live/lttng-live-comm.c b/formats/lttng-live/lttng-live-comm.c
> index 77bf34a..3ffa257 100644
> --- a/formats/lttng-live/lttng-live-comm.c
> +++ b/formats/lttng-live/lttng-live-comm.c
> @@ -1275,6 +1275,7 @@ retry:
>  		pos->offset = 0;
>  	}
>  
> +	/* Beacon packet index */
>  	if (cur_index->content_size == 0) {
>  		if (file_stream->parent.stream_class) {
>  			file_stream->parent.cycles_timestamp =
> @@ -1282,8 +1283,23 @@ retry:
>  			file_stream->parent.real_timestamp = ctf_get_real_timestamp(
>  					&file_stream->parent,
>  					cur_index->ts_cycles.timestamp_end);
> +
> +			/*
> +			 * Duplicate the data from the previous index, because
> +			 * the one we just received is only a beacon with no
> +			 * relevant information except the timestamp_end. We
> +			 * don't need to keep this timestamp_end because we already
> +			 * updated the file_stream timestamps, so we only need
> +			 * to keep the last real index data as prev_index. That
> +			 * way, we keep the original prev timestamps and
> +			 * discarded events counter. This is the same behaviour
> +			 * as if we were reading a local trace, we would not
> +			 * have fake indexes between real indexes.
> +			 */
> +			memcpy(cur_index, prev_index, sizeof(struct packet_index));
>  		}
>  	} else {
> +	/* Real packet index */
>  		if (file_stream->parent.stream_class) {
>  			/* Convert the timestamps and append to the real_index. */
>  			cur_index->ts_real.timestamp_begin = ctf_get_real_timestamp(
> -- 
> 2.7.4
>
Jonathan Rajotte Jan. 9, 2018, 10:22 p.m. UTC | #2
On Tue, Jan 09, 2018 at 05:12:54PM -0500, Julien Desfossez wrote:
> When a stream is inactive, the consumer produces fake indexes which are
> beacons to let the viewer know a stream has not produced any data up to
> a certain timestamp. These beacon are actually real packet indexes with
> all the fields set to 0 except for the timestamp_end. Currently we keep
> these beacons just like we keep real indexes. The problem is that when
> we switch packet, we compare the events_discarded field in the index we
> just received with the same field in the previous index. In the case
> where a stream has been inactive, we have received inactivity beacons,
> and set the discarded_event field to 0, so the difference with the next
> real index might be wrong.
> 
> In fact, since the inactivity beacons are only used to push the
> timestamp end of a stream, we don't need to keep them and we actually
> need to keep most of the data from the real previous index. So we now
> copy the entire prev_index into the cur_index when we receive an
> inactivity beacon. We could refactor the code to avoid performing the
> pointer swap of cur and prev indexes, but this implies a redesign of
> much of the packet switching code which would affect other code paths.
> 

Tested-by: Jonathan Rajotte-Julien <jonathan.rajotte-julien at efficios.com>

> Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
> ---
>  formats/lttng-live/lttng-live-comm.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/formats/lttng-live/lttng-live-comm.c b/formats/lttng-live/lttng-live-comm.c
> index 77bf34a..3ffa257 100644
> --- a/formats/lttng-live/lttng-live-comm.c
> +++ b/formats/lttng-live/lttng-live-comm.c
> @@ -1275,6 +1275,7 @@ retry:
>  		pos->offset = 0;
>  	}
>  
> +	/* Beacon packet index */
>  	if (cur_index->content_size == 0) {
>  		if (file_stream->parent.stream_class) {
>  			file_stream->parent.cycles_timestamp =
> @@ -1282,8 +1283,23 @@ retry:
>  			file_stream->parent.real_timestamp = ctf_get_real_timestamp(
>  					&file_stream->parent,
>  					cur_index->ts_cycles.timestamp_end);
> +
> +			/*
> +			 * Duplicate the data from the previous index, because
> +			 * the one we just received is only a beacon with no
> +			 * relevant information except the timestamp_end. We
> +			 * don't need to keep this timestamp_end because we already
> +			 * updated the file_stream timestamps, so we only need
> +			 * to keep the last real index data as prev_index. That
> +			 * way, we keep the original prev timestamps and
> +			 * discarded events counter. This is the same behaviour
> +			 * as if we were reading a local trace, we would not
> +			 * have fake indexes between real indexes.
> +			 */
> +			memcpy(cur_index, prev_index, sizeof(struct packet_index));
>  		}
>  	} else {
> +	/* Real packet index */
>  		if (file_stream->parent.stream_class) {
>  			/* Convert the timestamps and append to the real_index. */
>  			cur_index->ts_real.timestamp_begin = ctf_get_real_timestamp(
> -- 
> 2.7.4
>
Mathieu Desnoyers Jan. 9, 2018, 10:35 p.m. UTC | #3
----- On Jan 9, 2018, at 5:12 PM, Julien Desfossez jdesfossez at efficios.com wrote:

> When a stream is inactive, the consumer produces fake indexes which are
> beacons to let the viewer know a stream has not produced any data up to
> a certain timestamp. These beacon are actually real packet indexes with
> all the fields set to 0 except for the timestamp_end. Currently we keep
> these beacons just like we keep real indexes. The problem is that when
> we switch packet, we compare the events_discarded field in the index we
> just received with the same field in the previous index. In the case
> where a stream has been inactive, we have received inactivity beacons,
> and set the discarded_event field to 0, so the difference with the next
> real index might be wrong.
> 
> In fact, since the inactivity beacons are only used to push the
> timestamp end of a stream, we don't need to keep them and we actually
> need to keep most of the data from the real previous index. So we now
> copy the entire prev_index into the cur_index when we receive an
> inactivity beacon. We could refactor the code to avoid performing the
> pointer swap of cur and prev indexes, but this implies a redesign of
> much of the packet switching code which would affect other code paths.
> 
> Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>

Acked-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>

> ---
> formats/lttng-live/lttng-live-comm.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> 
> diff --git a/formats/lttng-live/lttng-live-comm.c
> b/formats/lttng-live/lttng-live-comm.c
> index 77bf34a..3ffa257 100644
> --- a/formats/lttng-live/lttng-live-comm.c
> +++ b/formats/lttng-live/lttng-live-comm.c
> @@ -1275,6 +1275,7 @@ retry:
> 		pos->offset = 0;
> 	}
> 
> +	/* Beacon packet index */
> 	if (cur_index->content_size == 0) {
> 		if (file_stream->parent.stream_class) {
> 			file_stream->parent.cycles_timestamp =
> @@ -1282,8 +1283,23 @@ retry:
> 			file_stream->parent.real_timestamp = ctf_get_real_timestamp(
> 					&file_stream->parent,
> 					cur_index->ts_cycles.timestamp_end);
> +
> +			/*
> +			 * Duplicate the data from the previous index, because
> +			 * the one we just received is only a beacon with no
> +			 * relevant information except the timestamp_end. We
> +			 * don't need to keep this timestamp_end because we already
> +			 * updated the file_stream timestamps, so we only need
> +			 * to keep the last real index data as prev_index. That
> +			 * way, we keep the original prev timestamps and
> +			 * discarded events counter. This is the same behaviour
> +			 * as if we were reading a local trace, we would not
> +			 * have fake indexes between real indexes.
> +			 */
> +			memcpy(cur_index, prev_index, sizeof(struct packet_index));
> 		}
> 	} else {
> +	/* Real packet index */
> 		if (file_stream->parent.stream_class) {
> 			/* Convert the timestamps and append to the real_index. */
> 			cur_index->ts_real.timestamp_begin = ctf_get_real_timestamp(
> --
> 2.7.4
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Jérémie Galarneau Jan. 30, 2018, 7:17 p.m. UTC | #4
Merged in stable-1.5 and stable-1.4. Thanks!

Jérémie

On 9 January 2018 at 17:12, Julien Desfossez <jdesfossez at efficios.com> wrote:
> When a stream is inactive, the consumer produces fake indexes which are
> beacons to let the viewer know a stream has not produced any data up to
> a certain timestamp. These beacon are actually real packet indexes with
> all the fields set to 0 except for the timestamp_end. Currently we keep
> these beacons just like we keep real indexes. The problem is that when
> we switch packet, we compare the events_discarded field in the index we
> just received with the same field in the previous index. In the case
> where a stream has been inactive, we have received inactivity beacons,
> and set the discarded_event field to 0, so the difference with the next
> real index might be wrong.
>
> In fact, since the inactivity beacons are only used to push the
> timestamp end of a stream, we don't need to keep them and we actually
> need to keep most of the data from the real previous index. So we now
> copy the entire prev_index into the cur_index when we receive an
> inactivity beacon. We could refactor the code to avoid performing the
> pointer swap of cur and prev indexes, but this implies a redesign of
> much of the packet switching code which would affect other code paths.
>
> Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
> ---
>  formats/lttng-live/lttng-live-comm.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/formats/lttng-live/lttng-live-comm.c b/formats/lttng-live/lttng-live-comm.c
> index 77bf34a..3ffa257 100644
> --- a/formats/lttng-live/lttng-live-comm.c
> +++ b/formats/lttng-live/lttng-live-comm.c
> @@ -1275,6 +1275,7 @@ retry:
>                 pos->offset = 0;
>         }
>
> +       /* Beacon packet index */
>         if (cur_index->content_size == 0) {
>                 if (file_stream->parent.stream_class) {
>                         file_stream->parent.cycles_timestamp =
> @@ -1282,8 +1283,23 @@ retry:
>                         file_stream->parent.real_timestamp = ctf_get_real_timestamp(
>                                         &file_stream->parent,
>                                         cur_index->ts_cycles.timestamp_end);
> +
> +                       /*
> +                        * Duplicate the data from the previous index, because
> +                        * the one we just received is only a beacon with no
> +                        * relevant information except the timestamp_end. We
> +                        * don't need to keep this timestamp_end because we already
> +                        * updated the file_stream timestamps, so we only need
> +                        * to keep the last real index data as prev_index. That
> +                        * way, we keep the original prev timestamps and
> +                        * discarded events counter. This is the same behaviour
> +                        * as if we were reading a local trace, we would not
> +                        * have fake indexes between real indexes.
> +                        */
> +                       memcpy(cur_index, prev_index, sizeof(struct packet_index));
>                 }
>         } else {
> +       /* Real packet index */
>                 if (file_stream->parent.stream_class) {
>                         /* Convert the timestamps and append to the real_index. */
>                         cur_index->ts_real.timestamp_begin = ctf_get_real_timestamp(
> --
> 2.7.4
>
diff mbox series

Patch

diff --git a/formats/lttng-live/lttng-live-comm.c b/formats/lttng-live/lttng-live-comm.c
index 77bf34a..3ffa257 100644
--- a/formats/lttng-live/lttng-live-comm.c
+++ b/formats/lttng-live/lttng-live-comm.c
@@ -1275,6 +1275,7 @@  retry:
 		pos->offset = 0;
 	}
 
+	/* Beacon packet index */
 	if (cur_index->content_size == 0) {
 		if (file_stream->parent.stream_class) {
 			file_stream->parent.cycles_timestamp =
@@ -1282,8 +1283,23 @@  retry:
 			file_stream->parent.real_timestamp = ctf_get_real_timestamp(
 					&file_stream->parent,
 					cur_index->ts_cycles.timestamp_end);
+
+			/*
+			 * Duplicate the data from the previous index, because
+			 * the one we just received is only a beacon with no
+			 * relevant information except the timestamp_end. We
+			 * don't need to keep this timestamp_end because we already
+			 * updated the file_stream timestamps, so we only need
+			 * to keep the last real index data as prev_index. That
+			 * way, we keep the original prev timestamps and
+			 * discarded events counter. This is the same behaviour
+			 * as if we were reading a local trace, we would not
+			 * have fake indexes between real indexes.
+			 */
+			memcpy(cur_index, prev_index, sizeof(struct packet_index));
 		}
 	} else {
+	/* Real packet index */
 		if (file_stream->parent.stream_class) {
 			/* Convert the timestamps and append to the real_index. */
 			cur_index->ts_real.timestamp_begin = ctf_get_real_timestamp(