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

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

[Konrad Rzeszutek Wilk]

Hey George, Ian:

Since v1 posting (http://comments.gmane.org/gmane.comp.emulators.xen.devel/201962)
I've incorporated all the feedback I've received. If I missed
something - please remind me.

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.

Comments:

 [PATCH v2 1/4] xentrace: Use PERROR from xc_private. instead of our

is needed to use 'xc_private.h' instead of 'xc_bitops.h' in the xentrace.c
file. However, the

 [PATCH v2 2/4] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU

still needs the 'xc_bitops.h' for bit manipulations so the initial goal
of shuffling DIV_ROUND_UP to a different header had been meet - but we
still need to include the other header.

 [PATCH v2 3/4] libxc/tbuf: Use the xc_hypercall_bounce_[pre|post]

Can be squashed in the "2/4 libxc/xentrace: Replace xc_tbuf_set_cpu_mask
with CPU" if desired. However the 2/4 patch is busy enough that I was
a bit uncomfortable to squash it myself. Naturally if the maintainers
want it that way I will be thrilled to do it.

 tools/libxc/xc_private.h  |   2 +
 tools/libxc/xc_tbuf.c     |  28 +++--
 tools/libxc/xenctrl.h     |   2 +-
 tools/xentrace/xentrace.8 |  22 +++-
 tools/xentrace/xentrace.c | 266 ++++++++++++++++++++++++++++++++++++++++------
 5 files changed, 273 insertions(+), 47 deletions(-)

Konrad Rzeszutek Wilk (4):
      xentrace: Use PERROR from xc_private. instead of our own.
      libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t
      libxc/tbuf: Use the xc_hypercall_bounce_[pre|post] instead of memcpy
      xentrace: Implement cpu mask range parsing of human values (-c).

[PATCH v2 1/4] xentrace: Use PERROR from xc_private. instead of our own.

[Konrad Rzeszutek Wilk]

However, the xc_private PERROR uses 'xch' while we call said
structure 'xc_handle' - so this patch also changes the variable
to xch.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/xentrace/xentrace.c |   28 ++++++++++------------------
 1 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index 8a38e32..16034e3 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -30,15 +30,7 @@
 #include <xen/trace.h>
 
 #include <xenctrl.h>
-
-#define PERROR(_m, _a...)                                       \
-do {                                                            \
-    int __saved_errno = errno;                                  \
-    fprintf(stderr, "ERROR: " _m " (%d = %s)\n" , ## _a ,       \
-            __saved_errno, strerror(__saved_errno));            \
-    errno = __saved_errno;                                      \
-} while (0)
-
+#include "xc_private.h"
 
 /***** Compile time configuration of defaults ********************************/
 
@@ -72,7 +64,7 @@ settings_t opts;
 
 int interrupted = 0; /* gets set if we get a SIGHUP */
 
-static xc_interface *xc_handle;
+static xc_interface *xch;
 static xc_evtchn *xce_handle = NULL;
 static int virq_port = -1;
 static int outfd = 1;
@@ -447,7 +439,7 @@ static void get_tbufs(unsigned long *mfn, unsigned long *size)
     if(!opts.tbuf_size)
       opts.tbuf_size = DEFAULT_TBUF_SIZE;
 
-    ret = xc_tbuf_enable(xc_handle, opts.tbuf_size, mfn, size);
+    ret = xc_tbuf_enable(xch, opts.tbuf_size, mfn, size);
 
     if ( ret != 0 )
     {
@@ -471,7 +463,7 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
     int i;
 
     /* Map t_info metadata structure */
-    tbufs.t_info = xc_map_foreign_range(xc_handle, DOMID_XEN, tinfo_size,
+    tbufs.t_info = xc_map_foreign_range(xch, DOMID_XEN, tinfo_size,
                                         PROT_READ, tbufs_mfn);
 
     if ( tbufs.t_info == 0 ) 
@@ -506,7 +498,7 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
         for ( j=0; j<tbufs.t_info->tbuf_size; j++)
             pfn_list[j] = (xen_pfn_t)mfn_list[j];
 
-        tbufs.meta[i] = xc_map_foreign_batch(xc_handle, DOMID_XEN,
+        tbufs.meta[i] = xc_map_foreign_batch(xch, DOMID_XEN,
                                              PROT_READ | PROT_WRITE,
                                              pfn_list,
                                              tbufs.t_info->tbuf_size);
@@ -532,10 +524,10 @@ static void set_mask(uint32_t mask, int type)
     int ret = 0;
 
     if (type == 1) {
-        ret = xc_tbuf_set_cpu_mask(xc_handle, mask);
+        ret = xc_tbuf_set_cpu_mask(xch, mask);
         fprintf(stderr, "change cpumask to 0x%x\n", mask);
     } else if (type == 0) {
-        ret = xc_tbuf_set_evt_mask(xc_handle, mask);
+        ret = xc_tbuf_set_evt_mask(xch, mask);
         fprintf(stderr, "change evtmask to 0x%x\n", mask);
     }
 
@@ -554,7 +546,7 @@ static unsigned int get_num_cpus(void)
     xc_physinfo_t physinfo = { 0 };
     int ret;
     
-    ret = xc_physinfo(xc_handle, &physinfo);
+    ret = xc_physinfo(xch, &physinfo);
     
     if ( ret != 0 )
     {
@@ -1010,8 +1002,8 @@ int main(int argc, char **argv)
 
     parse_args(argc, argv);
 
-    xc_handle = xc_interface_open(0,0,0);
-    if ( !xc_handle ) 
+    xch = xc_interface_open(0,0,0);
+    if ( !xch )
     {
         perror("xenctrl interface open");
         exit(EXIT_FAILURE);
-- 
1.7.7.6

Re: [PATCH v2 1/4] xentrace: Use PERROR from xc_private. instead of our own.

[George Dunlap]

On 06/13/2014 09:41 PM, Konrad Rzeszutek Wilk wrote:
> However, the xc_private PERROR uses 'xch' while we call said
> structure 'xc_handle' - so this patch also changes the variable
> to xch.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I wasn't sure whether "xc_private.h" was intended to be included by 
tools outside the libxc directory, but it seems to be included by 
xenpaging and a couple of random things inside tools/misc; so:

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

Re: [PATCH v2 1/4] xentrace: Use PERROR from xc_private. instead of our own.

[Ian Campbell]

On Mon, 2014-06-16 at 12:43 +0100, George Dunlap wrote:
> On 06/13/2014 09:41 PM, Konrad Rzeszutek Wilk wrote:
> > However, the xc_private PERROR uses 'xch' while we call said
> > structure 'xc_handle' - so this patch also changes the variable
> > to xch.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> I wasn't sure whether "xc_private.h" was intended to be included by 
> tools outside the libxc directory, but it seems to be included by 
> xenpaging and a couple of random things inside tools/misc;

Despite this it's really not supposed to be used outside of libxc
itself. We'd like to get rid of those included with time. Things outside
of libxc itself should use the xtl_* interface I think.

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

Re: [PATCH v2 1/4] xentrace: Use PERROR from xc_private. instead of our own.

[Andrew Cooper]

On 16/06/14 12:43, George Dunlap wrote:
> On 06/13/2014 09:41 PM, Konrad Rzeszutek Wilk wrote:
>> However, the xc_private PERROR uses 'xch' while we call said
>> structure 'xc_handle' - so this patch also changes the variable
>> to xch.
>>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> I wasn't sure whether "xc_private.h" was intended to be included by
> tools outside the libxc directory, but it seems to be included by
> xenpaging and a couple of random things inside tools/misc; so:
>
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

I disagree - xc_private.h is private to libxc and should not be used
outside of tools/libxc/*

There are some naughty examples which should be fixed.

If there are bits in xc_private.h which are needed by outside programs,
then they should move to xen{ctrl,guest}.h as appropriate.

~Andrew

Re: [PATCH v2 1/4] xentrace: Use PERROR from xc_private. instead of our own.

[Konrad Rzeszutek Wilk]

On Mon, Jun 16, 2014 at 12:49:25PM +0100, Andrew Cooper wrote:
> On 16/06/14 12:43, George Dunlap wrote:
> > On 06/13/2014 09:41 PM, Konrad Rzeszutek Wilk wrote:
> >> However, the xc_private PERROR uses 'xch' while we call said
> >> structure 'xc_handle' - so this patch also changes the variable
> >> to xch.
> >>
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > I wasn't sure whether "xc_private.h" was intended to be included by
> > tools outside the libxc directory, but it seems to be included by
> > xenpaging and a couple of random things inside tools/misc; so:
> >
> > Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> I disagree - xc_private.h is private to libxc and should not be used
> outside of tools/libxc/*
> 
> There are some naughty examples which should be fixed.
> 
> If there are bits in xc_private.h which are needed by outside programs,
> then they should move to xen{ctrl,guest}.h as appropriate.

Heh. All of this started with me wanting to have 'DIV_ROUNDS_UP' being
a header shared between libxc and xentrace.

It can be in the 'xc_bitsops.h" or in the "trace.h" file. Ian Campbell
didn't want in xc_bitops.h so would "trace.h" file be OK?

> 
> ~Andrew

[PATCH v2 2/4] 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>
[v2: Use DIV_ROUND_UP macro as suggested by Daniel]
[v3: Use 'int' for bits instead of 'unsigned int' as spotted by Boris]
[v4: Move DIV_ROUND_UP in xc_private instead of xc_bitops]
---
 tools/libxc/xc_private.h  |    2 +
 tools/libxc/xc_tbuf.c     |   19 ++++++--
 tools/libxc/xenctrl.h     |    2 +-
 tools/xentrace/xentrace.8 |    3 +
 tools/xentrace/xentrace.c |   97 ++++++++++++++++++++++++++++++++++++++-------
 5 files changed, 102 insertions(+), 21 deletions(-)

diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index c7730f2..4bd1528 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -66,6 +66,8 @@
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 #endif
 
+#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+
 /*
 ** Define max dirty page cache to permit during save/restore -- need to balance 
 ** keeping cache usage down with CPU impact of invalidating too often.
diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
index 8777492..2828aee 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -113,14 +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);
     int ret = -1;
-    uint64_t mask64 = mask;
+    int local_bits;
 
-    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, sizeof(mask64));
+    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;
+    }
+    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 hypercall");
@@ -131,10 +140,10 @@ 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);
+    memcpy(bytemap, mask, DIV_ROUND_UP(bits, 8));
 
     set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
-    sysctl.u.tbuf_op.cpu_mask.nr_bits = sizeof(bytemap) * 8;
+    sysctl.u.tbuf_op.cpu_mask.nr_bits = bits;
 
     ret = do_sysctl(xch, &sysctl);
 
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index b55d857..4f30db0 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1489,7 +1489,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 16034e3..03b5537 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -44,7 +44,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;
@@ -513,23 +513,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(xch);
+    if ( bits <= 0 )
+        goto out;
+
+    if ( !mask )
+    {
+        mask = xc_cpumap_alloc(xch);
+        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(xch, 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(xch, mask);
-        fprintf(stderr, "change cpumask to 0x%x\n", mask);
-    } else if (type == 0) {
-        ret = xc_tbuf_set_evt_mask(xch, mask);
-        fprintf(stderr, "change evtmask to 0x%x\n", mask);
-    }
+    ret = xc_tbuf_set_evt_mask(xch, mask);
+    fprintf(stderr, "change evtmask to 0x%x\n", mask);
 
     if ( ret != 0 )
     {
@@ -898,6 +941,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)
 {
@@ -929,7 +989,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*/
@@ -994,7 +1059,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;
@@ -1010,10 +1075,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 v2 2/4] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t

[George Dunlap]

On 06/13/2014 09:41 PM, 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>

xentrace side looks good to me, with one minor thing...

> @@ -513,23 +513,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");

If I'm reading this right, if the user enters "-c 0x0", the output of 
this will be as follows:

  change cpumask to 0x

Maybe we should add "i==0" (or "!i") to the conditions under which it 
will print the mask?

  -George

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

[Konrad Rzeszutek Wilk]

On Mon, Jun 16, 2014 at 02:30:28PM +0100, George Dunlap wrote:
> On 06/13/2014 09:41 PM, 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>
> 
> xentrace side looks good to me, with one minor thing...
> 
> >@@ -513,23 +513,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");
> 
> If I'm reading this right, if the user enters "-c 0x0", the output of this
> will be as follows:
> 
>  change cpumask to 0x
> 
> Maybe we should add "i==0" (or "!i") to the conditions under which it will
> print the mask?

Will do in the next posting!
> 
>  -George
> 

[PATCH v2 3/4] libxc/tbuf: Use the xc_hypercall_bounce_[pre|post] instead of memcpy

[Konrad Rzeszutek Wilk]

for bounce buffer of the xc_cpumask_t.

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

diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
index 2828aee..85156dd 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -116,7 +116,7 @@ int xc_tbuf_disable(xc_interface *xch)
 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, 0, XC_HYPERCALL_BUFFER_BOUNCE_IN);
     int ret = -1;
     int local_bits;
 
@@ -129,8 +129,9 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask, int bits)
         PERROR("Wrong amount of bits supplied: %d > %d!\n", bits, local_bits);
         goto out;
     }
-    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, DIV_ROUND_UP(bits, 8));
-    if ( bytemap == NULL )
+
+    HYPERCALL_BOUNCE_SET_SIZE(mask, DIV_ROUND_UP(bits, 8));
+    if ( xc_hypercall_bounce_pre(xch, mask) )
     {
         PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask hypercall");
         goto out;
@@ -140,14 +141,12 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask, int bits)
     sysctl.interface_version = XEN_SYSCTL_INTERFACE_VERSION;
     sysctl.u.tbuf_op.cmd  = XEN_SYSCTL_TBUFOP_set_cpu_mask;
 
-    memcpy(bytemap, mask, DIV_ROUND_UP(bits, 8));
-
-    set_xen_guest_handle(sysctl.u.tbuf_op.cpu_mask.bitmap, bytemap);
+    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;
-- 
1.7.7.6

Re: [PATCH v2 3/4] libxc/tbuf: Use the xc_hypercall_bounce_[pre|post] instead of memcpy

[Ian Campbell]

On Fri, 2014-06-13 at 16:41 -0400, Konrad Rzeszutek Wilk wrote:
> for bounce buffer of the xc_cpumask_t.

You just added this stuff in the previous patch. Why not fold?

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxc/xc_tbuf.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
> index 2828aee..85156dd 100644
> --- a/tools/libxc/xc_tbuf.c
> +++ b/tools/libxc/xc_tbuf.c
> @@ -116,7 +116,7 @@ int xc_tbuf_disable(xc_interface *xch)
>  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, 0, XC_HYPERCALL_BUFFER_BOUNCE_IN);

