[PATCH v3] Support CPU-list parsing in xentrace.

[PATCH v3] Support CPU-list parsing in xentrace.

[Konrad Rzeszutek Wilk]

Hey George and Ian,

Since v2 (http://lists.xen.org/archives/html/xen-devel/2014-06/msg01835.html)
I've modified it per the reviews I've received. Hopefully this is the
last round of reviews :-)

The purpose of these patches is  to allow users of xentrace to narrow
down a specific CPU without having to figure out a bit mask. They
fix the limitation of the bit mask which is it can only do up to 32-bits
- which on large machines (say 120CPUs), you can't selectively trace anything
past 32CPUs.

The code expands the -c parameter where you can do -c <starting cpu>-<end cpu>
or -c <cpu1>,<cpu2> or a combination of them. This along with 'xl vcpu-list'
makes it extremely easy to trace a specific guest (if pinned).

You can still use the -c 0x<some hex value> option if you prefer.

 tools/libxc/xc_tbuf.c     |   26 ++++--
 tools/libxc/xenctrl.h     |    7 ++-
 tools/xentrace/xentrace.8 |   21 ++++-
 tools/xentrace/xentrace.c |  221 +++++++++++++++++++++++++++++++++++++++++----
 4 files changed, 244 insertions(+), 31 deletions(-)

Konrad Rzeszutek Wilk (2):
      libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t
      xentrace: Implement cpu mask range parsing of human values (-c).

[PATCH v3 1/2] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t

[Konrad Rzeszutek Wilk]

We replace the implementation of xc_tbuf_set_cpu_mask with
an xc_cpumap_t instead of a uint32_t. This means we can use an
arbitrary bitmap without being limited to the 32-bits as
previously we were. Furthermore since there is only one
user of xc_tbuf_set_cpu_mask we just replace it and
its user in one go.

We also add an macro which can be used by both libxc and
xentrace.

And update the man page to describe this behavior.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_tbuf.c     |   26 +++++++-----
 tools/libxc/xenctrl.h     |    7 +++-
 tools/xentrace/xentrace.8 |    3 +
 tools/xentrace/xentrace.c |   97 ++++++++++++++++++++++++++++++++++++++-------
 4 files changed, 107 insertions(+), 26 deletions(-)

diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
index 8777492..d54da8a 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -113,15 +113,23 @@ int xc_tbuf_disable(xc_interface *xch)
     return tbuf_enable(xch, 0);
 }
 
-int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
+int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask, int bits)
 {
     DECLARE_SYSCTL;
-    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
+    DECLARE_HYPERCALL_BOUNCE(mask, XC_DIV_ROUND_UP(bits, 8), XC_HYPERCALL_BUFFER_BOUNCE_IN);
     int ret = -1;
-    uint64_t mask64 = mask;
+    int local_bits;
 
-    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, sizeof(mask64));
-    if ( bytemap == NULL )
+    if ( bits <= 0 )
+        goto out;
+
+    local_bits = xc_get_max_cpus(xch);
+    if ( bits > local_bits )
+    {
+        PERROR("Wrong amount of bits supplied: %d > %d!\n", bits, local_bits);
+        goto out;
+    }
+    if ( xc_hypercall_bounce_pre(xch, mask) )
     {
         PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask hypercall");
         goto out;
@@ -131,14 +139,12 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
     sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
     sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_set_cpu_mask;
 
-    bitmap_64_to_byte(bytemap, &mask64, sizeof (mask64) * 8);
-
-    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
-    sysctl.u.tbuf_op.cpu_mask.nr_bits = sizeof(bytemap) * 8;
+    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, mask);
+    sysctl.u.tbuf_op.cpu_mask.nr_bits = bits;
 
     ret = do_sysctl(xch, &sysctl);
 
-    xc_hypercall_buffer_free(xch, bytemap);
+    xc_hypercall_bounce_post(xch, mask);
 
  out:
     return ret;
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index b55d857..b378312 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1449,6 +1449,11 @@ int xc_availheap(xc_interface *xch, int min_width, int max_width, int node,
  */
 
 /**
+ * Useful macro for converting byte arrays to bitmaps.
+ */
+#define XC_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+
+/**
  * xc_tbuf_enable - enable tracing buffers
  *
  * @parm xch a handle to an open hypervisor interface
@@ -1489,7 +1494,7 @@ int xc_tbuf_set_size(xc_interface *xch, unsigned long size);
  */
 int xc_tbuf_get_size(xc_interface *xch, unsigned long *size);
 
-int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask);
+int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask, int bits);
 
 int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
 
diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index ac18e9f..c176a96 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -38,6 +38,9 @@ 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.
+
 .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 8a38e32..ee1d021 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -52,7 +52,7 @@ typedef struct settings_st {
     char *outfile;
     unsigned long poll_sleep; /* milliseconds to sleep between polls */
     uint32_t evt_mask;
-    uint32_t cpu_mask;
+    xc_cpumap_t cpu_mask;
     unsigned long tbuf_size;
     unsigned long disk_rsvd;
     unsigned long timeout;
@@ -521,23 +521,66 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
     return &tbufs;
 }
 
+void print_cpu_mask(xc_cpumap_t mask, int bits)
+{
+    unsigned int v, had_printed = 0;
+    int i;
+
+    fprintf(stderr, "change cpumask to 0x");
+
+    for ( i = XC_DIV_ROUND_UP(bits, 8); i >= 0; i-- )
+    {
+        v = mask[i];
+        if ( v || had_printed || !i ) {
+            fprintf(stderr,"%x", v);
+            had_printed = 1;
+        }
+   }
+   fprintf(stderr, "\n");
+}
+
+static void set_cpu_mask(xc_cpumap_t mask)
+{
+    int bits, i, ret = 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. */
+    ret = xc_tbuf_set_cpu_mask(xc_handle, mask, bits);
+    if ( ret != 0 )
+        goto out;
+
+    print_cpu_mask(mask, bits);
+    return;
+out:
+    PERROR("Failure to get trace buffer pointer from Xen and set the new mask");
+    exit(EXIT_FAILURE);
+}
+
 /**
- * set_mask - set the cpu/event mask in HV
+ * set_mask - set the event mask in HV
  * @mask:           the new mask 
  * @type:           the new mask type,0-event mask, 1-cpu mask
  *
  */
-static void set_mask(uint32_t mask, int type)
+static void set_evt_mask(uint32_t mask)
 {
     int ret = 0;
 
-    if (type == 1) {
-        ret = xc_tbuf_set_cpu_mask(xc_handle, mask);
-        fprintf(stderr, "change cpumask to 0x%x\n", mask);
-    } else if (type == 0) {
-        ret = xc_tbuf_set_evt_mask(xc_handle, mask);
-        fprintf(stderr, "change evtmask to 0x%x\n", mask);
-    }
+    ret = xc_tbuf_set_evt_mask(xc_handle, mask);
+    fprintf(stderr, "change evtmask to 0x%x\n", mask);
 
     if ( ret != 0 )
     {
@@ -906,6 +949,23 @@ static int parse_evtmask(char *arg)
     return 0;
 }
 
+static int parse_cpumask(const char *arg)
+{
+    xc_cpumap_t map;
+    uint32_t v, i;
+
+    map = malloc(sizeof(uint32_t));
+    if ( !map )
+        return -ENOMEM;
+
+    v = argtol(arg, 0);
+    for ( i = 0; i < sizeof(uint32_t); i++ )
+        map[i] = (v >> (i * 8)) & 0xff;
+
+    opts.cpu_mask = map;
+    return 0;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -937,7 +997,12 @@ static void parse_args(int argc, char **argv)
             break;
 
         case 'c': /* set new cpu mask for filtering*/
-            opts.cpu_mask = argtol(optarg, 0);
+            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
+            if ( parse_cpumask(optarg) )
+            {
+                perror("Not enough memory!");
+                exit(EXIT_FAILURE);
+            }
             break;
         
         case 'e': /* set new event mask for filtering*/
@@ -1002,7 +1067,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 = NULL;
     opts.disk_rsvd = 0;
     opts.disable_tracing = 1;
     opts.start_disabled = 0;
@@ -1018,10 +1083,12 @@ int main(int argc, char **argv)
     }
 
     if ( opts.evt_mask != 0 )
-        set_mask(opts.evt_mask, 0);
+        set_evt_mask(opts.evt_mask);
+
 
-    if ( opts.cpu_mask != 0 )
-        set_mask(opts.cpu_mask, 1);
+    set_cpu_mask(opts.cpu_mask);
+    /* We don't use it pass this point. */
+    free(opts.cpu_mask);
 
     if ( opts.timeout != 0 ) 
         alarm(opts.timeout);
-- 
1.7.7.6

Re: [PATCH v3 1/2] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t

[George Dunlap]

On 06/20/2014 08:33 PM, Konrad Rzeszutek Wilk wrote:
> @@ -906,6 +949,23 @@ static int parse_evtmask(char *arg)
>       return 0;
>   }
>
> +static int parse_cpumask(const char *arg)
> +{
> +    xc_cpumap_t map;
> +    uint32_t v, i;
> +
> +    map = malloc(sizeof(uint32_t));
> +    if ( !map )
> +        return -ENOMEM;
> +
> +    v = argtol(arg, 0);
> +    for ( i = 0; i < sizeof(uint32_t); i++ )
> +        map[i] = (v >> (i * 8)) & 0xff;
> +
> +    opts.cpu_mask = map;

Sorry for not noticing this befori.  If I'm reading this right, if 
someone sets the cpumask as a hex, then opts.cpu_mask will point to an 
area of memory only 32 bits long.  However, up in set_cpu_mask(), it 
always calls xc_tbuf_set_cpu_mask() with bits equal to xc_get_max_cpus().

On systems with more than 32 logical cpus, won't this cause a buffer 
overrun?  Should you be calling xc_cpumap_alloc() here instead of malloc()?


> @@ -937,7 +997,12 @@ static void parse_args(int argc, char **argv)
>               break;
>
>           case 'c': /* set new cpu mask for filtering*/
> -            opts.cpu_mask = argtol(optarg, 0);
> +            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
> +            if ( parse_cpumask(optarg) )

Nit: You seem to be parsing it now. :-)

  -George

