[PATCH v1] Misc fixes to xentrace, docs, and add code to support selective human CPU selection.

[PATCH v1] Misc fixes to xentrace, docs, and add code to support selective human CPU selection.

[Konrad Rzeszutek Wilk]

Hey George,

These patches add a bit of code to allow users of xentrace to narrow
down a specific CPU without having to figure out a bit mask. Also 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 adds an -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).
   
Thank you
 
 tools/libxc/xc_bitops.h   |   2 +
 tools/libxc/xc_tbuf.c     |  44 +++++++++-
 tools/libxc/xenctrl.h     |   1 +
 tools/xentrace/xentrace.8 |  26 +++++-
 tools/xentrace/xentrace.c | 205 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 257 insertions(+), 21 deletions(-)

Konrad Rzeszutek Wilk (5):
      docs: xentrace manpage
      libxc/trace: Add xc_tbuf_set_cpu_mask_array a variant of xc_tbuf_set_cpu_mask (v3)
      libxc/trace: Fix style
      xentrace: Use xc_cpumask_t when setting the cpu mask (v4)
      xentrace: Implement cpu mask range parsing of human values (-C).

[PATCH v1 1/5] docs: xentrace manpage

[Konrad Rzeszutek Wilk]

Update the -c and -e parameters wording.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v2: Update per Boris's comments]
---
 tools/xentrace/xentrace.8 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index c84e2e9..ac18e9f 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -37,10 +37,10 @@ set the time, p, (in milliseconds) to sleep between polling the buffers
 for new data.
 .TP
 .B -c, --cpu-mask=c
-set cpu-mask
+set bitmask of CPUs to trace. It is limited to 32-bits.
 .TP
 .B -e, --evt-mask=e
-set evt-mask
+set event capture mask. If not specified the TRC_ALL will be used.
 .TP
 .B -?, --help
 Give this help list
-- 
1.9.3

[PATCH v1 2/5] libxc/trace: Add xc_tbuf_set_cpu_mask_array a variant of xc_tbuf_set_cpu_mask (v3)

[Konrad Rzeszutek Wilk]

which uses an xc_cpumap_t instead of a uint32_t. This means
we can use an arbitrary bitmap without being limited to the
32-bits the xc_tbuf_set_cpu_mask_array can only do.

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

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v2: Use DIV_ROUND_UP macro as suggested by Daniel]
[v3: Use 'int' for bits instead of 'unsigned int' as spotted by Boris]
---
 tools/libxc/xc_bitops.h |  2 ++
 tools/libxc/xc_tbuf.c   | 40 ++++++++++++++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h   |  1 +
 3 files changed, 43 insertions(+)

diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h
index d8e0c16..b8cf2bd 100644
--- a/tools/libxc/xc_bitops.h
+++ b/tools/libxc/xc_bitops.h
@@ -12,6 +12,8 @@
 #define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG]
 #define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG)
 
+#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+
 /* calculate required space for number of longs needed to hold nr_bits */
 static inline int bitmap_size(int nr_bits)
 {
diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
index 4fb7bb1..efa8094 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -24,6 +24,7 @@
  */
 
 #include "xc_private.h"
+#include "xc_bitops.h"
 #include <xen/trace.h>
 
 static int tbuf_enable(xc_interface *xch, int enable)
@@ -143,6 +144,45 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
  out:
     return ret;
 }