s/0/DIV_ROUND_UP(bits, 8)/ works here and avoids the need for
HYPERCALL_BOUNCE_SET_SIZE (I think). 

The SET_SIZE macro is only generally used when you don't know the size
at declaration time (i.e. because you need to query it first).

Ian.

Re: [PATCH v2 3/4] libxc/tbuf: Use the xc_hypercall_bounce_[pre|post] instead of memcpy

[Konrad Rzeszutek Wilk]

On Wed, Jun 18, 2014 at 02:55:05PM +0100, Ian Campbell wrote:
> On Fri, 2014-06-13 at 16:41 -0400, Konrad Rzeszutek Wilk wrote:
> > for bounce buffer of the xc_cpumask_t.
> 
> You just added this stuff in the previous patch. Why not fold?

I was thinking it might be a bit too busy.

But I will be more than happy to fold it in.
> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxc/xc_tbuf.c |   13 ++++++-------
> >  1 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
> > index 2828aee..85156dd 100644
> > --- a/tools/libxc/xc_tbuf.c
> > +++ b/tools/libxc/xc_tbuf.c
> > @@ -116,7 +116,7 @@ int xc_tbuf_disable(xc_interface *xch)
> >  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, 0, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> 
> s/0/DIV_ROUND_UP(bits, 8)/ works here and avoids the need for
> HYPERCALL_BOUNCE_SET_SIZE (I think). 

