Re: [PATCH v3 3/3] xentrace: Implement cpu mask range parsing of human values (-c).

[George Dunlap <george.dunlap@eu.citrix.com>]

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@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?

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