Re: [PATCH v3 1/2] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t

[Konrad Rzeszutek Wilk]

On Tue, Jun 24, 2014 at 03:35:53PM +0100, George Dunlap wrote:
> On 06/20/2014 08:33 PM, Konrad Rzeszutek Wilk wrote:
> >@@ -906,6 +949,23 @@ static int parse_evtmask(char *arg)
> >      return 0;
> >  }
> >
> >+static int parse_cpumask(const char *arg)
> >+{
> >+    xc_cpumap_t map;
> >+    uint32_t v, i;
> >+
> >+    map = malloc(sizeof(uint32_t));
> >+    if ( !map )
> >+        return -ENOMEM;
> >+
> >+    v = argtol(arg, 0);
> >+    for ( i = 0; i < sizeof(uint32_t); i++ )
> >+        map[i] = (v >> (i * 8)) & 0xff;
> >+
> >+    opts.cpu_mask = map;
> 
> Sorry for not noticing this befori.  If I'm reading this right, if someone
> sets the cpumask as a hex, then opts.cpu_mask will point to an area of
> memory only 32 bits long.  However, up in set_cpu_mask(), it always calls

Correct.
> xc_tbuf_set_cpu_mask() with bits equal to xc_get_max_cpus().

<duh!>
> 
> On systems with more than 32 logical cpus, won't this cause a buffer
> overrun?  Should you be calling xc_cpumap_alloc() here instead of malloc()?

Unfortunatly we cannot call 'xc_cpumap_alloc' as it requires an 'xch' which
we do not have at that time.

A better option might be to have set_cpu_mask require the bit size to
be passed in. And the parse_cpumask function can figure that out from the
value.

> 
> 
> >@@ -937,7 +997,12 @@ static void parse_args(int argc, char **argv)
> >              break;
> >
> >          case 'c': /* set new cpu mask for filtering*/
> >-            opts.cpu_mask = argtol(optarg, 0);
> >+            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
> >+            if ( parse_cpumask(optarg) )
> 
> Nit: You seem to be parsing it now. :-)

Should it be called something else? There is an 'parse_evtmask' so
it follows the naming?

Anyhow here is an updated version of this patch:

>From f9e02e9e85c18cbb7a7c33ec4b0335d5c80887be Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 20 Jun 2014 15:34:53 -0400
Subject: [PATCH] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask
 with xc_cpumap_t instead of uint32_t

We replace the implementation of xc_tbuf_set_cpu_mask with
an xc_cpumap_t instead of a uint32_t. This means we can use an
arbitrary bitmap without being limited to the 32-bits as
previously we were. Furthermore since there is only one
user of xc_tbuf_set_cpu_mask we just replace it and
its user in one go.

We also add an macro which can be used by both libxc and
xentrace.

And update the man page to describe this behavior.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
[libxc pieces]
---
 tools/libxc/include/xenctrl.h |   7 ++-
 tools/libxc/xc_tbuf.c         |  26 +++++++----
 tools/xentrace/xentrace.8     |   3 ++
 tools/xentrace/xentrace.c     | 106 ++++++++++++++++++++++++++++++++++++------
 4 files changed, 116 insertions(+), 26 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 790db53..72ca33e 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1534,6 +1534,11 @@ int xc_availheap(xc_interface *xch, int min_width, int max_width, int node,
  */
 
 /**
+ * Useful macro for converting byte arrays to bitmaps.
+ */
+#define XC_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+
+/**
  * xc_tbuf_enable - enable tracing buffers
  *
  * @parm xch a handle to an open hypervisor interface
@@ -1574,7 +1579,7 @@ int xc_tbuf_set_size(xc_interface *xch, unsigned long size);
  */
 int xc_tbuf_get_size(xc_interface *xch, unsigned long *size);
 
-int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask);
+int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask, int bits);
 
 int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
 
diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
index 8777492c..d54da8a 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -113,15 +113,23 @@ int xc_tbuf_disable(xc_interface *xch)
     return tbuf_enable(xch, 0);
 }
 
-int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
+int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask, int bits)
 {
     DECLARE_SYSCTL;
-    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
+    DECLARE_HYPERCALL_BOUNCE(mask, XC_DIV_ROUND_UP(bits, 8), XC_HYPERCALL_BUFFER_BOUNCE_IN);
     int ret = -1;
-    uint64_t mask64 = mask;
+    int local_bits;
 
-    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, sizeof(mask64));
-    if ( bytemap == NULL )
+    if ( bits <= 0 )
+        goto out;
+
+    local_bits = xc_get_max_cpus(xch);
+    if ( bits > local_bits )
+    {
+        PERROR("Wrong amount of bits supplied: %d > %d!\n", bits, local_bits);
+        goto out;
+    }
+    if ( xc_hypercall_bounce_pre(xch, mask) )
     {
         PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask hypercall");
         goto out;
@@ -131,14 +139,12 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
     sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
     sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_set_cpu_mask;
 
-    bitmap_64_to_byte(bytemap, &mask64, sizeof (mask64) * 8);
-
-    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
-    sysctl.u.tbuf_op.cpu_mask.nr_bits = sizeof(bytemap) * 8;
+    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, mask);
+    sysctl.u.tbuf_op.cpu_mask.nr_bits = bits;
 
     ret = do_sysctl(xch, &sysctl);
 
-    xc_hypercall_buffer_free(xch, bytemap);
+    xc_hypercall_bounce_post(xch, mask);
 
  out:
     return ret;
diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index ac18e9f..c176a96 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -38,6 +38,9 @@ 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.
+
 .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 8a38e32..f515bd2 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -52,7 +52,8 @@ typedef struct settings_st {
     char *outfile;
     unsigned long poll_sleep; /* milliseconds to sleep between polls */
     uint32_t evt_mask;
-    uint32_t cpu_mask;
+    xc_cpumap_t cpu_mask;
+    int cpu_bits;
     unsigned long tbuf_size;
     unsigned long disk_rsvd;
     unsigned long timeout;
@@ -521,23 +522,68 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
     return &tbufs;
 }
 
+void print_cpu_mask(xc_cpumap_t mask, int bits)
+{
+    unsigned int v, had_printed = 0;
+    int i;
+
+    fprintf(stderr, "change cpumask to 0x");
+
+    for ( i = XC_DIV_ROUND_UP(bits, 8); i >= 0; i-- )
+    {
+        v = mask[i];
+        if ( v || had_printed || !i ) {
+            fprintf(stderr,"%x", v);
+            had_printed = 1;
+        }
+   }
+   fprintf(stderr, "\n");
+}
+
+static void set_cpu_mask(xc_cpumap_t mask, int bits)
+{
+    int i, ret = 0;
+
+    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. */
+    ret = xc_tbuf_set_cpu_mask(xc_handle, mask, bits);
+    if ( ret != 0 )
+        goto out;
+
+    print_cpu_mask(mask, bits);
+    return;
+out:
+    PERROR("Failure to get trace buffer pointer from Xen and set the new mask");
+    exit(EXIT_FAILURE);
+}
+
 /**
- * set_mask - set the cpu/event mask in HV
+ * set_mask - set the event mask in HV
  * @mask:           the new mask 
  * @type:           the new mask type,0-event mask, 1-cpu mask
  *
  */
