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

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

On 06/13/2014 09:41 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.
>
> 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>
> ---
>   tools/xentrace/xentrace.8 |   21 +++++-
>   tools/xentrace/xentrace.c |  155 ++++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 165 insertions(+), 11 deletions(-)
>
> diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
> index c176a96..9beb72d 100644
> --- a/tools/xentrace/xentrace.8
> +++ b/tools/xentrace/xentrace.8
> @@ -36,11 +36,26 @@ 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]
> +set bitmask of CPUs to trace.
>   It is limited to 32-bits if using hex digits.
>   If not specified, the cpu-mask of all of the available CPUs will be

I think something like this would be better:

"This can be either 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..."

> -constructed.
> +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.
> +.RE
> +.Sp
> +
> +If this option is not specified, xentrace will trace all of the physical
> +CPUs on the machine.

This was already said above.  I'm not sure whether here or at the 
beginning is better, but it should only be said once.

>   .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 03b5537..45b0597 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>
>   
> @@ -31,6 +32,7 @@
>   
>   #include <xenctrl.h>
>   #include "xc_private.h"
> +#include "xc_bitops.h" /* for set_bit */
>   
>   /***** Compile time configuration of defaults ********************************/
>   
> @@ -45,6 +47,7 @@ typedef struct settings_st {
>       unsigned long poll_sleep; /* milliseconds to sleep between polls */
>       uint32_t evt_mask;
>       xc_cpumap_t cpu_mask;
> +    char *cpu_mask_str;
>       unsigned long tbuf_size;
>       unsigned long disk_rsvd;
>       unsigned long timeout;
> @@ -809,7 +812,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 hex and CPU ranges\n" \

"Set cpu-mask, using either hex or CPU ranges"

> +static int check_for_hex(const char *s)
> +{
> +    int buflen;
> +    int c_old, c;
> +
> +    buflen = strlen(s);
> +    c_old = c = 0;
> +    do {
> +            c = *s++;
> +            buflen--;
> +
> +            if ( isspace(c) && c_old != ZERO_DIGIT )
> +                continue;
> +
> +            if ( c == '\0' )
> +                return -1;
> +
> +            if ( c == ZERO_DIGIT )
> +                c_old = c;
> +
> +            if ( c == 'x' && c_old == ZERO_DIGIT )
> +                return 1;
> +
> +     } while ( buflen );

I'm a bit confused why you don't just do strcmp("0x", s) here.

> @@ -989,14 +1112,25 @@ static void parse_args(int argc, char **argv)
>               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);
> +                int ret = check_for_hex(optarg);
> +
> +                if ( ret < 0 )
> +                    usage();
> +
> +                if ( ret )
> +                    ret = parse_cpumask(optarg);
> +                else
> +                    /* Set opts.cpu_mask later as we don't have 'xch' set yet.*/
> +                    opts.cpu_mask_str = strdup(optarg);
> +
> +                if ( ret )
> +                {
> +                    perror("Not enough memory!");
> +                    exit(EXIT_FAILURE);
> +                }
> +                break;

While we're at it, is there any chance we could put all the parsing 
stuff in the same place?  If we can't call parse_cpumask_range here, I'd 
prefer to just do a strdup() always, and then do the check_for_hex() and 
parse_cpumask() or parse_cpumask_range() down below.

  -George