+int xc_tbuf_set_cpu_mask_array(xc_interface *xch, xc_cpumap_t mask, int bits)
+{
+    DECLARE_SYSCTL;
+    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
+    int ret = -1;
+    int local_bits;
+
+    if ( bits <= 0 )
+        goto out;
+
+    local_bits = xc_get_max_cpus(xch);
+    if ( bits > local_bits )
+    {
+        PERROR("Wrong amount of bits supplied: %u, using %u\n", bits, local_bits);
+        bits = local_bits;
+    }
+    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, DIV_ROUND_UP(bits, 8));
+    if ( bytemap == NULL )
+    {
+        PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask_array hypercall");
+        goto out;
+    }
+
+    memcpy(bytemap, mask, DIV_ROUND_UP(bits, 8));
+
+    sysctl.cmd = XEN_SYSCTL_tbuf_op;
+    sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
+    sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_set_cpu_mask;
+
+    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
+    sysctl.u.tbuf_op.cpu_mask.nr_bits = bits;
+
+    ret = do_sysctl(xch, &sysctl);
+
+    xc_hypercall_buffer_free(xch, bytemap);
+
+ out:
+    return ret;
+}
 
 int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask)
 {
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 400f0df..df9eb46 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1472,6 +1472,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_array(xc_interface *xch, xc_cpumap_t mask, int bits);
 
 int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
 
-- 
1.9.3

[PATCH v1 3/5] libxc/trace: Fix style

[Konrad Rzeszutek Wilk]

Most of the functions follow the proper style, but these
two are the odd ones out.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_tbuf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
index efa8094..15ec146 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -33,7 +33,7 @@ static int tbuf_enable(xc_interface *xch, int enable)
 
     sysctl.cmd = XEN_SYSCTL_tbuf_op;
     sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
-    if (enable)
+    if ( enable )
         sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_enable;
     else
         sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_disable;
@@ -122,7 +122,7 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
     uint64_t mask64 = mask;
 
     bytemap = xc_hypercall_buffer_alloc(xch, bytemap, sizeof(mask64));
-    if (bytemap == NULL)
+    if ( bytemap == NULL )
     {
         PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask hypercall");
         goto out;
-- 
1.9.3

[PATCH v1 4/5] xentrace: Use xc_cpumask_t when setting the cpu mask (v4)

[Konrad Rzeszutek Wilk]

Instead of using an uint32_t. This requires us using the
xc_tbuf_set_cpu_mask_array API call that can handle variable
sized uint8_t arrays.

As this is just swapping over to use the new API, the existing
shortcomming when using -c is still present: it can only do up
to 32 CPUs.

But if '-c' has not been specified it will construct an CPU mask
for the full amount of physical CPUs.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v2: Changes per Boris's and Daniel's review]
[v3,v4: Change per Boris's review]
---
 tools/xentrace/xentrace.8 |  3 ++
 tools/xentrace/xentrace.c | 98 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 86 insertions(+), 15 deletions(-)

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..2063ae8 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -30,6 +30,7 @@
 #include <xen/trace.h>
 
 #include <xenctrl.h>
+#include "xc_bitops.h"
 
 #define PERROR(_m, _a...)                                       \
 do {                                                            \
@@ -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;
+    xc_cpumap_t cpu_mask;
     unsigned long tbuf_size;
     unsigned long disk_rsvd;
     unsigned long timeout;
@@ -521,23 +522,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 = DIV_ROUND_UP(bits, 8); i >= 0; i-- )
+    {
+        v = mask[i];
+        if ( v || had_printed ) {
+            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 < 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_array(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 +950,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 +998,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 'xc_handle' set yet. */
+            if ( parse_cpumask(optarg) )
+            {
+                perror("Not enough memory!");
+                exit(EXIT_FAILURE);
+            }
             break;
         
         case 'e': /* set new event mask for filtering*/
@@ -1002,7 +1068,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 +1084,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.9.3

[PATCH v1 5/5] 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.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/xentrace/xentrace.8 |  19 ++++++++
 tools/xentrace/xentrace.c | 109 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 125 insertions(+), 3 deletions(-)

diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index c176a96..ebfb47e 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -42,6 +42,25 @@ If not specified, the cpu-mask of all of the available CPUs will be
 constructed.
 
 .TP
+.B -C, --cpu-range="CPU-LIST"
+List of which CPUs to trace. By default it will pick all (all CPUs on the
+machine). A "CPU-LIST" 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.
+.TP
 .B -e, --evt-mask=e
 set event capture mask. If not specified the TRC_ALL will be used.
 .TP
diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index 2063ae8..378a714 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 {
     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;
@@ -819,6 +821,7 @@ static void usage(void)
 "Tool to capture Xen trace buffer data\n" \
 "\n" \
 "  -c, --cpu-mask=c        Set cpu-mask\n" \
+"  -C, --cpu-range=CPU-LIST Set cpu-mask using 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" \
@@ -967,6 +970,98 @@ static int parse_cpumask(const char *arg)
     return 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 exp_digit, in_range;
+
+    if ( !buflen )
+    {
+        fprintf(stderr, "Invalid option argument: %s\n", arg);
+        usage(); /* does exit */
+    }
+    nmaskbits = xc_get_max_cpus(xc_handle);
+    if ( nmaskbits <= 0 )
+    {
+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
+        usage();
+    }
+    map = xc_cpumap_alloc(xc_handle);
+    if ( !map )
+    {
+        fprintf(stderr, "Out of memory!\n");
+        usage();
+    }
+    c = c_old = totaldigits = 0;
+    do {
+        exp_digit = 1;
+        in_range = 0;
+        a = b = 0;
+        while ( buflen )
+        {
+            c = *arg++;
+            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 ( exp_digit || in_range )
+                        goto err_out;
+                b = 0;
+                in_range = 1;
+                exp_digit = 1;
+                continue;
+            }
+            if ( !isdigit(c) )
+            {
+                fprintf(stderr, "Only digits allowed in: %s\n", arg);
+                goto err_out;
+            }
+            b = b * 10 + (c - '0');
+            if ( !in_range )
+                a = b;
+            exp_digit = 0;
+            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);
+    usage();
+    return 0; /* Never reached */
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -975,6 +1070,7 @@ static void parse_args(int argc, char **argv)
         { "log-thresh",     required_argument, 0, 't' },
         { "poll-sleep",     required_argument, 0, 's' },
         { "cpu-mask",       required_argument, 0, 'c' },
+        { "cpu-range",      required_argument, 0, 'C' },
         { "evt-mask",       required_argument, 0, 'e' },
         { "trace-buf-size", required_argument, 0, 'S' },
         { "reserve-disk-space", required_argument, 0, 'r' },
@@ -988,7 +1084,7 @@ static void parse_args(int argc, char **argv)
         { 0, 0, 0, 0 }
     };
 
-    while ( (option = getopt_long(argc, argv, "t:s:c:e:S:r:T:M:DxX?V",
+    while ( (option = getopt_long(argc, argv, "t:s:c:C:e:S:r:T:M:DxX?V",
                     long_options, NULL)) != -1) 
     {
         switch ( option )
@@ -1005,7 +1101,9 @@ static void parse_args(int argc, char **argv)
                 exit(EXIT_FAILURE);
             }
             break;
-        
+        case 'C':
+            opts.cpu_mask_str = strdup(optarg);
+            break;
         case 'e': /* set new event mask for filtering*/
             parse_evtmask(optarg);
             break;
@@ -1069,6 +1167,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;
@@ -1086,7 +1185,11 @@ int main(int argc, char **argv)
     if ( opts.evt_mask != 0 )
         set_evt_mask(opts.evt_mask);
 
-
+    if ( opts.cpu_mask_str )
+    {
+        parse_cpumask_range(opts.cpu_mask_str);
+        free(opts.cpu_mask_str);
+    }
     set_cpu_mask(opts.cpu_mask);
     /* We don't use it pass this point. */
     free(opts.cpu_mask);
-- 
1.9.3

Re: [PATCH v1 1/5] docs: xentrace manpage

[George Dunlap]

On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
> Update the -c and -e parameters wording.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Not sure why we're updating the manpage only to update it again later, but:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

> [v2: Update per Boris's comments]
> ---
>   tools/xentrace/xentrace.8 | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
> index c84e2e9..ac18e9f 100644
> --- a/tools/xentrace/xentrace.8
> +++ b/tools/xentrace/xentrace.8
> @@ -37,10 +37,10 @@ set the time, p, (in milliseconds) to sleep between polling the buffers
>   for new data.
>   .TP
>   .B -c, --cpu-mask=c
> -set cpu-mask
> +set bitmask of CPUs to trace. It is limited to 32-bits.
>   .TP
>   .B -e, --evt-mask=e
> -set evt-mask
> +set event capture mask. If not specified the TRC_ALL will be used.
>   .TP
>   .B -?, --help
>   Give this help list
>

Re: [PATCH v1 2/5] libxc/trace: Add xc_tbuf_set_cpu_mask_array a variant of xc_tbuf_set_cpu_mask (v3)

[George Dunlap]

On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
> which uses an xc_cpumap_t instead of a uint32_t. This means
> we can use an arbitrary bitmap without being limited to the
> 32-bits the xc_tbuf_set_cpu_mask_array can only do.
>
> We also add an macro which can be used by both libxc and
> xentrace.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> [v2: Use DIV_ROUND_UP macro as suggested by Daniel]
> [v3: Use 'int' for bits instead of 'unsigned int' as spotted by Boris]


Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

> ---
>   tools/libxc/xc_bitops.h |  2 ++
>   tools/libxc/xc_tbuf.c   | 40 ++++++++++++++++++++++++++++++++++++++++
>   tools/libxc/xenctrl.h   |  1 +
>   3 files changed, 43 insertions(+)
>
> diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h
> index d8e0c16..b8cf2bd 100644
> --- a/tools/libxc/xc_bitops.h
> +++ b/tools/libxc/xc_bitops.h
> @@ -12,6 +12,8 @@
>   #define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG]
>   #define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG)
>
> +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> +
>   /* calculate required space for number of longs needed to hold nr_bits */
>   static inline int bitmap_size(int nr_bits)
>   {
> diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
> index 4fb7bb1..efa8094 100644
> --- a/tools/libxc/xc_tbuf.c
> +++ b/tools/libxc/xc_tbuf.c
> @@ -24,6 +24,7 @@
>    */
>
>   #include "xc_private.h"
> +#include "xc_bitops.h"
>   #include <xen/trace.h>
>
>   static int tbuf_enable(xc_interface *xch, int enable)
> @@ -143,6 +144,45 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
>    out:
>       return ret;
>   }
> +int xc_tbuf_set_cpu_mask_array(xc_interface *xch, xc_cpumap_t mask, int bits)
> +{
> +    DECLARE_SYSCTL;
> +    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
> +    int ret = -1;
> +    int local_bits;
> +
> +    if ( bits <= 0 )
> +        goto out;
> +
> +    local_bits = xc_get_max_cpus(xch);
> +    if ( bits > local_bits )
> +    {
> +        PERROR("Wrong amount of bits supplied: %u, using %u\n", bits, local_bits);
> +        bits = local_bits;
> +    }
> +    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, DIV_ROUND_UP(bits, 8));
> +    if ( bytemap == NULL )
> +    {
> +        PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask_array hypercall");
> +        goto out;
> +    }
> +
> +    memcpy(bytemap, mask, DIV_ROUND_UP(bits, 8));
> +
> +    sysctl.cmd = XEN_SYSCTL_tbuf_op;
> +    sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
> +    sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_set_cpu_mask;
> +
> +    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
> +    sysctl.u.tbuf_op.cpu_mask.nr_bits = bits;
> +
> +    ret = do_sysctl(xch, &sysctl);
> +
> +    xc_hypercall_buffer_free(xch, bytemap);
> +
> + out:
> +    return ret;
> +}
>
>   int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask)
>   {
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 400f0df..df9eb46 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1472,6 +1472,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_array(xc_interface *xch, xc_cpumap_t mask, int bits);
>
>   int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
>
>

Re: [PATCH v1 3/5] libxc/trace: Fix style

[George Dunlap]

On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
> Most of the functions follow the proper style, but these
> two are the odd ones out.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

> ---
>   tools/libxc/xc_tbuf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
> index efa8094..15ec146 100644
> --- a/tools/libxc/xc_tbuf.c
> +++ b/tools/libxc/xc_tbuf.c
> @@ -33,7 +33,7 @@ static int tbuf_enable(xc_interface *xch, int enable)
>
>       sysctl.cmd = XEN_SYSCTL_tbuf_op;
>       sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
> -    if (enable)
> +    if ( enable )
>           sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_enable;
>       else
>           sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_disable;
> @@ -122,7 +122,7 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask)
>       uint64_t mask64 = mask;
>
>       bytemap = xc_hypercall_buffer_alloc(xch, bytemap, sizeof(mask64));
> -    if (bytemap == NULL)
> +    if ( bytemap == NULL )
>       {
>           PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask hypercall");
>           goto out;
>

Re: [PATCH v1 2/5] libxc/trace: Add xc_tbuf_set_cpu_mask_array a variant of xc_tbuf_set_cpu_mask (v3)

[George Dunlap]

On 06/04/2014 05:45 PM, George Dunlap wrote:
> On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
>> which uses an xc_cpumap_t instead of a uint32_t. This means
>> we can use an arbitrary bitmap without being limited to the
>> 32-bits the xc_tbuf_set_cpu_mask_array can only do.
>>
>> We also add an macro which can be used by both libxc and
>> xentrace.
>>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> [v2: Use DIV_ROUND_UP macro as suggested by Daniel]
>> [v3: Use 'int' for bits instead of 'unsigned int' as spotted by Boris]
>
>
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Oh, sorry -- I meant to say: from what I can tell, at the moment 
xentrace is the only user of xc_tbuf_set_cpu_mask().  The libxc 
interface isn't stable: why not just leave the name 
xc_tbuf_set_cpu_mask() and just change the arguments?  (This would 
obviously involve merging patch 4 into this one as well.)

  -George

Re: [PATCH v1 4/5] xentrace: Use xc_cpumask_t when setting the cpu mask (v4)

[George Dunlap]

On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
> Instead of using an uint32_t. This requires us using the
> xc_tbuf_set_cpu_mask_array API call that can handle variable
> sized uint8_t arrays.
>
> As this is just swapping over to use the new API, the existing
> shortcomming when using -c is still present: it can only do up
> to 32 CPUs.
>
> But if '-c' has not been specified it will construct an CPU mask
> for the full amount of physical CPUs.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> [v2: Changes per Boris's and Daniel's review]
> [v3,v4: Change per Boris's review]

Looks good (saving the reviewed-by because I think this should probably 
be merged into patch 2).

  -George

> ---
>   tools/xentrace/xentrace.8 |  3 ++
>   tools/xentrace/xentrace.c | 98 +++++++++++++++++++++++++++++++++++++++--------
>   2 files changed, 86 insertions(+), 15 deletions(-)
>
> 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..2063ae8 100644
> --- a/tools/xentrace/xentrace.c
> +++ b/tools/xentrace/xentrace.c
> @@ -30,6 +30,7 @@
>   #include <xen/trace.h>
>
>   #include <xenctrl.h>
> +#include "xc_bitops.h"
>
>   #define PERROR(_m, _a...)                                       \
>   do {                                                            \
> @@ -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;
> +    xc_cpumap_t cpu_mask;
>       unsigned long tbuf_size;
>       unsigned long disk_rsvd;
>       unsigned long timeout;
> @@ -521,23 +522,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 = DIV_ROUND_UP(bits, 8); i >= 0; i-- )
> +    {
> +        v = mask[i];
> +        if ( v || had_printed ) {
> +            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 < 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_array(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 +950,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 +998,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 'xc_handle' set yet. */
> +            if ( parse_cpumask(optarg) )
> +            {
> +                perror("Not enough memory!");
> +                exit(EXIT_FAILURE);
> +            }
>               break;
>
>           case 'e': /* set new event mask for filtering*/
> @@ -1002,7 +1068,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 +1084,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);
>

Re: [PATCH v1 5/5] xentrace: Implement cpu mask range parsing of human values (-C).

[George Dunlap]

On 06/04/2014 02:44 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>

Would it be better, I wonder, to just try to overload the -c operator, 
special-casing "0x[hex]" to use the old interface?  Anyone who's 
currently using -c should almost certainly be using a hex string there, 
I should think -- using decimal would be pretty daft.

All it would take, I think, would be to check for bytes 0 and 1 being 
"0x", and if so, calling strtoul() rather than parse_cpumask_range().

[snip]
> @@ -967,6 +970,98 @@ static int parse_cpumask(const char *arg)
>       return 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 exp_digit, in_range;
> +
> +    if ( !buflen )
> +    {
> +        fprintf(stderr, "Invalid option argument: %s\n", arg);
> +        usage(); /* does exit */
> +    }
> +    nmaskbits = xc_get_max_cpus(xc_handle);
> +    if ( nmaskbits <= 0 )
> +    {
> +        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
> +        usage();
> +    }
> +    map = xc_cpumap_alloc(xc_handle);
> +    if ( !map )
> +    {
> +        fprintf(stderr, "Out of memory!\n");
> +        usage();
> +    }
> +    c = c_old = totaldigits = 0;
> +    do {
> +        exp_digit = 1;
> +        in_range = 0;
> +        a = b = 0;
> +        while ( buflen )
> +        {
> +            c = *arg++;
> +            buflen--;
> +
> +            if ( isspace(c) )
> +                continue;

Is it possible for this to have a space at the beginning?  Doesn't 
getopt() take care of that?

> +
> +            if ( totaldigits && c && isspace(c_old) )

c_old doesn't seem to be set anywhere after it's initialized above.

> +            {
> +                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 ( exp_digit || in_range )
> +                        goto err_out;

Isn't exp_digit a bit redundant, as if "in_range" is 1, "exp_digit" will 
also always be 1?

Everything else looks reasonable.

  -George

Re: [PATCH v1 2/5] libxc/trace: Add xc_tbuf_set_cpu_mask_array a variant of xc_tbuf_set_cpu_mask (v3)

[Ian Campbell]

On Wed, 2014-06-04 at 09:44 -0400, Konrad Rzeszutek Wilk wrote:
> which uses an xc_cpumap_t instead of a uint32_t. This means
> we can use an arbitrary bitmap without being limited to the
> 32-bits the xc_tbuf_set_cpu_mask_array can only do.

We do not guarantee API stability for libxc, so it is OK to either fix
the existing one or replace it. No need to keep the old one around
(unless perhaps calling applications fall into two sets each of whom
finds a different interface best).

> We also add an macro which can be used by both libxc and
> xentrace.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> [v2: Use DIV_ROUND_UP macro as suggested by Daniel]
> [v3: Use 'int' for bits instead of 'unsigned int' as spotted by Boris]
> ---
>  tools/libxc/xc_bitops.h |  2 ++
>  tools/libxc/xc_tbuf.c   | 40 ++++++++++++++++++++++++++++++++++++++++
>  tools/libxc/xenctrl.h   |  1 +
>  3 files changed, 43 insertions(+)
> 
> diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h
> index d8e0c16..b8cf2bd 100644
> --- a/tools/libxc/xc_bitops.h
> +++ b/tools/libxc/xc_bitops.h
> @@ -12,6 +12,8 @@
>  #define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG]
>  #define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG)
>  
> +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))

This isn't really a bitops.h thing, xc_private.h seems like the usual
dumping ground for stuff which doesn't fit elsewhere.

> +int xc_tbuf_set_cpu_mask_array(xc_interface *xch, xc_cpumap_t mask, int bits)
> +{
> +    DECLARE_SYSCTL;
> +    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
> +    int ret = -1;
> +    int local_bits;
> +
> +    if ( bits <= 0 )
> +        goto out;
> +
> +    local_bits = xc_get_max_cpus(xch);
> +    if ( bits > local_bits )
> +    {
> +        PERROR("Wrong amount of bits supplied: %u, using %u\n", bits, local_bits);

Should we not just return an error?

> +        bits = local_bits;
> +    }
> +    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, DIV_ROUND_UP(bits, 8));
> +    if ( bytemap == NULL )
> +    {
> +        PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask_array hypercall");
> +        goto out;
> +    }
> +
> +    memcpy(bytemap, mask, DIV_ROUND_UP(bits, 8));

Take a look at Dario's "libxc: get and set soft and hard affinity"[0]
for how to do this using the hypercall bounce buffer interface.

[0] <1401237770-7003-6-git-send-email-dario.faggioli@citrix.com>

Ian.

> +
> +    sysctl.cmd = XEN_SYSCTL_tbuf_op;
> +    sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
> +    sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_set_cpu_mask;
> +
> +    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
> +    sysctl.u.tbuf_op.cpu_mask.nr_bits = bits;
> +
> +    ret = do_sysctl(xch, &sysctl);
> +
> +    xc_hypercall_buffer_free(xch, bytemap);
> +
> + out:
> +    return ret;
> +}
>  
>  int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask)
>  {

Re: [PATCH v1 4/5] xentrace: Use xc_cpumask_t when setting the cpu mask (v4)

[Ian Campbell]

On Wed, 2014-06-04 at 18:01 +0100, George Dunlap wrote:
> On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
> > Instead of using an uint32_t. This requires us using the
> > xc_tbuf_set_cpu_mask_array API call that can handle variable
> > sized uint8_t arrays.
> >
> > As this is just swapping over to use the new API, the existing
> > shortcomming when using -c is still present: it can only do up
> > to 32 CPUs.
> >
> > But if '-c' has not been specified it will construct an CPU mask
> > for the full amount of physical CPUs.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > [v2: Changes per Boris's and Daniel's review]
> > [v3,v4: Change per Boris's review]
> 
> Looks good (saving the reviewed-by because

> I think this should probably be merged into patch 2).

I agree.

Re: [PATCH v1 1/5] docs: xentrace manpage

[Ian Campbell]

On Wed, 2014-06-04 at 17:25 +0100, George Dunlap wrote:
> On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
> > Update the -c and -e parameters wording.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Not sure why we're updating the manpage only to update it again later, but:
> 
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

I've applied this and #3. The rest had comments made.

> > [v2: Update per Boris's comments]

Please put these things after the --- break so they don't end up in the
final commit log.

Re: [PATCH v1 2/5] libxc/trace: Add xc_tbuf_set_cpu_mask_array a variant of xc_tbuf_set_cpu_mask (v3)

[Konrad Rzeszutek Wilk]

On Wed, Jun 04, 2014 at 05:52:27PM +0100, George Dunlap wrote:
> On 06/04/2014 05:45 PM, George Dunlap wrote:
> >On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
> >>which uses an xc_cpumap_t instead of a uint32_t. This means
> >>we can use an arbitrary bitmap without being limited to the
> >>32-bits the xc_tbuf_set_cpu_mask_array can only do.
> >>
> >>We also add an macro which can be used by both libxc and
> >>xentrace.
> >>
> >>Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>[v2: Use DIV_ROUND_UP macro as suggested by Daniel]
> >>[v3: Use 'int' for bits instead of 'unsigned int' as spotted by Boris]
> >
> >
> >Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> Oh, sorry -- I meant to say: from what I can tell, at the moment xentrace is
> the only user of xc_tbuf_set_cpu_mask().  The libxc interface isn't stable:
> why not just leave the name xc_tbuf_set_cpu_mask() and just change the
> arguments?  (This would obviously involve merging patch 4 into this one as
> well.)

Done.
> 
>  -George
> 

Re: [PATCH v1 2/5] libxc/trace: Add xc_tbuf_set_cpu_mask_array a variant of xc_tbuf_set_cpu_mask (v3)

[Konrad Rzeszutek Wilk]

On Thu, Jun 05, 2014 at 01:49:35PM +0100, Ian Campbell wrote:
> On Wed, 2014-06-04 at 09:44 -0400, Konrad Rzeszutek Wilk wrote:
> > which uses an xc_cpumap_t instead of a uint32_t. This means
> > we can use an arbitrary bitmap without being limited to the
> > 32-bits the xc_tbuf_set_cpu_mask_array can only do.
> 
> We do not guarantee API stability for libxc, so it is OK to either fix
> the existing one or replace it. No need to keep the old one around
> (unless perhaps calling applications fall into two sets each of whom
> finds a different interface best).
> 
> > We also add an macro which can be used by both libxc and
> > xentrace.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > [v2: Use DIV_ROUND_UP macro as suggested by Daniel]
> > [v3: Use 'int' for bits instead of 'unsigned int' as spotted by Boris]
> > ---
> >  tools/libxc/xc_bitops.h |  2 ++
> >  tools/libxc/xc_tbuf.c   | 40 ++++++++++++++++++++++++++++++++++++++++
> >  tools/libxc/xenctrl.h   |  1 +
> >  3 files changed, 43 insertions(+)
> > 
> > diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h
> > index d8e0c16..b8cf2bd 100644
> > --- a/tools/libxc/xc_bitops.h
> > +++ b/tools/libxc/xc_bitops.h
> > @@ -12,6 +12,8 @@
> >  #define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG]
> >  #define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG)
> >  
> > +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> 
> This isn't really a bitops.h thing, xc_private.h seems like the usual
> dumping ground for stuff which doesn't fit elsewhere.
> 
> > +int xc_tbuf_set_cpu_mask_array(xc_interface *xch, xc_cpumap_t mask, int bits)
> > +{
> > +    DECLARE_SYSCTL;
> > +    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
> > +    int ret = -1;
> > +    int local_bits;
> > +
> > +    if ( bits <= 0 )
> > +        goto out;
> > +
> > +    local_bits = xc_get_max_cpus(xch);
> > +    if ( bits > local_bits )
> > +    {
> > +        PERROR("Wrong amount of bits supplied: %u, using %u\n", bits, local_bits);
> 
> Should we not just return an error?
> 
> > +        bits = local_bits;
> > +    }
> > +    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, DIV_ROUND_UP(bits, 8));
> > +    if ( bytemap == NULL )
> > +    {
> > +        PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask_array hypercall");
> > +        goto out;
> > +    }
> > +
> > +    memcpy(bytemap, mask, DIV_ROUND_UP(bits, 8));
> 
> Take a look at Dario's "libxc: get and set soft and hard affinity"[0]
> for how to do this using the hypercall bounce buffer interface.
> 
> [0] <1401237770-7003-6-git-send-email-dario.faggioli@citrix.com>

Since George asked me to merge two patches (libxc + xentrace) and also
throw out the old xc_tbuf_set_cpu_mask, would you be OK if this was
a seperate commit? Too many things going in the patch already.

> 
> Ian.
> 
> > +
> > +    sysctl.cmd = XEN_SYSCTL_tbuf_op;
> > +    sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
> > +    sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_set_cpu_mask;
> > +
> > +    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
> > +    sysctl.u.tbuf_op.cpu_mask.nr_bits = bits;
> > +
> > +    ret = do_sysctl(xch, &sysctl);
> > +
> > +    xc_hypercall_buffer_free(xch, bytemap);
> > +
> > + out:
> > +    return ret;
> > +}
> >  
> >  int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask)
> >  {
> 
> 

Re: [PATCH v1 5/5] xentrace: Implement cpu mask range parsing of human values (-C).

[Konrad Rzeszutek Wilk]

On Wed, Jun 04, 2014 at 06:18:55PM +0100, George Dunlap wrote:
> On 06/04/2014 02:44 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>
> 
> Would it be better, I wonder, to just try to overload the -c operator,
> special-casing "0x[hex]" to use the old interface?  Anyone who's currently
> using -c should almost certainly be using a hex string there, I should think
> -- using decimal would be pretty daft.
> 
> All it would take, I think, would be to check for bytes 0 and 1 being "0x",
> and if so, calling strtoul() rather than parse_cpumask_range().

Done.
> 
> [snip]
> >@@ -967,6 +970,98 @@ static int parse_cpumask(const char *arg)
> >      return 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 exp_digit, in_range;
> >+
> >+    if ( !buflen )
> >+    {
> >+        fprintf(stderr, "Invalid option argument: %s\n", arg);
> >+        usage(); /* does exit */
> >+    }
> >+    nmaskbits = xc_get_max_cpus(xc_handle);
> >+    if ( nmaskbits <= 0 )
> >+    {
> >+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
> >+        usage();
> >+    }
> >+    map = xc_cpumap_alloc(xc_handle);
> >+    if ( !map )
> >+    {
> >+        fprintf(stderr, "Out of memory!\n");
> >+        usage();
> >+    }
> >+    c = c_old = totaldigits = 0;
> >+    do {
> >+        exp_digit = 1;
> >+        in_range = 0;
> >+        a = b = 0;
> >+        while ( buflen )
> >+        {
> >+            c = *arg++;
> >+            buflen--;
> >+
> >+            if ( isspace(c) )
> >+                continue;
> 
> Is it possible for this to have a space at the beginning?  Doesn't getopt()
> take care of that?

Yes. If the user does something like this: -c "     0xff"

> 
> >+
> >+            if ( totaldigits && c && isspace(c_old) )
> 
> c_old doesn't seem to be set anywhere after it's initialized above.

Fixed
> 
> >+            {
> >+                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 ( exp_digit || in_range )
> >+                        goto err_out;
> 
> Isn't exp_digit a bit redundant, as if "in_range" is 1, "exp_digit" will
> also always be 1?

Fixed.
> 
> Everything else looks reasonable.

OK, posting a patch shortly
> 
>  -George
>