[lttng-dev] UST lttng enable-event output improvement

Ettore Del Negro ettore at ettoredelnegro.me
Fri May 11 22:32:18 EDT 2012


https://bugs.lttng.org/issues/228
https://bugs.lttng.org/issues/235

The attached patch should do it.


Ettore

On 10/5/2012 4:46 PM, David Goulet wrote:
> Hey,
> 
> We have right now an opened bug about this exact issue. We will soon fix this
> and, as you said, help considerably the user experience with subbuffer size :)
> 
> Thanks for the report!
> 
> Cheers
> David
> 
> On 09/05/12 08:51 PM, Ettore Del Negro wrote:
>> Hello, i have a simple question which may lead to a small user experience
>> improvement:
> 
>>> lttng create tsession
>> Session tsession created. Traces will be written in
>> .../lttng-traces/tsession-20120510-015819
>>> lttng enable-channel tchan -u -s tsession --subbuf-size=1024
>> UST channel tchan enabled for session tsession
> 
>> a message stating that buffer size is set to a value different than default
>> would be nice
> 
>>> lttng enable-channel tchan -u -s tsession --subbuf-size=1022
>> UST channel tchan enabled for session tsession
> 
>> What happens? Is it set to a value not power of 2? Is it set to the default
>> value? (i guess the latter). The help says: --subbuf-size SIZE   Subbuffer
>> size in bytes (default: 4096, kernel default: 262144) Needs to be a power
>> of 2 for kernel and ust tracers
> 
>> Thanks, Ettore
> 
>> _______________________________________________ lttng-dev mailing list 
>> lttng-dev at lists.lttng.org 
>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 
> 
-------------- next part --------------
diff --git a/include/lttng/lttng.h b/include/lttng/lttng.h
index f621fa8..7d83119 100644
--- a/include/lttng/lttng.h
+++ b/include/lttng/lttng.h
@@ -236,7 +236,7 @@ struct lttng_event {
 #define LTTNG_CHANNEL_ATTR_PADDING1        LTTNG_SYMBOL_NAME_LEN + 32
 struct lttng_channel_attr {
 	int overwrite;                      /* 1: overwrite, 0: discard */
-	uint64_t subbuf_size;               /* bytes */
+	uint64_t subbuf_size;               /* bytes, power of 2 */
 	uint64_t num_subbuf;                /* power of 2 */
 	unsigned int switch_timer_interval; /* usec */
 	unsigned int read_timer_interval;   /* usec */
diff --git a/src/bin/lttng/commands/enable_channels.c b/src/bin/lttng/commands/enable_channels.c
index 9b5f8d6..4d3eb15 100644
--- a/src/bin/lttng/commands/enable_channels.c
+++ b/src/bin/lttng/commands/enable_channels.c
@@ -68,8 +68,8 @@ static struct poptOption long_options[] = {
 #endif
 	{"discard",        0,   POPT_ARG_NONE, 0, OPT_DISCARD, 0, 0},
 	{"overwrite",      0,   POPT_ARG_NONE, 0, OPT_OVERWRITE, 0, 0},
-	{"subbuf-size",    0,   POPT_ARG_DOUBLE, 0, OPT_SUBBUF_SIZE, 0, 0},
-	{"num-subbuf",     0,   POPT_ARG_INT, 0, OPT_NUM_SUBBUF, 0, 0},
+	{"subbuf-size",    0,   POPT_ARG_STRING, 0, OPT_SUBBUF_SIZE, 0, 0},
+	{"num-subbuf",     0,   POPT_ARG_STRING, 0, OPT_NUM_SUBBUF, 0, 0},
 	{"switch-timer",   0,   POPT_ARG_INT, 0, OPT_SWITCH_TIMER, 0, 0},
 	{"read-timer",     0,   POPT_ARG_INT, 0, OPT_READ_TIMER, 0, 0},
 	{"list-options",   0, POPT_ARG_NONE, NULL, OPT_LIST_OPTIONS, NULL, NULL},
@@ -106,12 +106,14 @@ static void usage(FILE *ofp)
 		DEFAULT_CHANNEL_SUBBUF_SIZE,
 		DEFAULT_KERNEL_CHANNEL_SUBBUF_SIZE);
 	fprintf(ofp, "                               Needs to be a power of 2 for\n");
-        fprintf(ofp, "                               kernel and ust tracers\n");
+    fprintf(ofp, "                               kernel and ust tracers.\n");
+    fprintf(ofp, "                               Hex values allowed using 0x prefix\n");
 	fprintf(ofp, "      --num-subbuf NUM     Number of subbufers\n");
 	fprintf(ofp, "                               (default: %u)\n",
 		DEFAULT_CHANNEL_SUBBUF_NUM);
 	fprintf(ofp, "                               Needs to be a power of 2 for\n");
-        fprintf(ofp, "                               kernel and ust tracers\n");
+    fprintf(ofp, "                               kernel and ust tracers.\n");
+    fprintf(ofp, "                               Hex values allowed using 0x prefix\n");
 	fprintf(ofp, "      --switch-timer USEC  Switch timer interval in usec (default: %u)\n",
 		DEFAULT_CHANNEL_SWITCH_TIMER);
 	fprintf(ofp, "      --read-timer USEC    Read timer interval in usec (default: %u)\n",
@@ -245,6 +247,8 @@ int cmd_enable_channels(int argc, const char **argv)
 	int opt, ret = CMD_SUCCESS;
 	static poptContext pc;
 	char *session_name = NULL;
+	uint64_t lvalue;
+	char *argstr, *endptr;
 
 	init_channel_config();
 
@@ -265,13 +269,111 @@ int cmd_enable_channels(int argc, const char **argv)
 			DBG("Channel set to overwrite");
 			break;
 		case OPT_SUBBUF_SIZE:
-			/* TODO Replace atol with strtol and check for errors */
-			chan.attr.subbuf_size = atol(poptGetOptArg(pc));
-			DBG("Channel subbuf size set to %" PRIu64, chan.attr.subbuf_size);
+			argstr = poptGetOptArg(pc);
+			errno = 0;
+			lvalue = (uint64_t) strtoll(argstr, &endptr, 0); /* Allow hex or octal values */
+			if (errno != 0)
+			{
+				ERR("Invalid channel subbuf size: %s", strerror(errno));
+				usage(stderr);
+				ret = CMD_ERROR;
+				goto end;
+			}
+			else if (lvalue == 0 && argstr == endptr)
+			{
+				ERR("Invalid channel subbuf size: not a number");
+				usage(stderr);
+				ret = CMD_ERROR;
+				goto end;
+			}
+			/* Human friendly imput */
+			if (endptr != NULL && *endptr != '\0')
+			{
+				switch (*endptr) {
+					case 'k':
+					case 'K':
+						lvalue *= 1024;
+						break;
+					case 'm':
+					case 'M':
+						lvalue *= 1024*1024;
+						break;
+					case 'g':
+					case 'G':
+						lvalue *= 1024*1024*1024;
+						break;
+					case 't':
+					case 'T':
+						lvalue *= 1024*1024*1024*1024;
+						break;
+				}
+			}
+			/*
+			 * Not power of 2
+			 * http://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2
+			 */
+			if (!(lvalue && !(lvalue & (lvalue-1))))
+			{
+				/* Round up to power of 2 */
+				lvalue--;
+				lvalue |= lvalue >> 1; lvalue |= lvalue >> 2; lvalue |= lvalue >> 4;
+				lvalue |= lvalue >> 8; lvalue |= lvalue >> 16; lvalue |= lvalue >> 32;
+				lvalue++;
+				/* and tell the user about it */
+				MSG("Setting channel subbuf size to %"PRIu64" bytes", lvalue);
+			}
+			chan.attr.subbuf_size = lvalue;
+			DBG("Setting channel subbuf size to %" PRIu64, chan.attr.subbuf_size);
 			break;
 		case OPT_NUM_SUBBUF:
-			/* TODO Replace atoi with strtol and check for errors */
-			chan.attr.num_subbuf = atoi(poptGetOptArg(pc));
+			argstr = poptGetOptArg(pc);
+			errno = 0;
+			lvalue = (uint64_t) strtoll(argstr, &endptr, 0); /* Allow hex or octal values  */
+			if (errno != 0)
+			{
+				ERR("Invalid channel subbuf number: %s", strerror(errno));
+				usage(stderr);
+				ret = CMD_ERROR;
+				goto end;
+			}
+			else if (lvalue == 0 && argstr == endptr)
+			{
+				ERR("Invalid channel subbuf number: not a number");
+				usage(stderr);
+				ret = CMD_ERROR;
+				goto end;
+			}
+			if (endptr != NULL && *endptr != '\0')
+			{
+				switch (*endptr) {
+					case 'k':
+					case 'K':
+						lvalue *= 1024;
+						break;
+					case 'm':
+					case 'M':
+						lvalue *= 1024*1024;
+						break;
+					case 'g':
+					case 'G':
+						lvalue *= 1024*1024*1024;
+						break;
+					case 't':
+					case 'T':
+						lvalue *= 1024*1024*1024*1024;
+						break;
+				}
+			}
+			if (!(lvalue && !(lvalue & (lvalue-1))))
+			{
+				lvalue--;
+				lvalue |= lvalue >> 1; lvalue |= lvalue >> 2; lvalue |= lvalue >> 4;
+				lvalue |= lvalue >> 8; lvalue |= lvalue >> 16; lvalue |= lvalue >> 32;
+				lvalue++;
+				/* and tell the user about it */
+				MSG("Setting channel subbuf number to %" PRIu64, lvalue);
+			}
+			chan.attr.num_subbuf = lvalue;
 			DBG("Channel subbuf num set to %" PRIu64, chan.attr.num_subbuf);
 			break;
 		case OPT_SWITCH_TIMER:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 945 bytes
Desc: OpenPGP digital signature
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20120512/7d5af9d7/attachment-0001.pgp>


More information about the lttng-dev mailing list