<nods>
> 
> The SET_SIZE macro is only generally used when you don't know the size
> at declaration time (i.e. because you need to query it first).
> 
> Ian.
> 

[PATCH v2 4/4] 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 |   21 +++++-
 tools/xentrace/xentrace.c |  155 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 165 insertions(+), 11 deletions(-)

diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index c176a96..9beb72d 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -36,11 +36,26 @@ all new records to the output
 set the time, p, (in milliseconds) to sleep between polling the buffers
 for new data.
 .TP
-.B -c, --cpu-mask=c
-set bitmask of CPUs to trace. It is limited to 32-bits.
+.B -c, --cpu-mask=[\fIc\fP|\fICPU-LIST\fP]
+set bitmask of CPUs to trace. It is limited to 32-bits if using hex digits.
 If not specified, the cpu-mask of all of the available CPUs will be
-constructed.
+constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which
+may be specified as follows:
 
+.RS 4
+.ie n .IP """0-3""" 4
+.el .IP "``0-3''" 4
+.IX Item "0-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """0,2,5-7""" 4
+.el .IP "``0,2,5-7''" 4
+.IX Item "0,2,5-7"
+Trace only on CPUs 0, 2, and 5 through 7.
+.RE
+.Sp
+
+If this option is not specified, xentrace will trace all of the physical
+CPUs on the machine.
 .TP
 .B -e, --evt-mask=e
 set event capture mask. If not specified the TRC_ALL will be used.
diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index 03b5537..45b0597 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -23,6 +23,7 @@
 #include <string.h>
 #include <getopt.h>
 #include <assert.h>
+#include <ctype.h>
 #include <sys/poll.h>
 #include <sys/statvfs.h>
 
@@ -31,6 +32,7 @@
 
 #include <xenctrl.h>
 #include "xc_private.h"
+#include "xc_bitops.h" /* for set_bit */
 
 /***** Compile time configuration of defaults ********************************/
 
@@ -45,6 +47,7 @@ typedef struct settings_st {
     unsigned long poll_sleep; /* milliseconds to sleep between polls */
     uint32_t evt_mask;
     xc_cpumap_t cpu_mask;
+    char *cpu_mask_str;
     unsigned long tbuf_size;
     unsigned long disk_rsvd;
     unsigned long timeout;
@@ -809,7 +812,7 @@ static void usage(void)
 "Usage: xentrace [OPTION...] [output file]\n" \
 "Tool to capture Xen trace buffer data\n" \
 "\n" \
-"  -c, --cpu-mask=c        Set cpu-mask\n" \
+"  -c, --cpu-mask=c        Set cpu-mask using hex and CPU ranges\n" \
 "  -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" \
@@ -958,6 +961,126 @@ 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;
+
+    if ( !buflen )
+    {
+        fprintf(stderr, "Invalid option argument: %s\n", arg);
+        usage(); /* does exit */
+    }
+    nmaskbits = xc_get_max_cpus(xch);
+    if ( nmaskbits <= 0 )
+    {
+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
+        usage();
+    }
+    map = xc_cpumap_alloc(xch);
+    if ( !map )
+    {
+        fprintf(stderr, "Out of memory!\n");
+        usage();
+    }
+    c = c_old = totaldigits = 0;
+    do {
+        in_range = 0;
+        a = b = 0;
+        while ( buflen )
+        {
+            c_old = c;
+            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 ( 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);
+    usage();
+    return 0; /* Never reached */
+}
+
+static int check_for_hex(const char *s)
+{
+    int buflen;
+    int c_old, c;
+
+    buflen = strlen(s);
+    c_old = c = 0;
+    do {
+            c = *s++;
+            buflen--;
+
+            if ( isspace(c) && c_old != ZERO_DIGIT )
+                continue;
+
+            if ( c == '\0' )
+                return -1;
+
+            if ( c == ZERO_DIGIT )
+                c_old = c;
+
+            if ( c == 'x' && c_old == ZERO_DIGIT )
+                return 1;
+
+     } while ( buflen );
+
+    return 0;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -989,14 +1112,25 @@ static void parse_args(int argc, char **argv)
             break;
 
         case 'c': /* set new cpu mask for filtering*/
-            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
-            if ( parse_cpumask(optarg) )
             {
-                perror("Not enough memory!");
-                exit(EXIT_FAILURE);
+                int ret = check_for_hex(optarg);
+
+                if ( ret < 0 )
+                    usage();
+
+                if ( ret )
+                    ret = parse_cpumask(optarg);
+                else
+                    /* Set opts.cpu_mask later as we don't have 'xch' set yet.*/
+                    opts.cpu_mask_str = strdup(optarg);
+
+                if ( ret )
+                {
+                    perror("Not enough memory!");
+                    exit(EXIT_FAILURE);
+                }
+                break;
             }
-            break;
-        
         case 'e': /* set new event mask for filtering*/
             parse_evtmask(optarg);
             break;
@@ -1060,6 +1194,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;
@@ -1077,7 +1212,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.7.7.6

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

[George Dunlap]

On 06/13/2014 09:41 PM, Konrad Rzeszutek Wilk wrote:
> Instead of just using -c 0x<some hex value> we can
> also use -c <starting cpu>-<end cpu> or -c <cpu1>,<cpu2>
> or a combination of them.
>
> That should make it easier to trace the right CPU if
> using this along with 'xl vcpu-list'.
>
> The code has been lifted from the Linux kernel, see file
> lib/bitmap.c, function __bitmap_parselist.
>
> To make the old behavior and the new function work, we check
> to see if the arguments have '0x' in them. If they do
> we use the old style parsing (limited to 32 CPUs). If that
> does not exist we use the new parsing.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>   tools/xentrace/xentrace.8 |   21 +++++-
>   tools/xentrace/xentrace.c |  155 ++++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 165 insertions(+), 11 deletions(-)
>
> diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
> index c176a96..9beb72d 100644
> --- a/tools/xentrace/xentrace.8
> +++ b/tools/xentrace/xentrace.8
> @@ -36,11 +36,26 @@ all new records to the output
>   set the time, p, (in milliseconds) to sleep between polling the buffers
>   for new data.
>   .TP
> -.B -c, --cpu-mask=c
> -set bitmask of CPUs to trace. It is limited to 32-bits.
> +.B -c, --cpu-mask=[\fIc\fP|\fICPU-LIST\fP]
> +set bitmask of CPUs to trace.
>   It is limited to 32-bits if using hex digits.
>   If not specified, the cpu-mask of all of the available CPUs will be

I think something like this would be better:

"This can be either a hex value (of the form 0xNNNN...), or a set of cpu 
ranges as described below.  Hex values are limited to 32 bits. If not 
specified..."

> -constructed.
> +constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which
> +may be specified as follows:
>   
> +.RS 4
> +.ie n .IP """0-3""" 4
> +.el .IP "``0-3''" 4
> +.IX Item "0-3"
> +Trace only on CPUs 0 through 3
> +.ie n .IP """0,2,5-7""" 4
> +.el .IP "``0,2,5-7''" 4
> +.IX Item "0,2,5-7"
> +Trace only on CPUs 0, 2, and 5 through 7.
> +.RE
> +.Sp
> +
> +If this option is not specified, xentrace will trace all of the physical
> +CPUs on the machine.

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

>   .TP
>   .B -e, --evt-mask=e
>   set event capture mask. If not specified the TRC_ALL will be used.
> diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
> index 03b5537..45b0597 100644
> --- a/tools/xentrace/xentrace.c
> +++ b/tools/xentrace/xentrace.c
> @@ -23,6 +23,7 @@
>   #include <string.h>
>   #include <getopt.h>
>   #include <assert.h>
> +#include <ctype.h>
>   #include <sys/poll.h>
>   #include <sys/statvfs.h>
>   
> @@ -31,6 +32,7 @@
>   
>   #include <xenctrl.h>
>   #include "xc_private.h"
> +#include "xc_bitops.h" /* for set_bit */
>   
>   /***** Compile time configuration of defaults ********************************/
>   
> @@ -45,6 +47,7 @@ typedef struct settings_st {
>       unsigned long poll_sleep; /* milliseconds to sleep between polls */
>       uint32_t evt_mask;
>       xc_cpumap_t cpu_mask;
> +    char *cpu_mask_str;
>       unsigned long tbuf_size;
>       unsigned long disk_rsvd;
>       unsigned long timeout;
> @@ -809,7 +812,7 @@ static void usage(void)
>   "Usage: xentrace [OPTION...] [output file]\n" \
>   "Tool to capture Xen trace buffer data\n" \
>   "\n" \
> -"  -c, --cpu-mask=c        Set cpu-mask\n" \
> +"  -c, --cpu-mask=c        Set cpu-mask using hex and CPU ranges\n" \

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

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

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

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

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

  -George