diff mbox series

[lttng-tools,v2] Fix: wrong use of the relay_streams_sent in snapshot

Message ID 1503521333-4905-1-git-send-email-jdesfossez@efficios.com
State Accepted, archived
Delegated to: Jérémie Galarneau
Headers show
Series [lttng-tools,v2] Fix: wrong use of the relay_streams_sent in snapshot | expand

Commit Message

Julien Desfossez Aug. 23, 2017, 8:48 p.m. UTC
The relay_streams_sent message is only useful in live sessions and
should only be sent after all the streams of a channel have been sent.

Here we were sending this message every time we sent a stream to the
relay during a snapshot which makes no sense.

Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
---
 src/common/kernel-consumer/kernel-consumer.c | 8 --------
 src/common/ust-consumer/ust-consumer.c       | 6 ------
 2 files changed, 14 deletions(-)

Comments

Mathieu Desnoyers Aug. 24, 2017, 3:15 p.m. UTC | #1
If you remove this, I think the streams will never get published
within the relayd. (publish_connection_local_streams()). Is this
an expected side-effect ? It should be documented in the changelog.
My guess is that we indeed don't want to publish the snapshot
streams to the viewers.

The reason for doing this change should also be documented. What
behavior is unwanted here from a relayd perspective ?

Thanks,

Mathieu

----- On Aug 23, 2017, at 1:48 PM, Julien Desfossez jdesfossez at efficios.com wrote:

> The relay_streams_sent message is only useful in live sessions and
> should only be sent after all the streams of a channel have been sent.
> 
> Here we were sending this message every time we sent a stream to the
> relay during a snapshot which makes no sense.
> 
> Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
> ---
> src/common/kernel-consumer/kernel-consumer.c | 8 --------
> src/common/ust-consumer/ust-consumer.c       | 6 ------
> 2 files changed, 14 deletions(-)
> 
> diff --git a/src/common/kernel-consumer/kernel-consumer.c
> b/src/common/kernel-consumer/kernel-consumer.c
> index a5dcc66..1c2751b 100644
> --- a/src/common/kernel-consumer/kernel-consumer.c
> +++ b/src/common/kernel-consumer/kernel-consumer.c
> @@ -187,14 +187,6 @@ int lttng_kconsumer_snapshot_channel(uint64_t key, char
> *path,
> 			DBG("Kernel consumer snapshot stream %s/%s (%" PRIu64 ")",
> 					path, stream->name, stream->key);
> 		}
> -		if (relayd_id != -1ULL) {
> -			ret = consumer_send_relayd_streams_sent(relayd_id);
> -			if (ret < 0) {
> -				ERR("sending streams sent to relayd");
> -				goto end_unlock;
> -			}
> -			channel->streams_sent_to_relayd = true;
> -		}
> 
> 		ret = kernctl_buffer_flush_empty(stream->wait_fd);
> 		if (ret < 0) {
> diff --git a/src/common/ust-consumer/ust-consumer.c
> b/src/common/ust-consumer/ust-consumer.c
> index 366f855..bce7db8 100644
> --- a/src/common/ust-consumer/ust-consumer.c
> +++ b/src/common/ust-consumer/ust-consumer.c
> @@ -1101,12 +1101,6 @@ static int snapshot_channel(uint64_t key, char *path,
> uint64_t relayd_id,
> 			DBG("UST consumer snapshot stream %s/%s (%" PRIu64 ")", path,
> 					stream->name, stream->key);
> 		}
> -		if (relayd_id != -1ULL) {
> -			ret = consumer_send_relayd_streams_sent(relayd_id);
> -			if (ret < 0) {
> -				goto error_unlock;
> -			}
> -		}
> 
> 		/*
> 		 * If tracing is active, we want to perform a "full" buffer flush.
> --
> 2.7.4
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Julien Desfossez Aug. 24, 2017, 7:35 p.m. UTC | #2
> If you remove this, I think the streams will never get published
> within the relayd. (publish_connection_local_streams()). Is this
> an expected side-effect ? It should be documented in the changelog.
> My guess is that we indeed don't want to publish the snapshot
> streams to the viewers.
Indeed, a snapshot session cannot be a live session, so we don't
want/need to publish those streams. Also, sending this message every
time we send a stream is a wrong usage of the command.

> The reason for doing this change should also be documented. What
> behavior is unwanted here from a relayd perspective ?
We are sending this message to the relay (and waiting for the
confirmation) before taking the snapshot of each stream. So, in addition
to being wrong and useless, it adds a considerable delay before taking
the snapshot of each stream.

Do you agree ?

Thanks,

Julien
Mathieu Desnoyers Aug. 25, 2017, 4:47 p.m. UTC | #3
----- On Aug 24, 2017, at 12:35 PM, Julien Desfossez jdesfossez at efficios.com wrote:

>> If you remove this, I think the streams will never get published
>> within the relayd. (publish_connection_local_streams()). Is this
>> an expected side-effect ? It should be documented in the changelog.
>> My guess is that we indeed don't want to publish the snapshot
>> streams to the viewers.
> Indeed, a snapshot session cannot be a live session, so we don't
> want/need to publish those streams. Also, sending this message every
> time we send a stream is a wrong usage of the command.

You'll need to ensure that we are not freeing the streams too
quickly or leaking them in relayd after your change.

> 
>> The reason for doing this change should also be documented. What
>> behavior is unwanted here from a relayd perspective ?
> We are sending this message to the relay (and waiting for the
> confirmation) before taking the snapshot of each stream. So, in addition
> to being wrong and useless, it adds a considerable delay before taking
> the snapshot of each stream.
> 
> Do you agree ?

It looks like a good idea to remove it. I just want us to make sure the
snapshot mode does not somehow expect the streams to be published
within other parts of the relayd code.

Thanks,

Mathieu

> 
> Thanks,
> 
> Julien
Julien Desfossez Aug. 25, 2017, 7:47 p.m. UTC | #4
>>> If you remove this, I think the streams will never get published
>>> within the relayd. (publish_connection_local_streams()). Is this
>>> an expected side-effect ? It should be documented in the changelog.
>>> My guess is that we indeed don't want to publish the snapshot
>>> streams to the viewers.
>> Indeed, a snapshot session cannot be a live session, so we don't
>> want/need to publish those streams. Also, sending this message every
>> time we send a stream is a wrong usage of the command.
> 
> You'll need to ensure that we are not freeing the streams too
> quickly or leaking them in relayd after your change.

Publishing the stream does not take a new reference on the stream, it
just adds it in the stream_list so it becomes visible in the viewer
thread. So it does not change the lifetime of the stream. And if the
stream is not published when we remove it we don't try to unpublish it.

>>> The reason for doing this change should also be documented. What
>>> behavior is unwanted here from a relayd perspective ?
>> We are sending this message to the relay (and waiting for the
>> confirmation) before taking the snapshot of each stream. So, in addition
>> to being wrong and useless, it adds a considerable delay before taking
>> the snapshot of each stream.
>>
>> Do you agree ?
> 
> It looks like a good idea to remove it. I just want us to make sure the
> snapshot mode does not somehow expect the streams to be published
> within other parts of the relayd code.

It looks good.

Thanks,

Julien
Mathieu Desnoyers Aug. 26, 2017, 12:03 a.m. UTC | #5
----- On Aug 25, 2017, at 12:47 PM, Julien Desfossez jdesfossez at efficios.com wrote:

>>>> If you remove this, I think the streams will never get published
>>>> within the relayd. (publish_connection_local_streams()). Is this
>>>> an expected side-effect ? It should be documented in the changelog.
>>>> My guess is that we indeed don't want to publish the snapshot
>>>> streams to the viewers.
>>> Indeed, a snapshot session cannot be a live session, so we don't
>>> want/need to publish those streams. Also, sending this message every
>>> time we send a stream is a wrong usage of the command.
>> 
>> You'll need to ensure that we are not freeing the streams too
>> quickly or leaking them in relayd after your change.
> 
> Publishing the stream does not take a new reference on the stream, it
> just adds it in the stream_list so it becomes visible in the viewer
> thread. So it does not change the lifetime of the stream. And if the
> stream is not published when we remove it we don't try to unpublish it.
> 
>>>> The reason for doing this change should also be documented. What
>>>> behavior is unwanted here from a relayd perspective ?
>>> We are sending this message to the relay (and waiting for the
>>> confirmation) before taking the snapshot of each stream. So, in addition
>>> to being wrong and useless, it adds a considerable delay before taking
>>> the snapshot of each stream.
>>>
>>> Do you agree ?
>> 
>> It looks like a good idea to remove it. I just want us to make sure the
>> snapshot mode does not somehow expect the streams to be published
>> within other parts of the relayd code.
> 
> It looks good.

Allright then, thanks!

Mathieu

> 
> Thanks,
> 
> Julien
Jérémie Galarneau Nov. 14, 2017, 12:25 a.m. UTC | #6
Merged, thanks!

Jérémie

On 23 August 2017 at 16:48, Julien Desfossez <jdesfossez at efficios.com> wrote:
> The relay_streams_sent message is only useful in live sessions and
> should only be sent after all the streams of a channel have been sent.
>
> Here we were sending this message every time we sent a stream to the
> relay during a snapshot which makes no sense.
>
> Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
> ---
>  src/common/kernel-consumer/kernel-consumer.c | 8 --------
>  src/common/ust-consumer/ust-consumer.c       | 6 ------
>  2 files changed, 14 deletions(-)
>
> diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
> index a5dcc66..1c2751b 100644
> --- a/src/common/kernel-consumer/kernel-consumer.c
> +++ b/src/common/kernel-consumer/kernel-consumer.c
> @@ -187,14 +187,6 @@ int lttng_kconsumer_snapshot_channel(uint64_t key, char *path,
>                         DBG("Kernel consumer snapshot stream %s/%s (%" PRIu64 ")",
>                                         path, stream->name, stream->key);
>                 }
> -               if (relayd_id != -1ULL) {
> -                       ret = consumer_send_relayd_streams_sent(relayd_id);
> -                       if (ret < 0) {
> -                               ERR("sending streams sent to relayd");
> -                               goto end_unlock;
> -                       }
> -                       channel->streams_sent_to_relayd = true;
> -               }
>
>                 ret = kernctl_buffer_flush_empty(stream->wait_fd);
>                 if (ret < 0) {
> diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
> index 366f855..bce7db8 100644
> --- a/src/common/ust-consumer/ust-consumer.c
> +++ b/src/common/ust-consumer/ust-consumer.c
> @@ -1101,12 +1101,6 @@ static int snapshot_channel(uint64_t key, char *path, uint64_t relayd_id,
>                         DBG("UST consumer snapshot stream %s/%s (%" PRIu64 ")", path,
>                                         stream->name, stream->key);
>                 }
> -               if (relayd_id != -1ULL) {
> -                       ret = consumer_send_relayd_streams_sent(relayd_id);
> -                       if (ret < 0) {
> -                               goto error_unlock;
> -                       }
> -               }
>
>                 /*
>                  * If tracing is active, we want to perform a "full" buffer flush.
> --
> 2.7.4
>
diff mbox series

Patch

diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
index a5dcc66..1c2751b 100644
--- a/src/common/kernel-consumer/kernel-consumer.c
+++ b/src/common/kernel-consumer/kernel-consumer.c
@@ -187,14 +187,6 @@  int lttng_kconsumer_snapshot_channel(uint64_t key, char *path,
 			DBG("Kernel consumer snapshot stream %s/%s (%" PRIu64 ")",
 					path, stream->name, stream->key);
 		}
-		if (relayd_id != -1ULL) {
-			ret = consumer_send_relayd_streams_sent(relayd_id);
-			if (ret < 0) {
-				ERR("sending streams sent to relayd");
-				goto end_unlock;
-			}
-			channel->streams_sent_to_relayd = true;
-		}
 
 		ret = kernctl_buffer_flush_empty(stream->wait_fd);
 		if (ret < 0) {
diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
index 366f855..bce7db8 100644
--- a/src/common/ust-consumer/ust-consumer.c
+++ b/src/common/ust-consumer/ust-consumer.c
@@ -1101,12 +1101,6 @@  static int snapshot_channel(uint64_t key, char *path, uint64_t relayd_id,
 			DBG("UST consumer snapshot stream %s/%s (%" PRIu64 ")", path,
 					stream->name, stream->key);
 		}
-		if (relayd_id != -1ULL) {
-			ret = consumer_send_relayd_streams_sent(relayd_id);
-			if (ret < 0) {
-				goto error_unlock;
-			}
-		}
 
 		/*
 		 * If tracing is active, we want to perform a "full" buffer flush.