|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 3/3] xentrace: Implement cpu mask range parsing of human values (-c).
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.wilk@xxxxxxxxxx>
> [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?
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
w/ buflen?
[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.
> + {
> + 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?
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
> + 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();
Thanks,
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |