On Tue, Mar 31, 2015 at 12:31:34PM +0100, George Dunlap wrote: > On 03/24/2015 03:39 PM, Konrad Rzeszutek Wilk wrote: > > Instead of just using -c 0x<some hex value> we can > > also use -c <starting cpu>-<end cpu> or -c <cpu1>,<cpu2> > > or a combination of them. Also it can include just > > singular CPUs: -c <cpu1>, or ranges without an > > start or end (and xentrace will figure out the values), such > > as: -c -<cpu2> (which will include cpu0, cpu1, and cpu2) or > > -c <cpu2>- (which will include cpu2 and up to MAX_CPUS). > > > > That should make it easier to trace the right CPU if > > using this along with 'xl vcpu-list'. > > > > The code has been lifted from the Linux kernel, see file > > lib/bitmap.c, function __bitmap_parselist. > > > > To make the old behavior and the new function work, we check > > to see if the arguments have '0x' in them. If they do > > we use the old style parsing (limited to 32 CPUs). If that > > does not exist we use the new parsing. > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> > > [v4: Fix per George's review] > > --- > > tools/xentrace/xentrace.8 | 34 ++++++++- > > tools/xentrace/xentrace.c | 190 > > ++++++++++++++++++++++++++++++++++++++-------- > > 2 files changed, 188 insertions(+), 36 deletions(-) > > > > diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8 > > index c176a96..eb6fba8 100644 > > --- a/tools/xentrace/xentrace.8 > > +++ b/tools/xentrace/xentrace.8 > > @@ -36,10 +36,36 @@ all new records to the output > > set the time, p, (in milliseconds) to sleep between polling the buffers > > for new data. > > .TP > > -.B -c, --cpu-mask=c > > -set bitmask of CPUs to trace. It is limited to 32-bits. > > -If not specified, the cpu-mask of all of the available CPUs will be > > -constructed. > > +.B -c, --cpu-mask=[\fIc\fP|\fICPU-LIST\fP] > > +This can either be a hex value (of the form 0xNNNN...), or a set of cpu > > +ranges as described below. Hex values are limited to 32 bits. If not > > +specified, the cpu-mask of all of the available CPUs will be > > +constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which > > +may be specified as follows: > > + > > +.RS 4 > > +.ie n .IP """0-3""" 4 > > +.el .IP "``0-3''" 4 > > +.IX Item "0-3" > > +Trace only on CPUs 0 through 3 > > +.ie n .IP """0,2,5-7""" 4 > > +.el .IP "``0,2,5-7''" 4 > > +.IX Item "0,2,5-7" > > +Trace only on CPUs 0, 2, and 5 through 7. > > +.ie n .IP """-3""" 4 > > +.el .IP "``-3''" 4 > > +.IX Item "-3" > > +Trace only on CPUs 0 through 3 > > +.ie n .IP """-3,7""" 4 > > +.el .IP "``-3,7''" 4 > > +.IX Item "-3,7" > > +Trace only on CPUs 0 through 3 and 7 > > +.ie n .IP """3-""" 4 > > +.el .IP "``3-''" 4 > > +.IX Item "-3-" > > +Trace only on CPUs 3 up to maximum numbers of CPUs the host has. > > +.RE > > +.Sp > > > > .TP > > .B -e, --evt-mask=e > > diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c > > index 40504ec..3dd5c01 100644 > > --- a/tools/xentrace/xentrace.c > > +++ b/tools/xentrace/xentrace.c > > @@ -23,6 +23,7 @@ > > #include <string.h> > > #include <getopt.h> > > #include <assert.h> > > +#include <ctype.h> > > #include <sys/poll.h> > > #include <sys/statvfs.h> > > > > @@ -54,6 +55,7 @@ typedef struct settings_st { > > uint32_t evt_mask; > > xc_cpumap_t cpu_mask; > > int cpu_bits; > > + char *cpu_mask_str; > > unsigned long tbuf_size; > > unsigned long disk_rsvd; > > unsigned long timeout; > > @@ -545,25 +547,6 @@ void print_cpu_mask(xc_cpumap_t mask, int bits) > > > > static void set_cpu_mask(xc_cpumap_t mask, int bits) > > { > > - int i; > > - > > - if ( bits <= 0 ) > > - { > > - bits = xc_get_max_cpus(xc_handle); > > - if ( bits <= 0 ) > > - goto out; > > - } > > - if ( !mask ) > > - { > > - mask = xc_cpumap_alloc(xc_handle); > > - if ( !mask ) > > - goto out; > > - > > - /* Set it to include _all_ CPUs. */ > > - for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ ) > > - mask[i] = 0xff; > > - } > > - /* And this will limit it to the exact amount of bits. */ > > if ( xc_tbuf_set_cpu_mask(xc_handle, mask, bits) ) > > goto out; > > > > @@ -822,7 +805,7 @@ static void usage(void) > > "Usage: xentrace [OPTION...] [output file]\n" \ > > "Tool to capture Xen trace buffer data\n" \ > > "\n" \ > > -" -c, --cpu-mask=c Set cpu-mask\n" \ > > +" -c, --cpu-mask=c Set cpu-mask, using either hex or CPU ranges\n" > > \ > > " -e, --evt-mask=e Set evt-mask\n" \ > > " -s, --poll-sleep=p Set sleep time, p, in milliseconds between\n" \ > > " polling the trace buffer for new data\n" \ > > @@ -976,6 +959,156 @@ static int parse_cpumask(const char *arg) > > return 0; > > } > > > > +#define ZERO_DIGIT '0' > > + > > +static int parse_cpumask_range(const char *arg) > > +{ > > + xc_cpumap_t map; > > + unsigned int a, b, buflen = strlen(arg); > > + int c, c_old, totaldigits, nmaskbits; > > + int in_range; > > + const char *s; > > + > > + if ( !buflen ) > > + { > > + fprintf(stderr, "Invalid option argument: %s\n", arg); > > + errno = EINVAL; > > + return EXIT_FAILURE; > > + } > > I think we need blank lines between these if() statements > > > + nmaskbits = xc_get_max_cpus(xc_handle); > > [space] > > > + if ( nmaskbits <= 0 ) > > + { > > + fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", > > nmaskbits); > > + return EXIT_FAILURE; > > + } > > [space] > > > + map = xc_cpumap_alloc(xc_handle); > > [space] &c > > > + if ( !map ) > > + { > > + fprintf(stderr, "Out of memory!\n"); > > + return EXIT_FAILURE; > > + } > > + c = c_old = totaldigits = 0; > > + s = arg; > > + do { > > + in_range = 0; > > + a = b = 0; > > + /* The buflen can become zero in the '} while(..) below. */ > > + while ( buflen ) > > + { > > + c_old = c; > > + c = *s++; > > + buflen--; > > + > > + if ( isspace(c) ) > > + continue; > > + > > + if ( totaldigits && c && isspace(c_old) ) > > + { > > + fprintf(stderr, "No embedded whitespaces allowed in: > > %s\n", arg); > > + goto err_out; > > + } > > Why are we even bothering to handle spaces here? Who would put > xentrace -c " 0-3,5" /tmp/foo > and why would we want to accept that input?
This checks that and exits out. We would _not_ accept this input. Are you saying that before going in this function I should check that opts.cpu_mask_start starts with an space? But what if the string is: xentrace -c "0- 3,5". This check will catch it. Or should I have an check before this function that does: for ( i = 0; i < strlen(opts.cpu_mask_str); i++ ) if (isspace(opts.cpu_mask_str[i])) goto err_out; ? > > Not handling initial whitespace lets us get rid of totaldigits and c_old > as well. > > > + > > + /* A '\0' or a ',' signal the end of a cpu# or range */ > > + if ( c == '\0' || c == ',' ) > > + break; > > Can c=='\0' ever be true here? Isn't that why we do all the accounting Yes. > w/ buflen? At the start of the loop we decrease buflen by one which means it can get to zero - so on next iteration we will exit. However, if we do not have this check, then: 993 if ( !isdigit(c) ) 994 { 995 fprintf(stderr, "Only digits allowed in: %s\n", arg); 996 goto err_out; 997 } hit that and error out. We could move this check higher up, but would have to special case the ",", "-". And we would still have the conditionals on those characters. > > [snip] > > > + > > +/** > > + * Figure out which of the CPU types the user has provided - either the hex > > + * variant or the cpu-list. Once done set the CPU mask. > > + */ > > +static int figure_cpu_mask(void) > > +{ > > + int ret = EXIT_FAILURE; > > + > > + if ( opts.cpu_mask_str ) > > I think we should move this if into main(), similar to opts.evt_mask. Yes. > > > + { > > + if ( strlen(opts.cpu_mask_str) < 1 ) > > + { > > + errno = ENOSPC; > > + goto out; > > + } > > + if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 ) > > + ret = parse_cpumask(opts.cpu_mask_str); > > + else > > + ret = parse_cpumask_range(opts.cpu_mask_str); > > Would it make sense to add an "all" option here? Done. > > Also -- I think it might make sense to allocate the cpumask in this > function, and then have parse_cpumask* set the bits it wants. Then we > have a nice symmetric alloc and free. > > > + } > > + else > > And we can get rid of this else. > > > + { > > + int i, bits; > > + xc_cpumap_t mask; > > + > > + bits = xc_get_max_cpus(xc_handle); > > + if ( bits <= 0 ) > > + goto out; > > + > > + mask = xc_cpumap_alloc(xc_handle); > > + if ( !mask ) > > + goto out; > > + > > + /* Set it to include _all_ CPUs. */ > > + for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ ) > > + mask[i] = 0xff; > > + > > + opts.cpu_mask = mask; > > + opts.cpu_bits = bits; > > + ret = 0; > > + } > > + if ( ret != EXIT_FAILURE ) > > + set_cpu_mask(opts.cpu_mask, opts.cpu_bits); > > + out: > > + /* We don't use it pass this point. */ > > + free(opts.cpu_mask_str); > > I guess we also want to free opts.cpu_mask Aye. > > > + return ret; > > +} > > + > > /* parse command line arguments */ > > static void parse_args(int argc, char **argv) > > { > > @@ -1006,15 +1139,9 @@ static void parse_args(int argc, char **argv) > > opts.poll_sleep = argtol(optarg, 0); > > break; > > > > - case 'c': /* set new cpu mask for filtering*/ > > - /* Set opts.cpu_mask later as we don't have 'xch' set yet. */ > > - if ( parse_cpumask(optarg) ) > > - { > > - perror("Not enough memory!"); > > - exit(EXIT_FAILURE); > > - } > > + case 'c': /* set new cpu mask for filtering (when xch is set). */ > > + opts.cpu_mask_str = strdup(optarg); > > break; > > - > > case 'e': /* set new event mask for filtering*/ > > parse_evtmask(optarg); > > break; > > @@ -1079,6 +1206,7 @@ int main(int argc, char **argv) > > opts.evt_mask = 0; > > opts.cpu_mask = NULL; > > opts.cpu_bits = 0; > > + opts.cpu_mask_str = NULL; > > opts.disk_rsvd = 0; > > opts.disable_tracing = 1; > > opts.start_disabled = 0; > > @@ -1096,10 +1224,8 @@ int main(int argc, char **argv) > > if ( opts.evt_mask != 0 ) > > set_evt_mask(opts.evt_mask); > > > > - > > - set_cpu_mask(opts.cpu_mask, opts.cpu_bits); > > - /* We don't use it pass this point. */ > > - free(opts.cpu_mask); > > + if (figure_cpu_mask()) > > + usage(); /* calls exit. */ > > if (opts.cpu_mask_string) > figure_cpu_mask(); > Based on your previous patch this is what it became. I will be shortly testing it. >From c4a655e9645ee07af7f6186437ac5cd4316900bd Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> Date: Fri, 3 Apr 2015 14:45:57 -0400 Subject: [PATCH] xentrace: Implement cpu mask range parsing of human values (-c). Instead of just using -c 0x<some hex value> we can also use: -c <starting cpu>-<end cpu>, -c <cpu1>,<cpu2>, or a combination of them, or 'all' for all cpus. This new format can include just singular CPUs: -c <cpu1>, or ranges without an start or end (and xentrace will figure out the values), such as: -c -<cpu2> (which will include cpu0, cpu1, and cpu2) or -c <cpu2>- (which will include cpu2 and up to MAX_CPUS). That should make it easier to trace the right CPU if using this along with 'xl vcpu-list'. The code has been lifted from the Linux kernel, see file lib/bitmap.c, function __bitmap_parselist. To make the old behavior and the new function work, we check to see if the arguments have '0x' in them. If they do we use the old style parsing (limited to 32 CPUs). If that does not exist we use the new parsing. Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> --- [v4: Fix per George's review] [v5: Redo per George's review] --- tools/xentrace/xentrace.8 | 34 +++++++- tools/xentrace/xentrace.c | 196 ++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 195 insertions(+), 35 deletions(-) diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8 index ac18e9f..7b3172b 100644 --- a/tools/xentrace/xentrace.8 +++ b/tools/xentrace/xentrace.8 @@ -36,8 +36,38 @@ all new records to the output set the time, p, (in milliseconds) to sleep between polling the buffers for new data. .TP -.B -c, --cpu-mask=c -set bitmask of CPUs to trace. It is limited to 32-bits. +.B -c, --cpu-mask=[\fIc\fP|\fICPU-LIST\fP|\fIall\fP] +This can be: a hex value (of the form 0xNNNN...), or a set of cpu +ranges as described below, or the string \fIall\fP. Hex values are limited +to 32 bits. If not specified, the cpu-mask as set during bootup will be +constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which +may be specified as follows: + +.RS 4 +.ie n .IP """0-3""" 4 +.el .IP "``0-3''" 4 +.IX Item "0-3" +Trace only on CPUs 0 through 3 +.ie n .IP """0,2,5-7""" 4 +.el .IP "``0,2,5-7''" 4 +.IX Item "0,2,5-7" +Trace only on CPUs 0, 2, and 5 through 7. +.ie n .IP """-3""" 4 +.el .IP "``-3''" 4 +.IX Item "-3" +Trace only on CPUs 0 through 3 +.ie n .IP """-3,7""" 4 +.el .IP "``-3,7''" 4 +.IX Item "-3,7" +Trace only on CPUs 0 through 3 and 7 +.ie n .IP """3-""" 4 +.el .IP "``3-''" 4 +.IX Item "-3-" +Trace only on CPUs 3 up to maximum numbers of CPUs the host has. +.RE +.Sp + +If using \fIall\fP it will use all of the CPUs the host has. .TP .B -e, --evt-mask=e set event capture mask. If not specified the TRC_ALL will be used. diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c index 52278e3..c4d494d 100644 --- a/tools/xentrace/xentrace.c +++ b/tools/xentrace/xentrace.c @@ -23,6 +23,7 @@ #include <string.h> #include <getopt.h> #include <assert.h> +#include <ctype.h> #include <sys/poll.h> #include <sys/statvfs.h> @@ -52,7 +53,7 @@ typedef struct settings_st { char *outfile; unsigned long poll_sleep; /* milliseconds to sleep between polls */ uint32_t evt_mask; - uint32_t cpu_mask; + char *cpu_mask_str; unsigned long tbuf_size; unsigned long disk_rsvd; unsigned long timeout; @@ -542,33 +543,17 @@ void print_cpu_mask(xc_cpumap_t map) fprintf(stderr, "\n"); } -static void set_cpu_mask(uint32_t mask) +static int set_cpu_mask(xc_cpumap_t map) { - int i, ret = 0; - xc_cpumap_t map; - - map = xc_cpumap_alloc(xc_handle); - if ( !map ) - goto out; - - /* - * If mask is set, copy the bits out of it. This still works for - * systems with more than 32 cpus, as the shift will just shift - * mask down to zero. - */ - for ( i = 0; i < xc_get_cpumap_size(xc_handle) ; i++ ) - map[i] = (mask >> (i * 8)) & 0xff; - - ret = xc_tbuf_set_cpu_mask(xc_handle, map); - if ( ret != 0 ) - goto out; + int ret = xc_tbuf_set_cpu_mask(xc_handle, map); - print_cpu_mask(map); - free(map); - return; -out: + if ( ret == 0 ) + { + print_cpu_mask(map); + return 0; + } PERROR("Failure to get trace buffer pointer from Xen and set the new mask"); - exit(EXIT_FAILURE); + return EXIT_FAILURE; } /** @@ -819,7 +804,8 @@ static void usage(void) "Usage: xentrace [OPTION...] [output file]\n" \ "Tool to capture Xen trace buffer data\n" \ "\n" \ -" -c, --cpu-mask=c Set cpu-mask\n" \ +" -c, --cpu-mask=c Set cpu-mask, using either hex, CPU ranges, or\n" \ +" for all CPUs\n" \ " -e, --evt-mask=e Set evt-mask\n" \ " -s, --poll-sleep=p Set sleep time, p, in milliseconds between\n" \ " polling the trace buffer for new data\n" \ @@ -951,6 +937,149 @@ static int parse_evtmask(char *arg) return 0; } +#define ZERO_DIGIT '0' + +static int parse_cpumask_range(const char *arg, xc_cpumap_t map) +{ + unsigned int a, b, buflen = strlen(arg); + int c, c_old, totaldigits, nmaskbits; + int in_range; + const char *s; + + if ( !buflen ) + { + fprintf(stderr, "Invalid option argument: %s\n", arg); + errno = EINVAL; + return EXIT_FAILURE; + } + + nmaskbits = xc_get_max_cpus(xc_handle); + if ( nmaskbits <= 0 ) + { + fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits); + return EXIT_FAILURE; + } + + c = c_old = totaldigits = 0; + s = arg; + do { + in_range = 0; + a = b = 0; + /* The buflen can become zero in the '} while(..) below. */ + while ( buflen ) + { + c_old = c; + c = *s++; + buflen--; + + if ( isspace(c) ) + continue; + + if ( totaldigits && c && isspace(c_old) ) + { + fprintf(stderr, "No embedded whitespaces allowed in: %s\n", arg); + goto err_out; + } + + /* A '\0' or a ',' signal the end of a cpu# or range */ + if ( c == '\0' || c == ',' ) + break; + + if ( c == '-' ) + { + if ( in_range ) + goto err_out; + b = 0; + in_range = 1; + continue; + } + if ( !isdigit(c) ) + { + fprintf(stderr, "Only digits allowed in: %s\n", arg); + goto err_out; + } + b = b * 10 + (c - ZERO_DIGIT); + if ( !in_range ) + a = b; + totaldigits++; + } + /* Syntax: <digit>-[,] - expand to number of CPUs. */ + if ( b == 0 && in_range && (c == '-' || c == ',') ) + b = nmaskbits-1; + + if ( !(a <= b) ) + { + fprintf(stderr, "Wrong order of %d and %d\n", a, b); + goto err_out; + } + if ( b >= nmaskbits ) + { + fprintf(stderr, "Specified higher value then there are CPUS!\n"); + goto err_out; + } + while ( a <= b ) + { + xc_cpumap_setcpu(a, map); + a++; + } + } while ( buflen && c == ',' ); + + return 0; + err_out: + errno = EINVAL; + return EXIT_FAILURE; +} + +/** + * Figure out which of the CPU types the user has provided - either the hex + * variant, the cpu-list or 'all'. Once done set the CPU mask. + */ +static int figure_cpu_mask(void) +{ + int i, ret = EXIT_FAILURE; + xc_cpumap_t map; + + map = xc_cpumap_alloc(xc_handle); + if ( !map ) + goto out; + + if ( strlen(opts.cpu_mask_str) < 1 ) + { + errno = ENOSPC; + goto out; + } + + ret = 0; + if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 ) + { + uint32_t v; + + v = argtol(opts.cpu_mask_str, 0); + /* + * If mask is set, copy the bits out of it. This still works for + * systems with more than 32 cpus, as the shift will just shift + * mask down to zero. + */ + for ( i = 0; i < sizeof(uint32_t); i++ ) + map[i] = (v >> (i * 8)) & 0xff; + } + else if ( strncmp("all", opts.cpu_mask_str, 3) == 0 ) + { + for ( i = 0; i < xc_get_cpumap_size(xc_handle); i++ ) + map[i] = 0xff; + } + else + ret = parse_cpumask_range(opts.cpu_mask_str, map); + + if ( !ret ) + ret = set_cpu_mask(map); + out: + /* We don't use them pass this point. */ + free(map); + free(opts.cpu_mask_str); + return ret; +} + /* parse command line arguments */ static void parse_args(int argc, char **argv) { @@ -981,10 +1110,9 @@ static void parse_args(int argc, char **argv) opts.poll_sleep = argtol(optarg, 0); break; - case 'c': /* set new cpu mask for filtering*/ - opts.cpu_mask = argtol(optarg, 0); + case 'c': /* set new cpu mask for filtering (when xch is set). */ + opts.cpu_mask_str = strdup(optarg); break; - case 'e': /* set new event mask for filtering*/ parse_evtmask(optarg); break; @@ -1047,7 +1175,7 @@ int main(int argc, char **argv) opts.outfile = 0; opts.poll_sleep = POLL_SLEEP_MILLIS; opts.evt_mask = 0; - opts.cpu_mask = 0; + opts.cpu_mask_str = NULL; opts.disk_rsvd = 0; opts.disable_tracing = 1; opts.start_disabled = 0; @@ -1065,9 +1193,11 @@ int main(int argc, char **argv) if ( opts.evt_mask != 0 ) set_evt_mask(opts.evt_mask); - - if ( opts.cpu_mask != 0 ) - set_cpu_mask(opts.cpu_mask); + if ( opts.cpu_mask_str ) + { + if ( figure_cpu_mask() ) + exit(EXIT_FAILURE); + } if ( opts.timeout != 0 ) alarm(opts.timeout); -- 2.1.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel