From mboxrd@z Thu Jan 1 00:00:00 1970
From: jonathan.rajotte-julien@efficios.com (Jonathan Rajotte-Julien)
Date: Fri, 26 May 2017 14:18:14 -0400
Subject: [lttng-dev] [PATCH lttng-tools 3/3 master,
2.10-rc] Introduce "--blocking-timeout" channel parameter
In-Reply-To: <1495815260-21060-3-git-send-email-mathieu.desnoyers@efficios.com>
References: <1495815260-21060-1-git-send-email-mathieu.desnoyers@efficios.com>
<1495815260-21060-3-git-send-email-mathieu.desnoyers@efficios.com>
Message-ID: <20170526181814.ibvsfehqcifzp7o2@psrcode-TP-X230>
Hi,
Comments bellow, only kept relevant part of the patch.
On Fri, May 26, 2017 at 06:14:20PM +0200, Mathieu Desnoyers wrote:
> diff --git a/src/bin/lttng-sessiond/channel.c b/src/bin/lttng-sessiond/channel.c
> index 8a88ecc..5aa0cd8 100644
> --- a/src/bin/lttng-sessiond/channel.c
> +++ b/src/bin/lttng-sessiond/channel.c
> @@ -72,6 +72,7 @@ struct lttng_channel *channel_new_default_attr(int dom,
> chan->attr.switch_timer_interval = DEFAULT_KERNEL_CHANNEL_SWITCH_TIMER;
> chan->attr.read_timer_interval = DEFAULT_KERNEL_CHANNEL_READ_TIMER;
> chan->attr.live_timer_interval = DEFAULT_KERNEL_CHANNEL_LIVE_TIMER;
> + extended_attr->blocking_timeout = DEFAULT_KERNEL_CHANNEL_BLOCKING_TIMEOUT;
> extended_attr->monitor_timer_interval =
> DEFAULT_KERNEL_CHANNEL_MONITOR_TIMER;
> break;
> @@ -97,6 +98,7 @@ common_ust:
> DEFAULT_UST_UID_CHANNEL_READ_TIMER;
> chan->attr.live_timer_interval =
> DEFAULT_UST_UID_CHANNEL_LIVE_TIMER;
> + extended_attr->blocking_timeout = DEFAULT_UST_UID_CHANNEL_BLOCKING_TIMEOUT;
Put on two lines.
> extended_attr->monitor_timer_interval =
> DEFAULT_UST_UID_CHANNEL_MONITOR_TIMER;
> break;
> @@ -111,6 +113,7 @@ common_ust:
> DEFAULT_UST_PID_CHANNEL_READ_TIMER;
> chan->attr.live_timer_interval =
> DEFAULT_UST_PID_CHANNEL_LIVE_TIMER;
Same here.
> + extended_attr->blocking_timeout = DEFAULT_UST_PID_CHANNEL_BLOCKING_TIMEOUT;
> extended_attr->monitor_timer_interval =
> DEFAULT_UST_PID_CHANNEL_MONITOR_TIMER;
> break;
> @@ -217,6 +220,15 @@ static int channel_validate(struct lttng_channel *attr)
> return 0;
> }
>
> diff --git a/src/bin/lttng-sessiond/lttng-ust-abi.h b/src/bin/lttng-sessiond/lttng-ust-abi.h
> index 972de0c..687eb0b 100644
> --- a/src/bin/lttng-sessiond/lttng-ust-abi.h
> +++ b/src/bin/lttng-sessiond/lttng-ust-abi.h
> @@ -181,7 +181,12 @@ struct lttng_ust_channel_attr {
> unsigned int switch_timer_interval; /* usec */
> unsigned int read_timer_interval; /* usec */
> enum lttng_ust_output output; /* splice, mmap */
> - char padding[LTTNG_UST_CHANNEL_ATTR_PADDING];
> + union {
> + struct {
> + int64_t blocking_timeout; /* Retry timeout (usec) */
> + } s;
Should "s" be a more meaningful identifier here?
> + char padding[LTTNG_UST_CHANNEL_ATTR_PADDING];
> + } u;
> } LTTNG_PACKED;
>
> #define LTTNG_UST_TRACEPOINT_ITER_PADDING 16
> diff --git a/src/bin/lttng-sessiond/lttng-ust-ctl.h b/src/bin/lttng-sessiond/lttng-ust-ctl.h
> index cba0e27..1d67e85 100644
> --- a/src/bin/lttng-sessiond/lttng-ust-ctl.h
> +++ b/src/bin/lttng-sessiond/lttng-ust-ctl.h
> @@ -53,6 +53,7 @@ struct ustctl_consumer_channel_attr {
> enum lttng_ust_output output; /* splice, mmap */
> uint32_t chan_id; /* channel ID */
> unsigned char uuid[LTTNG_UST_UUID_LEN]; /* Trace session unique ID */
> + int64_t blocking_timeout; /* Retry timeout (usec) */
Only one/two tab here for style consistency or fix the previous lines.
> } LTTNG_PACKED;
>
> /*
> diff --git a/src/common/config/session.xsd b/src/common/config/session.xsd
> index 550fea0..35e56ca 100644
> --- a/src/common/config/session.xsd
> +++ b/src/common/config/session.xsd
> @@ -43,6 +43,14 @@ elementFormDefault="qualified" version="2.8">
>
>
>
> +
> +
> +
> +
Please add a comment here regarding the restriction on 4294967296000. Which is
due to the (int32) milliseconds overflow check.
> +
> +
> +
> +
>
>
>
> @@ -186,6 +194,7 @@ elementFormDefault="qualified" version="2.8">
>
>
>
> +
>
>
>
> diff --git a/src/common/defaults.h b/src/common/defaults.h
> index e0d0d86..b810302 100644
> --- a/src/common/defaults.h
> +++ b/src/common/defaults.h
> @@ -221,6 +221,7 @@
> #define DEFAULT_KERNEL_CHANNEL_MONITOR_TIMER CONFIG_DEFAULT_KERNEL_CHANNEL_MONITOR_TIMER
> #define DEFAULT_KERNEL_CHANNEL_READ_TIMER CONFIG_DEFAULT_KERNEL_CHANNEL_READ_TIMER
> #define DEFAULT_KERNEL_CHANNEL_LIVE_TIMER CONFIG_DEFAULT_KERNEL_CHANNEL_LIVE_TIMER
> +#define DEFAULT_KERNEL_CHANNEL_BLOCKING_TIMEOUT CONFIG_DEFAULT_KERNEL_CHANNEL_BLOCKING_TIMEOUT
>
> /* User space defaults */
>
> @@ -244,6 +245,9 @@
> #define DEFAULT_UST_PID_CHANNEL_READ_TIMER CONFIG_DEFAULT_UST_PID_CHANNEL_READ_TIMER
> #define DEFAULT_UST_UID_CHANNEL_READ_TIMER CONFIG_DEFAULT_UST_UID_CHANNEL_READ_TIMER
>
> +#define DEFAULT_UST_PID_CHANNEL_BLOCKING_TIMEOUT CONFIG_DEFAULT_UST_PID_CHANNEL_BLOCKING_TIMEOUT
> +#define DEFAULT_UST_UID_CHANNEL_BLOCKING_TIMEOUT CONFIG_DEFAULT_UST_UID_CHANNEL_BLOCKING_TIMEOUT
> +
> /*
> * Default timeout value for the sem_timedwait() call. Blocking forever is not
> * wanted so a timeout is used to control the data flow and not freeze the
> diff --git a/src/common/mi-lttng-3.0.xsd b/src/common/mi-lttng-3.0.xsd
> index 56b65a3..c5614f8 100644
> --- a/src/common/mi-lttng-3.0.xsd
> +++ b/src/common/mi-lttng-3.0.xsd
> @@ -43,6 +43,14 @@ THE SOFTWARE.
>
>
>
> +
> +
> +
> +
Same comment regarding the max value restriction found in session.xsd .
> +
> +
> +
> +
>
>
>
> @@ -363,6 +371,7 @@ THE SOFTWARE.
>
>
>
> +
>
>
>
--
Jonathan Rajotte-Julien
EfficiOS