-static void set_mask(uint32_t mask, int type)
+static void set_evt_mask(uint32_t mask)
 {
     int ret = 0;
 
-    if (type == 1) {
-        ret = xc_tbuf_set_cpu_mask(xc_handle, mask);
-        fprintf(stderr, "change cpumask to 0x%x\n", mask);
-    } else if (type == 0) {
-        ret = xc_tbuf_set_evt_mask(xc_handle, mask);
-        fprintf(stderr, "change evtmask to 0x%x\n", mask);
-    }
+    ret = xc_tbuf_set_evt_mask(xc_handle, mask);
+    fprintf(stderr, "change evtmask to 0x%x\n", mask);
 
     if ( ret != 0 )
     {
@@ -906,6 +952,28 @@ static int parse_evtmask(char *arg)
     return 0;
 }
 
+static int parse_cpumask(const char *arg)
+{
+    xc_cpumap_t map;
+    uint32_t v, i;
+    int bits = 0;
+
+    map = malloc(sizeof(uint32_t));
+    if ( !map )
+        return -ENOMEM;
+
+    v = argtol(arg, 0);
+    for ( i = 0; i < sizeof(uint32_t) ; i++ )
+        map[i] = (v >> (i * 8)) & 0xff;
+
+    for ( i = 0; v; v >>= 1)
+        bits += v & 1;
+
+    opts.cpu_mask = map;
+    opts.cpu_bits = bits;
+    return 0;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -937,7 +1005,12 @@ static void parse_args(int argc, char **argv)
             break;
 
         case 'c': /* set new cpu mask for filtering*/
-            opts.cpu_mask = argtol(optarg, 0);
+            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
+            if ( parse_cpumask(optarg) )
+            {
+                perror("Not enough memory!");
+                exit(EXIT_FAILURE);
+            }
             break;
         
         case 'e': /* set new event mask for filtering*/
@@ -1002,7 +1075,8 @@ 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 = NULL;
+    opts.cpu_bits = 0;
     opts.disk_rsvd = 0;
     opts.disable_tracing = 1;
     opts.start_disabled = 0;
@@ -1018,10 +1092,12 @@ int main(int argc, char **argv)
     }
 
     if ( opts.evt_mask != 0 )
-        set_mask(opts.evt_mask, 0);
+        set_evt_mask(opts.evt_mask);
+
 
-    if ( opts.cpu_mask != 0 )
-        set_mask(opts.cpu_mask, 1);
+    set_cpu_mask(opts.cpu_mask, opts.cpu_bits);
+    /* We don't use it pass this point. */
+    free(opts.cpu_mask);
 
     if ( opts.timeout != 0 ) 
         alarm(opts.timeout);
-- 
1.8.4.2

Re: [PATCH v3 1/2] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t

[Ian Campbell]

On Fri, 2014-06-20 at 15:33 -0400, Konrad Rzeszutek Wilk wrote:
> We replace the implementation of xc_tbuf_set_cpu_mask with
> an xc_cpumap_t instead of a uint32_t. This means we can use an
> arbitrary bitmap without being limited to the 32-bits as
> previously we were. Furthermore since there is only one
> user of xc_tbuf_set_cpu_mask we just replace it and
> its user in one go.
> 
> We also add an macro which can be used by both libxc and
> xentrace.
> 
> And update the man page to describe this behavior.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

For the libxc bits: Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/libxc/xc_tbuf.c     |   26 +++++++-----
>  tools/libxc/xenctrl.h     |    7 +++-
>  tools/xentrace/xentrace.8 |    3 +
>  tools/xentrace/xentrace.c |   97 ++++++++++++++++++++++++++++++++++++++-------
>  4 files changed, 107 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
> index 8777492..d54da8a 100644
> --- a/tools/libxc/xc_tbuf.c
> +++ b/tools/libxc/xc_tbuf.c
> @@ -113,15 +113,23 @@ int xc_tbuf_disable(xc_interface *xch)
>      return tbuf_enable(xch, 0);
>  }
>  
> -int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
> +int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask, int bits)
>  {
>      DECLARE_SYSCTL;
> -    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
> +    DECLARE_HYPERCALL_BOUNCE(mask, XC_DIV_ROUND_UP(bits, 8), XC_HYPERCALL_BUFFER_BOUNCE_IN);
>      int ret = -1;
> -    uint64_t mask64 = mask;
> +    int local_bits;
>  
> -    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, sizeof(mask64));
> -    if ( bytemap == NULL )
> +    if ( bits <= 0 )
> +        goto out;
> +
> +    local_bits = xc_get_max_cpus(xch);
> +    if ( bits > local_bits )
> +    {
> +        PERROR("Wrong amount of bits supplied: %d > %d!\n", bits, local_bits);
> +        goto out;
> +    }
> +    if ( xc_hypercall_bounce_pre(xch, mask) )
>      {
>          PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask hypercall");
>          goto out;
> @@ -131,14 +139,12 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
>      sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
>      sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_set_cpu_mask;
>  
> -    bitmap_64_to_byte(bytemap, &mask64, sizeof (mask64) * 8);
> -
> -    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
> -    sysctl.u.tbuf_op.cpu_mask.nr_bits = sizeof(bytemap) * 8;
> +    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, mask);
> +    sysctl.u.tbuf_op.cpu_mask.nr_bits = bits;
>  
>      ret = do_sysctl(xch, &sysctl);
>  
> -    xc_hypercall_buffer_free(xch, bytemap);
> +    xc_hypercall_bounce_post(xch, mask);
>  
>   out:
>      return ret;
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index b55d857..b378312 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1449,6 +1449,11 @@ int xc_availheap(xc_interface *xch, int min_width, int max_width, int node,
>   */
>  
>  /**
> + * Useful macro for converting byte arrays to bitmaps.
> + */
> +#define XC_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> +
> +/**
>   * xc_tbuf_enable - enable tracing buffers
>   *
>   * @parm xch a handle to an open hypervisor interface
> @@ -1489,7 +1494,7 @@ int xc_tbuf_set_size(xc_interface *xch, unsigned long size);
>   */
>  int xc_tbuf_get_size(xc_interface *xch, unsigned long *size);
>  
> -int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask);
> +int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask, int bits);
>  
>  int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
>  
> diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
> index ac18e9f..c176a96 100644
> --- a/tools/xentrace/xentrace.8
> +++ b/tools/xentrace/xentrace.8
> @@ -38,6 +38,9 @@ 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.
> +
>  .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 8a38e32..ee1d021 100644
> --- a/tools/xentrace/xentrace.c
> +++ b/tools/xentrace/xentrace.c
> @@ -52,7 +52,7 @@ typedef struct settings_st {
>      char *outfile;
>      unsigned long poll_sleep; /* milliseconds to sleep between polls */
>      uint32_t evt_mask;
> -    uint32_t cpu_mask;
> +    xc_cpumap_t cpu_mask;
>      unsigned long tbuf_size;
>      unsigned long disk_rsvd;
>      unsigned long timeout;
> @@ -521,23 +521,66 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
>      return &tbufs;
>  }
>  
> +void print_cpu_mask(xc_cpumap_t mask, int bits)
> +{
> +    unsigned int v, had_printed = 0;
> +    int i;
> +
> +    fprintf(stderr, "change cpumask to 0x");
> +
> +    for ( i = XC_DIV_ROUND_UP(bits, 8); i >= 0; i-- )
> +    {
> +        v = mask[i];
> +        if ( v || had_printed || !i ) {
> +            fprintf(stderr,"%x", v);
> +            had_printed = 1;
> +        }
> +   }
> +   fprintf(stderr, "\n");
> +}
> +
> +static void set_cpu_mask(xc_cpumap_t mask)
> +{
> +    int bits, i, ret = 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. */
> +    ret = xc_tbuf_set_cpu_mask(xc_handle, mask, bits);
> +    if ( ret != 0 )
> +        goto out;
> +
> +    print_cpu_mask(mask, bits);
> +    return;
> +out:
> +    PERROR("Failure to get trace buffer pointer from Xen and set the new mask");
> +    exit(EXIT_FAILURE);
> +}
> +
>  /**
> - * set_mask - set the cpu/event mask in HV
> + * set_mask - set the event mask in HV
>   * @mask:           the new mask 
>   * @type:           the new mask type,0-event mask, 1-cpu mask
>   *
>   */
> -static void set_mask(uint32_t mask, int type)
> +static void set_evt_mask(uint32_t mask)
>  {
>      int ret = 0;
>  
> -    if (type == 1) {
> -        ret = xc_tbuf_set_cpu_mask(xc_handle, mask);
> -        fprintf(stderr, "change cpumask to 0x%x\n", mask);
> -    } else if (type == 0) {
> -        ret = xc_tbuf_set_evt_mask(xc_handle, mask);
> -        fprintf(stderr, "change evtmask to 0x%x\n", mask);
> -    }
> +    ret = xc_tbuf_set_evt_mask(xc_handle, mask);
> +    fprintf(stderr, "change evtmask to 0x%x\n", mask);
>  
>      if ( ret != 0 )
>      {
> @@ -906,6 +949,23 @@ static int parse_evtmask(char *arg)
>      return 0;
>  }
>  
> +static int parse_cpumask(const char *arg)
> +{
> +    xc_cpumap_t map;
> +    uint32_t v, i;
> +
> +    map = malloc(sizeof(uint32_t));
> +    if ( !map )
> +        return -ENOMEM;
> +
> +    v = argtol(arg, 0);
> +    for ( i = 0; i < sizeof(uint32_t); i++ )
> +        map[i] = (v >> (i * 8)) & 0xff;
> +
> +    opts.cpu_mask = map;
> +    return 0;
> +}
> +
>  /* parse command line arguments */
>  static void parse_args(int argc, char **argv)
>  {
> @@ -937,7 +997,12 @@ static void parse_args(int argc, char **argv)
>              break;
>  
>          case 'c': /* set new cpu mask for filtering*/
> -            opts.cpu_mask = argtol(optarg, 0);
> +            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
> +            if ( parse_cpumask(optarg) )
> +            {
> +                perror("Not enough memory!");
> +                exit(EXIT_FAILURE);
> +            }
>              break;
>          
>          case 'e': /* set new event mask for filtering*/
> @@ -1002,7 +1067,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 = NULL;
>      opts.disk_rsvd = 0;
>      opts.disable_tracing = 1;
>      opts.start_disabled = 0;
> @@ -1018,10 +1083,12 @@ int main(int argc, char **argv)
>      }
>  
>      if ( opts.evt_mask != 0 )
> -        set_mask(opts.evt_mask, 0);
> +        set_evt_mask(opts.evt_mask);
> +
>  
> -    if ( opts.cpu_mask != 0 )
> -        set_mask(opts.cpu_mask, 1);
> +    set_cpu_mask(opts.cpu_mask);
> +    /* We don't use it pass this point. */
> +    free(opts.cpu_mask);
>  
>      if ( opts.timeout != 0 ) 
>          alarm(opts.timeout);

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

[Konrad Rzeszutek Wilk]

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 |   22 ++++++-
 tools/xentrace/xentrace.c |  144 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 149 insertions(+), 17 deletions(-)

diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index c176a96..7604597 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -36,10 +36,24 @@ 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.
+.RE
+.Sp
 
 .TP
 .B -e, --evt-mask=e
diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index ee1d021..ba58693 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>
 
@@ -30,6 +31,7 @@
 #include <xen/trace.h>
 
 #include <xenctrl.h>
+#include "xc_bitops.h" /* for set_bit */
 
 #define PERROR(_m, _a...)                                       \
 do {                                                            \
@@ -53,6 +55,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;
@@ -817,7 +820,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" \
@@ -966,6 +969,129 @@ 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);
+        return EINVAL;
+    }
+    nmaskbits = xc_get_max_cpus(xc_handle);
+    if ( nmaskbits <= 0 )
+    {
+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
+        return -ENOSPC;
+    }
+    map = xc_cpumap_alloc(xc_handle);
+    if ( !map )
+    {
+        fprintf(stderr, "Out of memory!\n");
+        return -ENOMEM;
+    }
+    c = c_old = totaldigits = 0;
+    s = arg;
+    do {
+        in_range = 0;
+        a = b = 0;
+        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++;
+        }
+        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 )
+        {
+            set_bit(a, (unsigned long *) map);
+            a++;
+        }
+    } while ( buflen && c == ',' );
+
+    opts.cpu_mask = map;
+    return 0;
+ err_out:
+    free(map);
+    return -EINVAL;
+}
+
+/**
+ * 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 = 0;
+    int buflen;
+
+    if ( opts.cpu_mask_str )
+    {
+        buflen = strlen(opts.cpu_mask_str);
+        if ( buflen < 2 )
+            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);
+    }
+    if ( !ret )
+        set_cpu_mask(opts.cpu_mask);
+ out:
+    /* We don't use it pass this point. */
+    free(opts.cpu_mask_str);
+    if ( ret )
+        usage(); /* Does not return. */
+    return ret;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -996,15 +1122,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;
@@ -1068,6 +1188,7 @@ int main(int argc, char **argv)
     opts.poll_sleep = POLL_SLEEP_MILLIS;
     opts.evt_mask = 0;
     opts.cpu_mask = NULL;
+    opts.cpu_mask_str = NULL;
     opts.disk_rsvd = 0;
     opts.disable_tracing = 1;
     opts.start_disabled = 0;
@@ -1085,10 +1206,7 @@ int main(int argc, char **argv)
     if ( opts.evt_mask != 0 )
         set_evt_mask(opts.evt_mask);
 
-
-    set_cpu_mask(opts.cpu_mask);
-    /* We don't use it pass this point. */
-    free(opts.cpu_mask);
+    figure_cpu_mask();
 
     if ( opts.timeout != 0 ) 
         alarm(opts.timeout);
-- 
1.7.7.6

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

[George Dunlap]

On 06/20/2014 08:33 PM, Konrad Rzeszutek Wilk wrote:
> @@ -966,6 +969,129 @@ 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);
> +        return EINVAL;
> +    }
> +    nmaskbits = xc_get_max_cpus(xc_handle);
> +    if ( nmaskbits <= 0 )
> +    {
> +        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
> +        return -ENOSPC;
> +    }
> +    map = xc_cpumap_alloc(xc_handle);
> +    if ( !map )
> +    {
> +        fprintf(stderr, "Out of memory!\n");
> +        return -ENOMEM;
> +    }
> +    c = c_old = totaldigits = 0;
> +    s = arg;
> +    do {
> +        in_range = 0;
> +        a = b = 0;
> +        while ( buflen )
> +        {
> +            c_old = c;
> +            c = *s++;
> +            buflen--;

Hmm, tricksy -- "buflen" may be non-zero above, but then may end up zero 
in the "} while()" below.  This caused me a bit of confusion -- might be 
worth a comment?

> +
> +            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;

This would appear to accept both of the following:
  -c -5 # equivalent to 0-5
  -c 2,-6 # equivalent to 2, 0-6

Is that what you want?

If not, maybe in_range needs to be tristate: RANGE_INIT, RANGE_ONE, 
RANGE_TWO.

Alternately, you might consider accepting both "-N" as meaning "0-N", 
and "N-" as meaning "N-MAX_CPUS".  I think you could do that by adding 
"if(b==0) { b=nmaskbits-1; }" just after the inner loop.

The rest of the parsing here looks correct to me.

> +/**
> + * 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 = 0;
> +    int buflen;
> +
> +    if ( opts.cpu_mask_str )
> +    {
> +        buflen = strlen(opts.cpu_mask_str);
> +        if ( buflen < 2 )

Isn't "-c 1" (i.e., just pinning to a single cpu) a valid option?

> +            goto out;
> +
> +        if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )

I think it should be safe to just to strcmp(), if one of your arguments 
is a static string, shouldn't it?  It's not that big a deal to me either 
way, though.

> +            ret = parse_cpumask(opts.cpu_mask_str);
> +        else
> +            ret = parse_cpumask_range(opts.cpu_mask_str);
> +    }
> +    if ( !ret )
> +        set_cpu_mask(opts.cpu_mask);

Having the "automatically set all bits" feature in set_cpu_mask() seems 
a bit confusing to me.  It also means that you have three distinct 
malloc() calls for the cpumask (one of which is wrong).

Would it make more sense to move the xc_cpumap_alloc() into this 
function, and then put the "automatically set all bits" as an else to 
the if(opts.cpu_mask_str) conditional?  That way you can see all the 
possibilities for what cpu_mask might end up in one place; and it 
automatically fixes the  (potential) bug with parse_cpumask() allocating 
a buffer that's too small.

Everything else looks good though.

  -George

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

[George Dunlap]

On Tue, Jun 24, 2014 at 6:17 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 06/20/2014 08:33 PM, Konrad Rzeszutek Wilk wrote:
>>
>> @@ -966,6 +969,129 @@ 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);
>> +        return EINVAL;
>> +    }
>> +    nmaskbits = xc_get_max_cpus(xc_handle);
>> +    if ( nmaskbits <= 0 )
>> +    {
>> +        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n",
>> nmaskbits);
>> +        return -ENOSPC;
>> +    }
>> +    map = xc_cpumap_alloc(xc_handle);
>> +    if ( !map )
>> +    {
>> +        fprintf(stderr, "Out of memory!\n");
>> +        return -ENOMEM;
>> +    }
>> +    c = c_old = totaldigits = 0;
>> +    s = arg;
>> +    do {
>> +        in_range = 0;
>> +        a = b = 0;
>> +        while ( buflen )
>> +        {
>> +            c_old = c;
>> +            c = *s++;
>> +            buflen--;
>
>
> Hmm, tricksy -- "buflen" may be non-zero above, but then may end up zero in
> the "} while()" below.  This caused me a bit of confusion -- might be worth
> a comment?
>
>
>> +
>> +            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;
>
>
> This would appear to accept both of the following:
>  -c -5 # equivalent to 0-5
>  -c 2,-6 # equivalent to 2, 0-6
>
> Is that what you want?
>
> If not, maybe in_range needs to be tristate: RANGE_INIT, RANGE_ONE,
> RANGE_TWO.
>
> Alternately, you might consider accepting both "-N" as meaning "0-N", and
> "N-" as meaning "N-MAX_CPUS".  I think you could do that by adding "if(b==0)
> { b=nmaskbits-1; }" just after the inner loop.
>
> The rest of the parsing here looks correct to me.
>
>
>> +/**
>> + * 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 = 0;
>> +    int buflen;
>> +
>> +    if ( opts.cpu_mask_str )
>> +    {
>> +        buflen = strlen(opts.cpu_mask_str);
>> +        if ( buflen < 2 )
>
>
> Isn't "-c 1" (i.e., just pinning to a single cpu) a valid option?
>
>
>> +            goto out;
>> +
>> +        if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )
>
>
> I think it should be safe to just to strcmp(), if one of your arguments is a
> static string, shouldn't it?  It's not that big a deal to me either way,
> though.

Dur... of course you only want to compare the first two characters,
not the null terminator.  N/m.

 -George