[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]
 - Redid the code per George's feedback
 - Expanded the xc_cpumap_* calls so that we have an 'setbit' variant.


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. There is also the mode of
automatic detection, such as: -c -,<cpu2> (so it will assume up to cpu2 - so
0,1, and 2 CPU), or the inverse: -c <cpu2>- (which will figure the max cpus and
do it from cpu2 up to maximum cpu).

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.

The patches are also at my git tree:

 git://xenbits.xen.org/people/konradwilk/xen.git xentrace.v3


 tools/libxc/include/xenctrl.h |  16 ++-
 tools/libxc/xc_misc.c         |  16 +++
 tools/libxc/xc_tbuf.c         |  26 +++--
 tools/xentrace/xentrace.8     |  33 +++++-
 tools/xentrace/xentrace.c     | 240 ++++++++++++++++++++++++++++++++++++++----
 5 files changed, 300 insertions(+), 31 deletions(-)

Konrad Rzeszutek Wilk (3):
      libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.
      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/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Konrad Rzeszutek Wilk]

We export the xc_cpumap_alloc but not the bit operations.
One could include 'xc_bitops.h' but that is naughty - so instead
we just export the proper functions to do it on the xc_cpumap_t
typedef.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/include/xenctrl.h |  9 +++++++++
 tools/libxc/xc_misc.c         | 16 ++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..565f098 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -394,6 +394,15 @@ int xc_get_cpumap_size(xc_interface *xch);
 /* allocate a cpumap */
 xc_cpumap_t xc_cpumap_alloc(xc_interface *xch);
 
+/* clear an CPU from the cpumap. */
+void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map);
+
+/* set an CPU in the cpumap. */
+void xc_cpumap_setcpu(int cpu, xc_cpumap_t map);
+
+/* Test whether the CPU in cpumap is set. */
+int xc_cpumap_testcpu(int cpu, xc_cpumap_t map);
+
 /*
  * NODEMAP handling
  */
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index be68291..7514b84 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include "xc_bitops.h"
 #include "xc_private.h"
 #include <xen/hvm/hvm_op.h>
 
@@ -93,6 +94,21 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
     return calloc(1, sz);
 }
 
+void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
+{
+    clear_bit(cpu, (unsigned long *)map);
+}
+
+void xc_cpumap_setcpu(int cpu, xc_cpumap_t map)
+{
+    set_bit(cpu, (unsigned long *)map);
+}
+
+int xc_cpumap_testcpu(int cpu, xc_cpumap_t map)
+{
+    return test_bit(cpu, (unsigned long *)map);
+}
+
 xc_nodemap_t xc_nodemap_alloc(xc_interface *xch)
 {
     int sz;
-- 
2.1.0

Re: [PATCH v3 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Ian Campbell]

On Tue, 2015-03-24 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:

Please make sure you CC all of the toolstack maintainers.

> +void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
> +{
> +    clear_bit(cpu, (unsigned long *)map);

Is it necessary to worry about alignment here, since xc_cpumap_t is
actually a uint8_t*.

I'm afraid I think it probably is on ARM at least, which is rather
tedious.

Or do we rely on all of these always being dynamically allocated (via
xc_cpumap_alloc) and therefore "suitably aligned so that it may be
assigned to a pointer to any type of object"[0]  following calloc ,
avoids the issue in practice?

I think we probably do, does anyone disagree with that assessment?

Ian.

[0]
http://pubs.opengroup.org/onlinepubs/9699919799/functions/calloc.html#tag_16_39

Re: [PATCH v3 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Konrad Rzeszutek Wilk]

On Tue, Mar 24, 2015 at 05:46:04PM +0000, Ian Campbell wrote:
> On Tue, 2015-03-24 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 
> Please make sure you CC all of the toolstack maintainers.
> 
> > +void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
> > +{
> > +    clear_bit(cpu, (unsigned long *)map);
> 
> Is it necessary to worry about alignment here, since xc_cpumap_t is
> actually a uint8_t*.
> 
> I'm afraid I think it probably is on ARM at least, which is rather
> tedious.
> 
> Or do we rely on all of these always being dynamically allocated (via
> xc_cpumap_alloc) and therefore "suitably aligned so that it may be
> assigned to a pointer to any type of object"[0]  following calloc ,
> avoids the issue in practice?
> 
> I think we probably do, does anyone disagree with that assessment?


We can also do and not worry about it:

>From 4620f9b35622ddf70db754f87a9114c235eb01de Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 24 Mar 2015 16:23:08 -0400
Subject: [PATCH] swap

---
 tools/libxc/xc_misc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 7514b84..19a1b18 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -94,19 +94,22 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
     return calloc(1, sz);
 }
 
+#define BITS_PER_CPUMAP(map) (sizeof(*map) * 8)
+#define CPUMAP_ENTRY(cpu, map) ((map))[(cpu) / BITS_PER_CPUMAP(map)]
+#define CPUMAP_SHIFT(cpu, map) ((cpu) % BITS_PER_CPUMAP(map))
 void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
 {
-    clear_bit(cpu, (unsigned long *)map);
+    CPUMAP_ENTRY(cpu, map) &= ~(1U << CPUMAP_SHIFT(cpu, map));
 }
 
 void xc_cpumap_setcpu(int cpu, xc_cpumap_t map)
 {
-    set_bit(cpu, (unsigned long *)map);
+    CPUMAP_ENTRY(cpu, map) |= (1U << CPUMAP_SHIFT(cpu, map));
 }
 
 int xc_cpumap_testcpu(int cpu, xc_cpumap_t map)
 {
-    return test_bit(cpu, (unsigned long *)map);
+    return (CPUMAP_ENTRY(cpu, map) >> CPUMAP_SHIFT(cpu, map)) & 1;
 }
 
 xc_nodemap_t xc_nodemap_alloc(xc_interface *xch)
-- 
2.1.0

Re: [PATCH v3 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1079 bytes --]

On Tue, 2015-03-24 at 16:29 -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 24, 2015 at 05:46:04PM +0000, Ian Campbell wrote:
> > Is it necessary to worry about alignment here, since xc_cpumap_t is
> > actually a uint8_t*.
>
> We can also do and not worry about it:
>
> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> index 7514b84..19a1b18 100644
> --- a/tools/libxc/xc_misc.c
> +++ b/tools/libxc/xc_misc.c
> @@ -94,19 +94,22 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
>      return calloc(1, sz);
>  }
>  
> +#define BITS_PER_CPUMAP(map) (sizeof(*map) * 8)
> +#define CPUMAP_ENTRY(cpu, map) ((map))[(cpu) / BITS_PER_CPUMAP(map)]
> +#define CPUMAP_SHIFT(cpu, map) ((cpu) % BITS_PER_CPUMAP(map))
>
> [...]
>
Maybe it's only me, but I really find it a bit hard to figure out what
the differences between this and what's in xc_bitops.h are.

If going for this, I'd say that the reasons why we need these, and such
differences between these and BITMAP_* should be made evident somehow
(changelog, comments, etc.).

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Konrad Rzeszutek Wilk]

On Wed, Mar 25, 2015 at 08:53:01AM +0000, Dario Faggioli wrote:
> On Tue, 2015-03-24 at 16:29 -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Mar 24, 2015 at 05:46:04PM +0000, Ian Campbell wrote:
> > > Is it necessary to worry about alignment here, since xc_cpumap_t is
> > > actually a uint8_t*.
> >
> > We can also do and not worry about it:
> >
> > diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> > index 7514b84..19a1b18 100644
> > --- a/tools/libxc/xc_misc.c
> > +++ b/tools/libxc/xc_misc.c
> > @@ -94,19 +94,22 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
> >      return calloc(1, sz);
> >  }
> >  
> > +#define BITS_PER_CPUMAP(map) (sizeof(*map) * 8)
> > +#define CPUMAP_ENTRY(cpu, map) ((map))[(cpu) / BITS_PER_CPUMAP(map)]
> > +#define CPUMAP_SHIFT(cpu, map) ((cpu) % BITS_PER_CPUMAP(map))
> >
> > [...]
> >
> Maybe it's only me, but I really find it a bit hard to figure out what
> the differences between this and what's in xc_bitops.h are.
> 
> If going for this, I'd say that the reasons why we need these, and such
> differences between these and BITMAP_* should be made evident somehow
> (changelog, comments, etc.).

Here is an updated patch:

>From dbb1bf2d12b24a6a91ad95abef0d4310e7141b79 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 23 Mar 2015 11:52:54 -0400
Subject: [PATCH] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to
 complement xc_cpumap_alloc.

We export the xc_cpumap_alloc but not the bit operations.
One could include 'xc_bitops.h' but that is naughty - so instead
we just export the proper functions to do it on the xc_cpumap_t
typedef.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
----
v2: Use our own macro to make sure ARM is not affected negatively
---
 tools/libxc/include/xenctrl.h |  9 +++++++++
 tools/libxc/xc_misc.c         | 30 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..565f098 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -394,6 +394,15 @@ int xc_get_cpumap_size(xc_interface *xch);
 /* allocate a cpumap */
 xc_cpumap_t xc_cpumap_alloc(xc_interface *xch);
 
+/* clear an CPU from the cpumap. */
+void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map);
+
+/* set an CPU in the cpumap. */
+void xc_cpumap_setcpu(int cpu, xc_cpumap_t map);
+
+/* Test whether the CPU in cpumap is set. */
+int xc_cpumap_testcpu(int cpu, xc_cpumap_t map);
+
 /*
  * NODEMAP handling
  */
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index be68291..e113385 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include "xc_bitops.h"
 #include "xc_private.h"
 #include <xen/hvm/hvm_op.h>
 
@@ -93,6 +94,35 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
     return calloc(1, sz);
 }
 
+/*
+ * xc_bitops.h has macros that do this as well - however xc_cpumap_t
+ * is an array of uint8 (1byte) while said macros expect an array
+ * of unsigned long (8 bytes). This is not a problem when setting
+ * an bit (as the computation will come with the same offset) - the
+ * issue is when clearing an bit. The aligment on ARM could affect
+ * other structures that are close in memory causing memory corruption.
+ * Hence our own macros that differ only in that we compute the
+ * size of the array elements (sizeof(*map)) as opposed to assuming
+ * it is unsigned long.
+ */
+#define BITS_PER_CPUMAP(map) (sizeof(*map) * 8)
+#define CPUMAP_ENTRY(cpu, map) ((map))[(cpu) / BITS_PER_CPUMAP(map)]
+#define CPUMAP_SHIFT(cpu, map) ((cpu) % BITS_PER_CPUMAP(map))
+void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
+{
+    CPUMAP_ENTRY(cpu, map) &= ~(1U << CPUMAP_SHIFT(cpu, map));
+}
+
+void xc_cpumap_setcpu(int cpu, xc_cpumap_t map)
+{
+    CPUMAP_ENTRY(cpu, map) |= (1U << CPUMAP_SHIFT(cpu, map));
+}
+
+int xc_cpumap_testcpu(int cpu, xc_cpumap_t map)
+{
+    return (CPUMAP_ENTRY(cpu, map) >> CPUMAP_SHIFT(cpu, map)) & 1;
+}
+
 xc_nodemap_t xc_nodemap_alloc(xc_interface *xch)
 {
     int sz;
-- 
2.1.0

Re: [PATCH v3 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1224 bytes --]

On Tue, 2015-03-24 at 17:46 +0000, Ian Campbell wrote:
> On Tue, 2015-03-24 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:

> > +void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
> > +{
> > +    clear_bit(cpu, (unsigned long *)map);
> 
> Is it necessary to worry about alignment here, since xc_cpumap_t is
> actually a uint8_t*.
>
> [..]
>
> Or do we rely on all of these always being dynamically allocated (via
> xc_cpumap_alloc) and therefore "suitably aligned so that it may be
> assigned to a pointer to any type of object"[0]  following calloc ,
> avoids the issue in practice?
> 
> I think we probably do, does anyone disagree with that assessment?
> 
FWIW, I agree with it.

The only use case that deviates from that which I could find is:

xc_vcpu_setaffinity()
  |
  --> xc_hypercall_bounce_pre() ==
      xc__hypercall_bounce_pre()
        |
        --> xc__hypercall_buffer_alloc()
              |
              --> xc__hypercall_buffer_alloc_pages()
                    |
                    --> hypercall_buffer_cache_alloc() ||
                        linux_privcmd_alloc_hypercall_buffer() (or OS speific variants)

which is probably still fine, isn't it?

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Ian Campbell]

On Wed, 2015-03-25 at 08:47 +0000, Dario Faggioli wrote:
> On Tue, 2015-03-24 at 17:46 +0000, Ian Campbell wrote:
> > On Tue, 2015-03-24 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 
> > > +void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
> > > +{
> > > +    clear_bit(cpu, (unsigned long *)map);
> > 
> > Is it necessary to worry about alignment here, since xc_cpumap_t is
> > actually a uint8_t*.
> >
> > [..]
> >
> > Or do we rely on all of these always being dynamically allocated (via
> > xc_cpumap_alloc) and therefore "suitably aligned so that it may be
> > assigned to a pointer to any type of object"[0]  following calloc ,
> > avoids the issue in practice?
> > 
> > I think we probably do, does anyone disagree with that assessment?
> > 
> FWIW, I agree with it.
> 
> The only use case that deviates from that which I could find is:
> 
> xc_vcpu_setaffinity()
>   |
>   --> xc_hypercall_bounce_pre() ==
>       xc__hypercall_bounce_pre()
>         |
>         --> xc__hypercall_buffer_alloc()
>               |
>               --> xc__hypercall_buffer_alloc_pages()
>                     |
>                     --> hypercall_buffer_cache_alloc() ||
>                         linux_privcmd_alloc_hypercall_buffer() (or OS speific variants)
> 
> which is probably still fine, isn't it?

Might we use test_bit and friends on a hypercall buffer directly? I
didn't expect so.

I think it would be safe none the less, since it is all page aligned,
but someone would need to check I didn't do something smarter for small
allocations at some point..

> 
> Regards,
> Dario

Re: [PATCH v3 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1026 bytes --]

On Wed, 2015-03-25 at 11:01 +0000, Ian Campbell wrote:
> On Wed, 2015-03-25 at 08:47 +0000, Dario Faggioli wrote:

> > The only use case that deviates from that which I could find is:
> > 
> > xc_vcpu_setaffinity()
> >   |
> >   --> xc_hypercall_bounce_pre() ==
> >       xc__hypercall_bounce_pre()
> >         |
> >         --> xc__hypercall_buffer_alloc()
> >               |
> >               --> xc__hypercall_buffer_alloc_pages()
> >                     |
> >                     --> hypercall_buffer_cache_alloc() ||
> >                         linux_privcmd_alloc_hypercall_buffer() (or OS speific variants)
> > 
> > which is probably still fine, isn't it?
> 
> Might we use test_bit and friends on a hypercall buffer directly? I
> didn't expect so.
> 
Exactly. I also thought about this, but then did not put it down. That
is a usecase where we don't go though xc_cpumap_alloc(), but we
certainly don't do any of these operations in there right now, and most
likely never will, IMO.

Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 2/3] 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>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
[libxc pieces]
----
[v2: Fix up the bit mask counting.]
[v3: Return EXIT_FAILURE instead of Exx]
[v4: Just do 'if (xc_tbuf..)' check for errors instead of stashing ret,
     fix print_cpu_mask not including leading zeros for long hex mask]
---
 tools/libxc/include/xenctrl.h |   7 ++-
 tools/libxc/xc_tbuf.c         |  26 ++++++----
 tools/xentrace/xentrace.8     |   3 ++
 tools/xentrace/xentrace.c     | 108 ++++++++++++++++++++++++++++++++++++------
 4 files changed, 118 insertions(+), 26 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 565f098..90b4487 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1543,6 +1543,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
@@ -1583,7 +1588,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 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/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..40504ec 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,70 @@ 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 ) {
+            if ( had_printed )
+                fprintf(stderr,"%02x", v);
+            else
+                fprintf(stderr,"%x", v);
+            had_printed = 1;
+        }
+   }
+   fprintf(stderr, "\n");
+}
+
+static void set_cpu_mask(xc_cpumap_t mask, int bits)
+{
+    int i;
+
+    if ( bits <= 0 )
+    {
+        bits = xc_get_max_cpus(xc_handle);
+        if ( bits <= 0 )
+            goto out;
+    }
+    if ( !mask )
+    {
+        mask = xc_cpumap_alloc(xc_handle);
+        if ( !mask )
+            goto out;
+
+        /* Set it to include _all_ CPUs. */
+        for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ )
+            mask[i] = 0xff;
+    }
+    /* And this will limit it to the exact amount of bits. */
+    if ( xc_tbuf_set_cpu_mask(xc_handle, mask, bits) )
+        goto out;
+
+    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 +954,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;
+
+    map = malloc(sizeof(uint32_t));
+    if ( !map )
+        return EXIT_FAILURE;
+
+    v = argtol(arg, 0);
+    for ( i = 0; i < sizeof(uint32_t) ; i++ )
+        map[i] = (v >> (i * 8)) & 0xff;
+
+    for ( bits = 0; v; v >>= 1 )
+        bits ++;
+
+    opts.cpu_mask = map;
+    opts.cpu_bits = bits;
+    return 0;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -937,7 +1007,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 +1077,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 +1094,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);
-- 
2.1.0

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

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 7168 bytes --]

On 03/24/2015 03:39 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>
> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> [libxc pieces]

OK, so I just took the time to wrap my brain around this patch more, and
I'm afraid I think it's almost entirely wrong. :-/

To sum up:

1. There's no reason to pass the number of bits to xc_tbuf_set_cpu_mask.
 The caller should just pass a fully-filled xc_cpumask_t.

2. xentrace shouldn't rely on open-coded knowledge about the length of
xc_cpumask_t; it should call xc_get_cpumask_size() and use that.

3. If the user doesn't pass a mask, then the mask should just be left
unchanged; it shouldn't silently go and set all the bits in the cpumask.

4. Allocating something the size of a 32-bit word?

Attached is a patch I wrote when I was trying to figure out what I
thought was the "right" thing to do here -- rather than try to make you
re-write the patch, I'm just attaching it (and I'll paste it in-line as
well).  (I've compiled it but not tested it.)

What do you think?

(I haven't taken a look at the other patches in the series yet.)

 -George


[PATCH] libxc/xentrace: Use xc_cpumap_t for xc_tbuf_set_cpu_mask

xentrace is the only caller at the moment.  Split the cpu and event
mask setting out into separate functions, but leave the current limit
of 32 bits for masks passed in from the command-line.

Based on a patch from Konrad Wilk <konrad.wilk@oracle.com>

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Konrad Wilk <konrad.wilk@oracle.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_tbuf.c         | 33 +++++++++++++++-------
 tools/xentrace/xentrace.c     | 64
+++++++++++++++++++++++++++++++++++--------
 3 files changed, 77 insertions(+), 22 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..92eb8da 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1574,7 +1574,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 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 8777492..a4de491 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -113,15 +113,30 @@ 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)
 {
     DECLARE_SYSCTL;
-    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
+    DECLARE_HYPERCALL_BOUNCE(mask, 0, XC_HYPERCALL_BUFFER_BOUNCE_IN);
     int ret = -1;
-    uint64_t mask64 = mask;
+    int bits, cpusize;

-    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, sizeof(mask64));
-    if ( bytemap == NULL )
+    cpusize = xc_get_cpumap_size(xch);
+    if (cpusize <= 0)
+    {
+        PERROR("Could not get number of cpus");
+        return -1;
+    }
+
+    HYPERCALL_BOUNCE_SET_SIZE(mask, cpusize);
+
+    bits = xc_get_max_cpus(xch);
+    if (bits <= 0)
+    {
+        PERROR("Could not get number of bits");
+        return -1;
+    }
+
+    if ( xc_hypercall_bounce_pre(xch, mask) )
     {
         PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask
hypercall");
         goto out;
@@ -131,14 +146,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.c b/tools/xentrace/xentrace.c
index 8a38e32..e35a131 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -521,23 +521,64 @@ static struct t_struct *map_tbufs(unsigned long
tbufs_mfn, unsigned int num,
     return &tbufs;
 }

+void print_cpu_mask(xc_cpumap_t map)
+{
+    unsigned int v, had_printed = 0;
+    int i;
+
+    fprintf(stderr, "change cpumask to 0x");
+
+    for ( i = xc_get_cpumap_size(xc_handle); i >= 0; i-- )
+    {
+        v = map[i];
+        if ( v || had_printed || !i ) {
+            fprintf(stderr,"%x", v);
+            had_printed = 1;
+        }
+   }
+   fprintf(stderr, "\n");
+}
+
+static void set_cpu_mask(uint32_t mask)
+{
+    int i, ret = 0;
+    xc_cpumap_t map;
+
+    map = xc_cpumap_alloc(xc_handle);
+    if ( !map )
+        goto out;
+
+    /*
+     * If mask is set, copy the bits out of it.  This still works for
+     * systems with more than 32 cpus, as the shift will just shift
+     * mask down to zero.
+     */
+    for ( i = 0; i < xc_get_cpumap_size(xc_handle) ; i++ )
+        map[i] = (mask >> (i*8)) & 0xff;
+
+    ret = xc_tbuf_set_cpu_mask(xc_handle, map);
+    if ( ret != 0 )
+        goto out;
+
+    print_cpu_mask(map);
+    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 )
     {
@@ -1018,10 +1059,11 @@ 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);

     if ( opts.timeout != 0 )
         alarm(opts.timeout);


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Commit-George-Dunlap-george.dunlap-eu.citrix.com.patch --]
[-- Type: text/x-patch; name="0001-Commit-George-Dunlap-george.dunlap-eu.citrix.com.patch", Size: 6025 bytes --]

From 80f4c52ae34c4457b6788d398260931ed06f084f Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@eu.citrix.com>
Date: Mon, 30 Mar 2015 16:48:44 +0100
Subject: [PATCH] Commit: George Dunlap <george.dunlap@eu.citrix.com>

[PATCH] libxc/xentrace: Use xc_cpumap_t for xc_tbuf_set_cpu_mask

xentrace is the only caller at the moment.  Split the cpu and event
mask setting out into separate functions, but leave the current limit
of 32 bits for masks passed in from the command-line.

Based on a patch from Konrad Wilk <konrad.wilk@oracle.com>

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Konrad Wilk <konrad.wilk@oracle.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_tbuf.c         | 33 +++++++++++++++-------
 tools/xentrace/xentrace.c     | 64 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 77 insertions(+), 22 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..92eb8da 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1574,7 +1574,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 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 8777492..a4de491 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -113,15 +113,30 @@ 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)
 {
     DECLARE_SYSCTL;
-    DECLARE_HYPERCALL_BUFFER(uint8_t, bytemap);
+    DECLARE_HYPERCALL_BOUNCE(mask, 0, XC_HYPERCALL_BUFFER_BOUNCE_IN);
     int ret = -1;
-    uint64_t mask64 = mask;
+    int bits, cpusize;
 
-    bytemap = xc_hypercall_buffer_alloc(xch, bytemap, sizeof(mask64));
-    if ( bytemap == NULL )
+    cpusize = xc_get_cpumap_size(xch);
+    if (cpusize <= 0)
+    {
+        PERROR("Could not get number of cpus");
+        return -1;
+    }
+
+    HYPERCALL_BOUNCE_SET_SIZE(mask, cpusize);
+
+    bits = xc_get_max_cpus(xch);
+    if (bits <= 0)
+    {
+        PERROR("Could not get number of bits");
+        return -1;
+    }
+
+    if ( xc_hypercall_bounce_pre(xch, mask) )
     {
         PERROR("Could not allocate memory for xc_tbuf_set_cpu_mask hypercall");
         goto out;
@@ -131,14 +146,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.c b/tools/xentrace/xentrace.c
index 8a38e32..e35a131 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -521,23 +521,64 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
     return &tbufs;
 }
 
+void print_cpu_mask(xc_cpumap_t map)
+{
+    unsigned int v, had_printed = 0;
+    int i;
+
+    fprintf(stderr, "change cpumask to 0x");
+
+    for ( i = xc_get_cpumap_size(xc_handle); i >= 0; i-- )
+    {
+        v = map[i];
+        if ( v || had_printed || !i ) {
+            fprintf(stderr,"%x", v);
+            had_printed = 1;
+        }
+   }
+   fprintf(stderr, "\n");
+}
+
+static void set_cpu_mask(uint32_t mask)
+{
+    int i, ret = 0;
+    xc_cpumap_t map;
+
+    map = xc_cpumap_alloc(xc_handle);
+    if ( !map )
+        goto out;
+    
+    /* 
+     * If mask is set, copy the bits out of it.  This still works for
+     * systems with more than 32 cpus, as the shift will just shift
+     * mask down to zero.
+     */
+    for ( i = 0; i < xc_get_cpumap_size(xc_handle) ; i++ )
+        map[i] = (mask >> (i*8)) & 0xff;
+
+    ret = xc_tbuf_set_cpu_mask(xc_handle, map);
+    if ( ret != 0 )
+        goto out;
+
+    print_cpu_mask(map);
+    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 )
     {
@@ -1018,10 +1059,11 @@ 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);
 
     if ( opts.timeout != 0 ) 
         alarm(opts.timeout);
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Konrad Rzeszutek Wilk]

On Mon, Mar 30, 2015 at 05:10:05PM +0100, George Dunlap wrote:
> On 03/24/2015 03:39 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>
> > Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> > [libxc pieces]
> 
> OK, so I just took the time to wrap my brain around this patch more, and
> I'm afraid I think it's almost entirely wrong. :-/
> 
> To sum up:
> 
> 1. There's no reason to pass the number of bits to xc_tbuf_set_cpu_mask.
>  The caller should just pass a fully-filled xc_cpumask_t.
> 
> 2. xentrace shouldn't rely on open-coded knowledge about the length of
> xc_cpumask_t; it should call xc_get_cpumask_size() and use that.
> 
> 3. If the user doesn't pass a mask, then the mask should just be left
> unchanged; it shouldn't silently go and set all the bits in the cpumask.

Which would be then an cpumask with zero CPUs set?
> 
> 4. Allocating something the size of a 32-bit word?
> 
> Attached is a patch I wrote when I was trying to figure out what I
> thought was the "right" thing to do here -- rather than try to make you
> re-write the patch, I'm just attaching it (and I'll paste it in-line as
> well).  (I've compiled it but not tested it.)
> 
> What do you think?

Below:

..snip..
> diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
> index 8a38e32..e35a131 100644
> --- a/tools/xentrace/xentrace.c
> +++ b/tools/xentrace/xentrace.c
> @@ -521,23 +521,64 @@ static struct t_struct *map_tbufs(unsigned long
> tbufs_mfn, unsigned int num,
>      return &tbufs;
>  }
> 
> +void print_cpu_mask(xc_cpumap_t map)
> +{
> +    unsigned int v, had_printed = 0;
> +    int i;
> +
> +    fprintf(stderr, "change cpumask to 0x");
> +
> +    for ( i = xc_get_cpumap_size(xc_handle); i >= 0; i-- )
> +    {
> +        v = map[i];
> +        if ( v || had_printed || !i ) {
> +            fprintf(stderr,"%x", v);
> +            had_printed = 1;

That (if had_printed) fprintf(stderr,"%02x", v);
is needed. Otherwise if you do something like -c 0x801
and it would print 'change cpumask to 0x81'

> +        }
> +   }
> +   fprintf(stderr, "\n");
> +}
> +
> +static void set_cpu_mask(uint32_t mask)
> +{
> +    int i, ret = 0;
> +    xc_cpumap_t map;
> +
> +    map = xc_cpumap_alloc(xc_handle);
> +    if ( !map )
> +        goto out;
> +
> +    /*
> +     * If mask is set, copy the bits out of it.  This still works for
> +     * systems with more than 32 cpus, as the shift will just shift
> +     * mask down to zero.
> +     */
> +    for ( i = 0; i < xc_get_cpumap_size(xc_handle) ; i++ )

The ';' has an space.
> +        map[i] = (mask >> (i*8)) & 0xff;

I was never sure of the right syntax for this so in my original patch I
had (mask >> (i * 8)) && 0xff;

To make it easier to read..
> +
> +    ret = xc_tbuf_set_cpu_mask(xc_handle, map);
> +    if ( ret != 0 )
> +        goto out;
> +
> +    print_cpu_mask(map);

free(map) ?

> +    return;
> +out:
> +    PERROR("Failure to get trace buffer pointer from Xen and set the
> new mask");
> +    exit(EXIT_FAILURE);
> +}

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

[George Dunlap]

On 03/30/2015 05:54 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 30, 2015 at 05:10:05PM +0100, George Dunlap wrote:
>> On 03/24/2015 03:39 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>
>>> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
>>> [libxc pieces]
>>
>> OK, so I just took the time to wrap my brain around this patch more, and
>> I'm afraid I think it's almost entirely wrong. :-/
>>
>> To sum up:
>>
>> 1. There's no reason to pass the number of bits to xc_tbuf_set_cpu_mask.
>>  The caller should just pass a fully-filled xc_cpumask_t.
>>
>> 2. xentrace shouldn't rely on open-coded knowledge about the length of
>> xc_cpumask_t; it should call xc_get_cpumask_size() and use that.
>>
>> 3. If the user doesn't pass a mask, then the mask should just be left
>> unchanged; it shouldn't silently go and set all the bits in the cpumask.
> 
> Which would be then an cpumask with zero CPUs set?

No -- what xentrace does at the moment: if there's no cpumask passed in,
just don't call xc_tbuf_set_cpu_mask() at all.  Leave it the way it was
when you found it.  When Xen boots will start with all cpus set; if a
previous caller has changed it, just leave it the way you found it.

I think the current behavior is fine, but my opinion isn't very strong.
 Feel free to try to make the case that the current UI is wrong and we
*should* set everything again by default.  But in that case, 1) it
should be a separate patch, 2) we should follow the same principle for
the evtmask.

>> +        map[i] = (mask >> (i*8)) & 0xff;
> 
> I was never sure of the right syntax for this so in my original patch I
> had (mask >> (i * 8)) && 0xff;

Hmm?  I just looked at the last two patches and they had '&'.  && is
logical and; it will give you either 0 or 1.


So do you want to take this patch and put it into your series (making
all the changes you suggest), or do you want me to polish it up and send
it separately?

 -George

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

[Konrad Rzeszutek Wilk]

On Mon, Mar 30, 2015 at 06:33:23PM +0100, George Dunlap wrote:
> On 03/30/2015 05:54 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Mar 30, 2015 at 05:10:05PM +0100, George Dunlap wrote:
> >> On 03/24/2015 03:39 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>
> >>> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> >>> [libxc pieces]
> >>
> >> OK, so I just took the time to wrap my brain around this patch more, and
> >> I'm afraid I think it's almost entirely wrong. :-/
> >>
> >> To sum up:
> >>
> >> 1. There's no reason to pass the number of bits to xc_tbuf_set_cpu_mask.
> >>  The caller should just pass a fully-filled xc_cpumask_t.
> >>
> >> 2. xentrace shouldn't rely on open-coded knowledge about the length of
> >> xc_cpumask_t; it should call xc_get_cpumask_size() and use that.
> >>
> >> 3. If the user doesn't pass a mask, then the mask should just be left
> >> unchanged; it shouldn't silently go and set all the bits in the cpumask.
> > 
> > Which would be then an cpumask with zero CPUs set?
> 
> No -- what xentrace does at the moment: if there's no cpumask passed in,
> just don't call xc_tbuf_set_cpu_mask() at all.  Leave it the way it was
> when you found it.  When Xen boots will start with all cpus set; if a
> previous caller has changed it, just leave it the way you found it.

Aah, I missed that.
> 
> I think the current behavior is fine, but my opinion isn't very strong.
>  Feel free to try to make the case that the current UI is wrong and we
> *should* set everything again by default.  But in that case, 1) it
> should be a separate patch, 2) we should follow the same principle for

Correct.
> the evtmask.
> 
> >> +        map[i] = (mask >> (i*8)) & 0xff;
> > 
> > I was never sure of the right syntax for this so in my original patch I
> > had (mask >> (i * 8)) && 0xff;
> 
> Hmm?  I just looked at the last two patches and they had '&'.  && is
> logical and; it will give you either 0 or 1.

Sorry, not '&&'.

It was the '(i * 8)' vs '(i*8)'

> 
> 
> So do you want to take this patch and put it into your series (making
> all the changes you suggest), or do you want me to polish it up and send
> it separately?

I will take it in, do the changes, and also test it on a large machine.
Thought should I wait until you are done looking at the other patch?
> 
>  -George

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

[George Dunlap]

On 03/30/2015 07:04 PM, Konrad Rzeszutek Wilk wrote:
>
> Sorry, not '&&'.

Phew. :-)

> It was the '(i * 8)' vs '(i*8)'

Yeah, as you can probably tell style isn't one of the things I'm very
particular about (at least compared with a lot of other developers).  I
think what you had originally was probably closer in line with what's in
xentrace.c.

>> So do you want to take this patch and put it into your series (making
>> all the changes you suggest), or do you want me to polish it up and send
>> it separately?
> 
> I will take it in, do the changes, and also test it on a large machine.
> Thought should I wait until you are done looking at the other patch?

Sure, I should have that done here shortly.

 -George

[PATCH v3 3/3] 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. Also it can include just
singular CPUs: -c <cpu1>, or ranges without an
start or end (and xentrace will figure out the values), such
as: -c -<cpu2> (which will include cpu0, cpu1, and cpu2) or
-c <cpu2>- (which will include cpu2 and up to MAX_CPUS).

That should make it easier to trace the right CPU if
using this along with 'xl vcpu-list'.

The code has been lifted from the Linux kernel, see file
lib/bitmap.c, function __bitmap_parselist.

To make the old behavior and the new function work, we check
to see if the arguments have '0x' in them. If they do
we use the old style parsing (limited to 32 CPUs). If that
does not exist we use the new parsing.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v4: Fix per George's review]
---
 tools/xentrace/xentrace.8 |  34 ++++++++-
 tools/xentrace/xentrace.c | 190 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 188 insertions(+), 36 deletions(-)

diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index c176a96..eb6fba8 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -36,10 +36,36 @@ all new records to the output
 set the time, p, (in milliseconds) to sleep between polling the buffers
 for new data.
 .TP
-.B -c, --cpu-mask=c
-set bitmask of CPUs to trace. It is limited to 32-bits.
-If not specified, the cpu-mask of all of the available CPUs will be
-constructed.
+.B -c, --cpu-mask=[\fIc\fP|\fICPU-LIST\fP]
+This can either be a hex value (of the form 0xNNNN...), or a set of cpu
+ranges as described below. Hex values are limited to 32 bits. If not
+specified, the cpu-mask of all of the available CPUs will be
+constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which
+may be specified as follows:
+
+.RS 4
+.ie n .IP """0-3""" 4
+.el .IP "``0-3''" 4
+.IX Item "0-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """0,2,5-7""" 4
+.el .IP "``0,2,5-7''" 4
+.IX Item "0,2,5-7"
+Trace only on CPUs 0, 2, and 5 through 7.
+.ie n .IP """-3""" 4
+.el .IP "``-3''" 4
+.IX Item "-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """-3,7""" 4
+.el .IP "``-3,7''" 4
+.IX Item "-3,7"
+Trace only on CPUs 0 through 3 and 7
+.ie n .IP """3-""" 4
+.el .IP "``3-''" 4
+.IX Item "-3-"
+Trace only on CPUs 3 up to maximum numbers of CPUs the host has.
+.RE
+.Sp
 
 .TP
 .B -e, --evt-mask=e
diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index 40504ec..3dd5c01 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -23,6 +23,7 @@
 #include <string.h>
 #include <getopt.h>
 #include <assert.h>
+#include <ctype.h>
 #include <sys/poll.h>
 #include <sys/statvfs.h>
 
@@ -54,6 +55,7 @@ typedef struct settings_st {
     uint32_t evt_mask;
     xc_cpumap_t cpu_mask;
     int cpu_bits;
+    char *cpu_mask_str;
     unsigned long tbuf_size;
     unsigned long disk_rsvd;
     unsigned long timeout;
@@ -545,25 +547,6 @@ void print_cpu_mask(xc_cpumap_t mask, int bits)
 
 static void set_cpu_mask(xc_cpumap_t mask, int bits)
 {
-    int i;
-
-    if ( bits <= 0 )
-    {
-        bits = xc_get_max_cpus(xc_handle);
-        if ( bits <= 0 )
-            goto out;
-    }
-    if ( !mask )
-    {
-        mask = xc_cpumap_alloc(xc_handle);
-        if ( !mask )
-            goto out;
-
-        /* Set it to include _all_ CPUs. */
-        for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ )
-            mask[i] = 0xff;
-    }
-    /* And this will limit it to the exact amount of bits. */
     if ( xc_tbuf_set_cpu_mask(xc_handle, mask, bits) )
         goto out;
 
@@ -822,7 +805,7 @@ static void usage(void)
 "Usage: xentrace [OPTION...] [output file]\n" \
 "Tool to capture Xen trace buffer data\n" \
 "\n" \
-"  -c, --cpu-mask=c        Set cpu-mask\n" \
+"  -c, --cpu-mask=c        Set cpu-mask, using either hex or CPU ranges\n" \
 "  -e, --evt-mask=e        Set evt-mask\n" \
 "  -s, --poll-sleep=p      Set sleep time, p, in milliseconds between\n" \
 "                          polling the trace buffer for new data\n" \
@@ -976,6 +959,156 @@ static int parse_cpumask(const char *arg)
     return 0;
 }
 
+#define ZERO_DIGIT '0'
+
+static int parse_cpumask_range(const char *arg)
+{
+    xc_cpumap_t map;
+    unsigned int a, b, buflen = strlen(arg);
+    int c, c_old, totaldigits, nmaskbits;
+    int in_range;
+    const char *s;
+
+    if ( !buflen )
+    {
+        fprintf(stderr, "Invalid option argument: %s\n", arg);
+        errno = EINVAL;
+        return EXIT_FAILURE;
+    }
+    nmaskbits = xc_get_max_cpus(xc_handle);
+    if ( nmaskbits <= 0 )
+    {
+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
+        return EXIT_FAILURE;
+    }
+    map = xc_cpumap_alloc(xc_handle);
+    if ( !map )
+    {
+        fprintf(stderr, "Out of memory!\n");
+        return EXIT_FAILURE;
+    }
+    c = c_old = totaldigits = 0;
+    s = arg;
+    do {
+        in_range = 0;
+        a = b = 0;
+        /* The buflen can become zero in the '} while(..) below. */
+        while ( buflen )
+        {
+            c_old = c;
+            c = *s++;
+            buflen--;
+
+            if ( isspace(c) )
+                continue;
+
+            if ( totaldigits && c && isspace(c_old) )
+            {
+                fprintf(stderr, "No embedded whitespaces allowed in: %s\n", arg);
+                goto err_out;
+            }
+
+            /* 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++;
+        }
+        /* Syntax: <digit>-[,] - expand to number of CPUs. */
+        if ( b == 0 && in_range && (c == '-' || c == ',') )
+            b = nmaskbits-1;
+
+        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 )
+        {
+            xc_cpumap_setcpu(a, map);
+            a++;
+        }
+    } while ( buflen && c == ',' );
+
+    opts.cpu_mask = map;
+    opts.cpu_bits = nmaskbits;
+    return 0;
+ err_out:
+    free(map);
+    errno = EINVAL;
+    return EXIT_FAILURE;
+}
+
+/**
+ * Figure out which of the CPU types the user has provided - either the hex
+ * variant or the cpu-list. Once done set the CPU mask.
+ */
+static int figure_cpu_mask(void)
+{
+    int ret = EXIT_FAILURE;
+
+    if ( opts.cpu_mask_str )
+    {
+        if ( strlen(opts.cpu_mask_str) < 1 )
+        {
+            errno = ENOSPC;
+            goto out;
+        }
+        if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )
+            ret = parse_cpumask(opts.cpu_mask_str);
+        else
+            ret = parse_cpumask_range(opts.cpu_mask_str);
+    }
+    else
+    {
+        int i, bits;
+        xc_cpumap_t mask;
+
+        bits = xc_get_max_cpus(xc_handle);
+        if ( bits <= 0 )
+            goto out;
+
+        mask = xc_cpumap_alloc(xc_handle);
+        if ( !mask )
+            goto out;
+
+        /* Set it to include _all_ CPUs. */
+        for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ )
+            mask[i] = 0xff;
+
+        opts.cpu_mask = mask;
+        opts.cpu_bits = bits;
+        ret = 0;
+    }
+    if ( ret != EXIT_FAILURE )
+        set_cpu_mask(opts.cpu_mask, opts.cpu_bits);
+ out:
+    /* We don't use it pass this point. */
+    free(opts.cpu_mask_str);
+    return ret;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -1006,15 +1139,9 @@ static void parse_args(int argc, char **argv)
             opts.poll_sleep = argtol(optarg, 0);
             break;
 
-        case 'c': /* set new cpu mask for filtering*/
-            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
-            if ( parse_cpumask(optarg) )
-            {
-                perror("Not enough memory!");
-                exit(EXIT_FAILURE);
-            }
+        case 'c': /* set new cpu mask for filtering (when xch is set). */
+            opts.cpu_mask_str = strdup(optarg);
             break;
-        
         case 'e': /* set new event mask for filtering*/
             parse_evtmask(optarg);
             break;
@@ -1079,6 +1206,7 @@ int main(int argc, char **argv)
     opts.evt_mask = 0;
     opts.cpu_mask = NULL;
     opts.cpu_bits = 0;
+    opts.cpu_mask_str = NULL;
     opts.disk_rsvd = 0;
     opts.disable_tracing = 1;
     opts.start_disabled = 0;
@@ -1096,10 +1224,8 @@ int main(int argc, char **argv)
     if ( opts.evt_mask != 0 )
         set_evt_mask(opts.evt_mask);
 
-
-    set_cpu_mask(opts.cpu_mask, opts.cpu_bits);
-    /* We don't use it pass this point. */
-    free(opts.cpu_mask);
+    if (figure_cpu_mask())
+        usage(); /* calls exit. */
 
     if ( opts.timeout != 0 ) 
         alarm(opts.timeout);
-- 
2.1.0

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

[George Dunlap]

On 03/24/2015 03:39 PM, Konrad Rzeszutek Wilk wrote:
> Instead of just using -c 0x<some hex value> we can
> also use -c <starting cpu>-<end cpu> or -c <cpu1>,<cpu2>
> or a combination of them. Also it can include just
> singular CPUs: -c <cpu1>, or ranges without an
> start or end (and xentrace will figure out the values), such
> as: -c -<cpu2> (which will include cpu0, cpu1, and cpu2) or
> -c <cpu2>- (which will include cpu2 and up to MAX_CPUS).
> 
> That should make it easier to trace the right CPU if
> using this along with 'xl vcpu-list'.
> 
> The code has been lifted from the Linux kernel, see file
> lib/bitmap.c, function __bitmap_parselist.
> 
> To make the old behavior and the new function work, we check
> to see if the arguments have '0x' in them. If they do
> we use the old style parsing (limited to 32 CPUs). If that
> does not exist we use the new parsing.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> [v4: Fix per George's review]
> ---
>  tools/xentrace/xentrace.8 |  34 ++++++++-
>  tools/xentrace/xentrace.c | 190 ++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 188 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
> index c176a96..eb6fba8 100644
> --- a/tools/xentrace/xentrace.8
> +++ b/tools/xentrace/xentrace.8
> @@ -36,10 +36,36 @@ all new records to the output
>  set the time, p, (in milliseconds) to sleep between polling the buffers
>  for new data.
>  .TP
> -.B -c, --cpu-mask=c
> -set bitmask of CPUs to trace. It is limited to 32-bits.
> -If not specified, the cpu-mask of all of the available CPUs will be
> -constructed.
> +.B -c, --cpu-mask=[\fIc\fP|\fICPU-LIST\fP]
> +This can either be a hex value (of the form 0xNNNN...), or a set of cpu
> +ranges as described below. Hex values are limited to 32 bits. If not
> +specified, the cpu-mask of all of the available CPUs will be
> +constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which
> +may be specified as follows:
> +
> +.RS 4
> +.ie n .IP """0-3""" 4
> +.el .IP "``0-3''" 4
> +.IX Item "0-3"
> +Trace only on CPUs 0 through 3
> +.ie n .IP """0,2,5-7""" 4
> +.el .IP "``0,2,5-7''" 4
> +.IX Item "0,2,5-7"
> +Trace only on CPUs 0, 2, and 5 through 7.
> +.ie n .IP """-3""" 4
> +.el .IP "``-3''" 4
> +.IX Item "-3"
> +Trace only on CPUs 0 through 3
> +.ie n .IP """-3,7""" 4
> +.el .IP "``-3,7''" 4
> +.IX Item "-3,7"
> +Trace only on CPUs 0 through 3 and 7
> +.ie n .IP """3-""" 4
> +.el .IP "``3-''" 4
> +.IX Item "-3-"
> +Trace only on CPUs 3 up to maximum numbers of CPUs the host has.
> +.RE
> +.Sp
>  
>  .TP
>  .B -e, --evt-mask=e
> diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
> index 40504ec..3dd5c01 100644
> --- a/tools/xentrace/xentrace.c
> +++ b/tools/xentrace/xentrace.c
> @@ -23,6 +23,7 @@
>  #include <string.h>
>  #include <getopt.h>
>  #include <assert.h>
> +#include <ctype.h>
>  #include <sys/poll.h>
>  #include <sys/statvfs.h>
>  
> @@ -54,6 +55,7 @@ typedef struct settings_st {
>      uint32_t evt_mask;
>      xc_cpumap_t cpu_mask;
>      int cpu_bits;
> +    char *cpu_mask_str;
>      unsigned long tbuf_size;
>      unsigned long disk_rsvd;
>      unsigned long timeout;
> @@ -545,25 +547,6 @@ void print_cpu_mask(xc_cpumap_t mask, int bits)
>  
>  static void set_cpu_mask(xc_cpumap_t mask, int bits)
>  {
> -    int i;
> -
> -    if ( bits <= 0 )
> -    {
> -        bits = xc_get_max_cpus(xc_handle);
> -        if ( bits <= 0 )
> -            goto out;
> -    }
> -    if ( !mask )
> -    {
> -        mask = xc_cpumap_alloc(xc_handle);
> -        if ( !mask )
> -            goto out;
> -
> -        /* Set it to include _all_ CPUs. */
> -        for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ )
> -            mask[i] = 0xff;
> -    }
> -    /* And this will limit it to the exact amount of bits. */
>      if ( xc_tbuf_set_cpu_mask(xc_handle, mask, bits) )
>          goto out;
>  
> @@ -822,7 +805,7 @@ static void usage(void)
>  "Usage: xentrace [OPTION...] [output file]\n" \
>  "Tool to capture Xen trace buffer data\n" \
>  "\n" \
> -"  -c, --cpu-mask=c        Set cpu-mask\n" \
> +"  -c, --cpu-mask=c        Set cpu-mask, using either hex or CPU ranges\n" \
>  "  -e, --evt-mask=e        Set evt-mask\n" \
>  "  -s, --poll-sleep=p      Set sleep time, p, in milliseconds between\n" \
>  "                          polling the trace buffer for new data\n" \
> @@ -976,6 +959,156 @@ static int parse_cpumask(const char *arg)
>      return 0;
>  }
>  
> +#define ZERO_DIGIT '0'
> +
> +static int parse_cpumask_range(const char *arg)
> +{
> +    xc_cpumap_t map;
> +    unsigned int a, b, buflen = strlen(arg);
> +    int c, c_old, totaldigits, nmaskbits;
> +    int in_range;
> +    const char *s;
> +
> +    if ( !buflen )
> +    {
> +        fprintf(stderr, "Invalid option argument: %s\n", arg);
> +        errno = EINVAL;
> +        return EXIT_FAILURE;
> +    }

I think we need blank lines between these if() statements

> +    nmaskbits = xc_get_max_cpus(xc_handle);

[space]

> +    if ( nmaskbits <= 0 )
> +    {
> +        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
> +        return EXIT_FAILURE;
> +    }

[space]

> +    map = xc_cpumap_alloc(xc_handle);

[space] &c

> +    if ( !map )
> +    {
> +        fprintf(stderr, "Out of memory!\n");
> +        return EXIT_FAILURE;
> +    }
> +    c = c_old = totaldigits = 0;
> +    s = arg;
> +    do {
> +        in_range = 0;
> +        a = b = 0;
> +        /* The buflen can become zero in the '} while(..) below. */
> +        while ( buflen )
> +        {
> +            c_old = c;
> +            c = *s++;
> +            buflen--;
> +
> +            if ( isspace(c) )
> +                continue;
> +
> +            if ( totaldigits && c && isspace(c_old) )
> +            {
> +                fprintf(stderr, "No embedded whitespaces allowed in: %s\n", arg);
> +                goto err_out;
> +            }

Why are we even bothering to handle spaces here?  Who would put
  xentrace -c "    0-3,5" /tmp/foo
and why would we want to accept that input?

Not handling initial whitespace lets us get rid of totaldigits and c_old
as well.

> +
> +            /* A '\0' or a ',' signal the end of a cpu# or range */
> +            if ( c == '\0' || c == ',' )
> +                break;

Can c=='\0' ever be true here?  Isn't that why we do all the accounting
w/ buflen?

[snip]

> +
> +/**
> + * Figure out which of the CPU types the user has provided - either the hex
> + * variant or the cpu-list. Once done set the CPU mask.
> + */
> +static int figure_cpu_mask(void)
> +{
> +    int ret = EXIT_FAILURE;
> +
> +    if ( opts.cpu_mask_str )

I think we should move this if into main(), similar to opts.evt_mask.

> +    {
> +        if ( strlen(opts.cpu_mask_str) < 1 )
> +        {
> +            errno = ENOSPC;
> +            goto out;
> +        }
> +        if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )
> +            ret = parse_cpumask(opts.cpu_mask_str);
> +        else
> +            ret = parse_cpumask_range(opts.cpu_mask_str);

Would it make sense to add an "all" option here?

Also -- I think it might make sense to allocate the cpumask in this
function, and then have parse_cpumask* set the bits it wants.  Then we
have a nice symmetric alloc and free.

> +    }
> +    else

And we can get rid of this else.

> +    {
> +        int i, bits;
> +        xc_cpumap_t mask;
> +
> +        bits = xc_get_max_cpus(xc_handle);
> +        if ( bits <= 0 )
> +            goto out;
> +
> +        mask = xc_cpumap_alloc(xc_handle);
> +        if ( !mask )
> +            goto out;
> +
> +        /* Set it to include _all_ CPUs. */
> +        for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ )
> +            mask[i] = 0xff;
> +
> +        opts.cpu_mask = mask;
> +        opts.cpu_bits = bits;
> +        ret = 0;
> +    }
> +    if ( ret != EXIT_FAILURE )
> +        set_cpu_mask(opts.cpu_mask, opts.cpu_bits);
> + out:
> +    /* We don't use it pass this point. */
> +    free(opts.cpu_mask_str);

I guess we also want to free opts.cpu_mask

> +    return ret;
> +}
> +
>  /* parse command line arguments */
>  static void parse_args(int argc, char **argv)
>  {
> @@ -1006,15 +1139,9 @@ static void parse_args(int argc, char **argv)
>              opts.poll_sleep = argtol(optarg, 0);
>              break;
>  
> -        case 'c': /* set new cpu mask for filtering*/
> -            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
> -            if ( parse_cpumask(optarg) )
> -            {
> -                perror("Not enough memory!");
> -                exit(EXIT_FAILURE);
> -            }
> +        case 'c': /* set new cpu mask for filtering (when xch is set). */
> +            opts.cpu_mask_str = strdup(optarg);
>              break;
> -        
>          case 'e': /* set new event mask for filtering*/
>              parse_evtmask(optarg);
>              break;
> @@ -1079,6 +1206,7 @@ int main(int argc, char **argv)
>      opts.evt_mask = 0;
>      opts.cpu_mask = NULL;
>      opts.cpu_bits = 0;
> +    opts.cpu_mask_str = NULL;
>      opts.disk_rsvd = 0;
>      opts.disable_tracing = 1;
>      opts.start_disabled = 0;
> @@ -1096,10 +1224,8 @@ int main(int argc, char **argv)
>      if ( opts.evt_mask != 0 )
>          set_evt_mask(opts.evt_mask);
>  
> -
> -    set_cpu_mask(opts.cpu_mask, opts.cpu_bits);
> -    /* We don't use it pass this point. */
> -    free(opts.cpu_mask);
> +    if (figure_cpu_mask())
> +        usage(); /* calls exit. */

if (opts.cpu_mask_string)
 figure_cpu_mask();

Thanks,
 -George

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

[Konrad Rzeszutek Wilk]

On Tue, Mar 31, 2015 at 12:31:34PM +0100, George Dunlap wrote:
> On 03/24/2015 03:39 PM, Konrad Rzeszutek Wilk wrote:
> > Instead of just using -c 0x<some hex value> we can
> > also use -c <starting cpu>-<end cpu> or -c <cpu1>,<cpu2>
> > or a combination of them. Also it can include just
> > singular CPUs: -c <cpu1>, or ranges without an
> > start or end (and xentrace will figure out the values), such
> > as: -c -<cpu2> (which will include cpu0, cpu1, and cpu2) or
> > -c <cpu2>- (which will include cpu2 and up to MAX_CPUS).
> > 
> > That should make it easier to trace the right CPU if
> > using this along with 'xl vcpu-list'.
> > 
> > The code has been lifted from the Linux kernel, see file
> > lib/bitmap.c, function __bitmap_parselist.
> > 
> > To make the old behavior and the new function work, we check
> > to see if the arguments have '0x' in them. If they do
> > we use the old style parsing (limited to 32 CPUs). If that
> > does not exist we use the new parsing.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > [v4: Fix per George's review]
> > ---
> >  tools/xentrace/xentrace.8 |  34 ++++++++-
> >  tools/xentrace/xentrace.c | 190 ++++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 188 insertions(+), 36 deletions(-)
> > 
> > diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
> > index c176a96..eb6fba8 100644
> > --- a/tools/xentrace/xentrace.8
> > +++ b/tools/xentrace/xentrace.8
> > @@ -36,10 +36,36 @@ all new records to the output
> >  set the time, p, (in milliseconds) to sleep between polling the buffers
> >  for new data.
> >  .TP
> > -.B -c, --cpu-mask=c
> > -set bitmask of CPUs to trace. It is limited to 32-bits.
> > -If not specified, the cpu-mask of all of the available CPUs will be
> > -constructed.
> > +.B -c, --cpu-mask=[\fIc\fP|\fICPU-LIST\fP]
> > +This can either be a hex value (of the form 0xNNNN...), or a set of cpu
> > +ranges as described below. Hex values are limited to 32 bits. If not
> > +specified, the cpu-mask of all of the available CPUs will be
> > +constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which
> > +may be specified as follows:
> > +
> > +.RS 4
> > +.ie n .IP """0-3""" 4
> > +.el .IP "``0-3''" 4
> > +.IX Item "0-3"
> > +Trace only on CPUs 0 through 3
> > +.ie n .IP """0,2,5-7""" 4
> > +.el .IP "``0,2,5-7''" 4
> > +.IX Item "0,2,5-7"
> > +Trace only on CPUs 0, 2, and 5 through 7.
> > +.ie n .IP """-3""" 4
> > +.el .IP "``-3''" 4
> > +.IX Item "-3"
> > +Trace only on CPUs 0 through 3
> > +.ie n .IP """-3,7""" 4
> > +.el .IP "``-3,7''" 4
> > +.IX Item "-3,7"
> > +Trace only on CPUs 0 through 3 and 7
> > +.ie n .IP """3-""" 4
> > +.el .IP "``3-''" 4
> > +.IX Item "-3-"
> > +Trace only on CPUs 3 up to maximum numbers of CPUs the host has.
> > +.RE
> > +.Sp
> >  
> >  .TP
> >  .B -e, --evt-mask=e
> > diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
> > index 40504ec..3dd5c01 100644
> > --- a/tools/xentrace/xentrace.c
> > +++ b/tools/xentrace/xentrace.c
> > @@ -23,6 +23,7 @@
> >  #include <string.h>
> >  #include <getopt.h>
> >  #include <assert.h>
> > +#include <ctype.h>
> >  #include <sys/poll.h>
> >  #include <sys/statvfs.h>
> >  
> > @@ -54,6 +55,7 @@ typedef struct settings_st {
> >      uint32_t evt_mask;
> >      xc_cpumap_t cpu_mask;
> >      int cpu_bits;
> > +    char *cpu_mask_str;
> >      unsigned long tbuf_size;
> >      unsigned long disk_rsvd;
> >      unsigned long timeout;
> > @@ -545,25 +547,6 @@ void print_cpu_mask(xc_cpumap_t mask, int bits)
> >  
> >  static void set_cpu_mask(xc_cpumap_t mask, int bits)
> >  {
> > -    int i;
> > -
> > -    if ( bits <= 0 )
> > -    {
> > -        bits = xc_get_max_cpus(xc_handle);
> > -        if ( bits <= 0 )
> > -            goto out;
> > -    }
> > -    if ( !mask )
> > -    {
> > -        mask = xc_cpumap_alloc(xc_handle);
> > -        if ( !mask )
> > -            goto out;
> > -
> > -        /* Set it to include _all_ CPUs. */
> > -        for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ )
> > -            mask[i] = 0xff;
> > -    }
> > -    /* And this will limit it to the exact amount of bits. */
> >      if ( xc_tbuf_set_cpu_mask(xc_handle, mask, bits) )
> >          goto out;
> >  
> > @@ -822,7 +805,7 @@ static void usage(void)
> >  "Usage: xentrace [OPTION...] [output file]\n" \
> >  "Tool to capture Xen trace buffer data\n" \
> >  "\n" \
> > -"  -c, --cpu-mask=c        Set cpu-mask\n" \
> > +"  -c, --cpu-mask=c        Set cpu-mask, using either hex or CPU ranges\n" \
> >  "  -e, --evt-mask=e        Set evt-mask\n" \
> >  "  -s, --poll-sleep=p      Set sleep time, p, in milliseconds between\n" \
> >  "                          polling the trace buffer for new data\n" \
> > @@ -976,6 +959,156 @@ static int parse_cpumask(const char *arg)
> >      return 0;
> >  }
> >  
> > +#define ZERO_DIGIT '0'
> > +
> > +static int parse_cpumask_range(const char *arg)
> > +{
> > +    xc_cpumap_t map;
> > +    unsigned int a, b, buflen = strlen(arg);
> > +    int c, c_old, totaldigits, nmaskbits;
> > +    int in_range;
> > +    const char *s;
> > +
> > +    if ( !buflen )
> > +    {
> > +        fprintf(stderr, "Invalid option argument: %s\n", arg);
> > +        errno = EINVAL;
> > +        return EXIT_FAILURE;
> > +    }
> 
> I think we need blank lines between these if() statements
> 
> > +    nmaskbits = xc_get_max_cpus(xc_handle);
> 
> [space]
> 
> > +    if ( nmaskbits <= 0 )
> > +    {
> > +        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
> > +        return EXIT_FAILURE;
> > +    }
> 
> [space]
> 
> > +    map = xc_cpumap_alloc(xc_handle);
> 
> [space] &c
> 
> > +    if ( !map )
> > +    {
> > +        fprintf(stderr, "Out of memory!\n");
> > +        return EXIT_FAILURE;
> > +    }
> > +    c = c_old = totaldigits = 0;
> > +    s = arg;
> > +    do {
> > +        in_range = 0;
> > +        a = b = 0;
> > +        /* The buflen can become zero in the '} while(..) below. */
> > +        while ( buflen )
> > +        {
> > +            c_old = c;
> > +            c = *s++;
> > +            buflen--;
> > +
> > +            if ( isspace(c) )
> > +                continue;
> > +
> > +            if ( totaldigits && c && isspace(c_old) )
> > +            {
> > +                fprintf(stderr, "No embedded whitespaces allowed in: %s\n", arg);
> > +                goto err_out;
> > +            }
> 
> Why are we even bothering to handle spaces here?  Who would put
>   xentrace -c "    0-3,5" /tmp/foo
> and why would we want to accept that input?

This checks that and exits out. We would _not_ accept this input.

Are you saying that before going in this function I should check
that opts.cpu_mask_start starts with an space? But what if the string is:
 xentrace -c "0-   3,5".

This check will catch it.

Or should I have an check before this function that does:

 for ( i = 0; i < strlen(opts.cpu_mask_str); i++ )
	if (isspace(opts.cpu_mask_str[i]))
		goto err_out;

?
> 
> Not handling initial whitespace lets us get rid of totaldigits and c_old
> as well.
> 
> > +
> > +            /* A '\0' or a ',' signal the end of a cpu# or range */
> > +            if ( c == '\0' || c == ',' )
> > +                break;
> 
> Can c=='\0' ever be true here?  Isn't that why we do all the accounting

Yes.
> w/ buflen?

At the start of the loop we decrease buflen by one which means it can
get to zero - so on next iteration we will exit. However, if we do not
have this check, then:

 993             if ( !isdigit(c) )                                                  
 994             {                                                                   
 995                 fprintf(stderr, "Only digits allowed in: %s\n", arg);           
 996                 goto err_out;                                                   
 997             }                       

hit that and error out. We could move this check higher up, but would
have to special case the ",", "-". And we would still have the
conditionals on those characters.

> 
> [snip]
> 
> > +
> > +/**
> > + * Figure out which of the CPU types the user has provided - either the hex
> > + * variant or the cpu-list. Once done set the CPU mask.
> > + */
> > +static int figure_cpu_mask(void)
> > +{
> > +    int ret = EXIT_FAILURE;
> > +
> > +    if ( opts.cpu_mask_str )
> 
> I think we should move this if into main(), similar to opts.evt_mask.

Yes.
> 
> > +    {
> > +        if ( strlen(opts.cpu_mask_str) < 1 )
> > +        {
> > +            errno = ENOSPC;
> > +            goto out;
> > +        }
> > +        if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )
> > +            ret = parse_cpumask(opts.cpu_mask_str);
> > +        else
> > +            ret = parse_cpumask_range(opts.cpu_mask_str);
> 
> Would it make sense to add an "all" option here?

Done.
> 
> Also -- I think it might make sense to allocate the cpumask in this
> function, and then have parse_cpumask* set the bits it wants.  Then we
> have a nice symmetric alloc and free.
> 
> > +    }
> > +    else
> 
> And we can get rid of this else.
> 
> > +    {
> > +        int i, bits;
> > +        xc_cpumap_t mask;
> > +
> > +        bits = xc_get_max_cpus(xc_handle);
> > +        if ( bits <= 0 )
> > +            goto out;
> > +
> > +        mask = xc_cpumap_alloc(xc_handle);
> > +        if ( !mask )
> > +            goto out;
> > +
> > +        /* Set it to include _all_ CPUs. */
> > +        for ( i = 0; i < XC_DIV_ROUND_UP(bits, 8); i++ )
> > +            mask[i] = 0xff;
> > +
> > +        opts.cpu_mask = mask;
> > +        opts.cpu_bits = bits;
> > +        ret = 0;
> > +    }
> > +    if ( ret != EXIT_FAILURE )
> > +        set_cpu_mask(opts.cpu_mask, opts.cpu_bits);
> > + out:
> > +    /* We don't use it pass this point. */
> > +    free(opts.cpu_mask_str);
> 
> I guess we also want to free opts.cpu_mask

Aye.
> 
> > +    return ret;
> > +}
> > +
> >  /* parse command line arguments */
> >  static void parse_args(int argc, char **argv)
> >  {
> > @@ -1006,15 +1139,9 @@ static void parse_args(int argc, char **argv)
> >              opts.poll_sleep = argtol(optarg, 0);
> >              break;
> >  
> > -        case 'c': /* set new cpu mask for filtering*/
> > -            /* Set opts.cpu_mask later as we don't have 'xch' set yet. */
> > -            if ( parse_cpumask(optarg) )
> > -            {
> > -                perror("Not enough memory!");
> > -                exit(EXIT_FAILURE);
> > -            }
> > +        case 'c': /* set new cpu mask for filtering (when xch is set). */
> > +            opts.cpu_mask_str = strdup(optarg);
> >              break;
> > -        
> >          case 'e': /* set new event mask for filtering*/
> >              parse_evtmask(optarg);
> >              break;
> > @@ -1079,6 +1206,7 @@ int main(int argc, char **argv)
> >      opts.evt_mask = 0;
> >      opts.cpu_mask = NULL;
> >      opts.cpu_bits = 0;
> > +    opts.cpu_mask_str = NULL;
> >      opts.disk_rsvd = 0;
> >      opts.disable_tracing = 1;
> >      opts.start_disabled = 0;
> > @@ -1096,10 +1224,8 @@ int main(int argc, char **argv)
> >      if ( opts.evt_mask != 0 )
> >          set_evt_mask(opts.evt_mask);
> >  
> > -
> > -    set_cpu_mask(opts.cpu_mask, opts.cpu_bits);
> > -    /* We don't use it pass this point. */
> > -    free(opts.cpu_mask);
> > +    if (figure_cpu_mask())
> > +        usage(); /* calls exit. */
> 
> if (opts.cpu_mask_string)
>  figure_cpu_mask();
>

Based on your previous patch this is what it became. I will be shortly
testing it.
>From c4a655e9645ee07af7f6186437ac5cd4316900bd Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 3 Apr 2015 14:45:57 -0400
Subject: [PATCH] xentrace: Implement cpu mask range parsing of human values
 (-c).

Instead of just using -c 0x<some hex value> we can
also use: -c <starting cpu>-<end cpu>, -c <cpu1>,<cpu2>, or a
combination of them, or 'all' for all cpus.

This new format can include just singular CPUs: -c <cpu1>,
or ranges without an start or end (and xentrace will figure out
the values), such as: -c -<cpu2> (which will include cpu0, cpu1,
and cpu2) or -c <cpu2>- (which will include cpu2 and up to MAX_CPUS).

That should make it easier to trace the right CPU if
using this along with 'xl vcpu-list'.

The code has been lifted from the Linux kernel, see file
lib/bitmap.c, function __bitmap_parselist.

To make the old behavior and the new function work, we check
to see if the arguments have '0x' in them. If they do
we use the old style parsing (limited to 32 CPUs). If that
does not exist we use the new parsing.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
[v4: Fix per George's review]
[v5: Redo per George's review]
---
 tools/xentrace/xentrace.8 |  34 +++++++-
 tools/xentrace/xentrace.c | 196 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 195 insertions(+), 35 deletions(-)

diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index ac18e9f..7b3172b 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -36,8 +36,38 @@ 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|\fIall\fP]
+This can be: a hex value (of the form 0xNNNN...), or a set of cpu
+ranges as described below, or the string \fIall\fP. Hex values are limited
+to 32 bits. If not specified, the cpu-mask as set during bootup will be
+constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which
+may be specified as follows:
+
+.RS 4
+.ie n .IP """0-3""" 4
+.el .IP "``0-3''" 4
+.IX Item "0-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """0,2,5-7""" 4
+.el .IP "``0,2,5-7''" 4
+.IX Item "0,2,5-7"
+Trace only on CPUs 0, 2, and 5 through 7.
+.ie n .IP """-3""" 4
+.el .IP "``-3''" 4
+.IX Item "-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """-3,7""" 4
+.el .IP "``-3,7''" 4
+.IX Item "-3,7"
+Trace only on CPUs 0 through 3 and 7
+.ie n .IP """3-""" 4
+.el .IP "``3-''" 4
+.IX Item "-3-"
+Trace only on CPUs 3 up to maximum numbers of CPUs the host has.
+.RE
+.Sp
+
+If using \fIall\fP it will use all of the CPUs the host has.
 .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 52278e3..c4d494d 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>
 
@@ -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;
+    char *cpu_mask_str;
     unsigned long tbuf_size;
     unsigned long disk_rsvd;
     unsigned long timeout;
@@ -542,33 +543,17 @@ void print_cpu_mask(xc_cpumap_t map)
    fprintf(stderr, "\n");
 }
 
-static void set_cpu_mask(uint32_t mask)
+static int set_cpu_mask(xc_cpumap_t map)
 {
-    int i, ret = 0;
-    xc_cpumap_t map;
-
-    map = xc_cpumap_alloc(xc_handle);
-    if ( !map )
-        goto out;
-
-    /*
-     * If mask is set, copy the bits out of it.  This still works for
-     * systems with more than 32 cpus, as the shift will just shift
-     * mask down to zero.
-     */
-    for ( i = 0; i < xc_get_cpumap_size(xc_handle) ; i++ )
-        map[i] = (mask >> (i * 8)) & 0xff;
-
-    ret = xc_tbuf_set_cpu_mask(xc_handle, map);
-    if ( ret != 0 )
-        goto out;
+    int ret = xc_tbuf_set_cpu_mask(xc_handle, map);
 
-    print_cpu_mask(map);
-    free(map);
-    return;
-out:
+    if ( ret == 0 )
+    {
+        print_cpu_mask(map);
+        return 0;
+    }
     PERROR("Failure to get trace buffer pointer from Xen and set the new mask");
-    exit(EXIT_FAILURE);
+    return EXIT_FAILURE;
 }
 
 /**
@@ -819,7 +804,8 @@ 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, CPU ranges, or\n" \
+"                          for all CPUs\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" \
@@ -951,6 +937,149 @@ static int parse_evtmask(char *arg)
     return 0;
 }
 
+#define ZERO_DIGIT '0'
+
+static int parse_cpumask_range(const char *arg, xc_cpumap_t map)
+{
+    unsigned int a, b, buflen = strlen(arg);
+    int c, c_old, totaldigits, nmaskbits;
+    int in_range;
+    const char *s;
+
+    if ( !buflen )
+    {
+        fprintf(stderr, "Invalid option argument: %s\n", arg);
+        errno = EINVAL;
+        return EXIT_FAILURE;
+    }
+
+    nmaskbits = xc_get_max_cpus(xc_handle);
+    if ( nmaskbits <= 0 )
+    {
+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
+        return EXIT_FAILURE;
+    }
+
+    c = c_old = totaldigits = 0;
+    s = arg;
+    do {
+        in_range = 0;
+        a = b = 0;
+        /* The buflen can become zero in the '} while(..) below. */
+        while ( buflen )
+        {
+            c_old = c;
+            c = *s++;
+            buflen--;
+
+            if ( isspace(c) )
+                continue;
+
+            if ( totaldigits && c && isspace(c_old) )
+            {
+                fprintf(stderr, "No embedded whitespaces allowed in: %s\n", arg);
+                goto err_out;
+            }
+
+            /* 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++;
+        }
+        /* Syntax: <digit>-[,] - expand to number of CPUs. */
+        if ( b == 0 && in_range && (c == '-' || c == ',') )
+            b = nmaskbits-1;
+
+        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 )
+        {
+            xc_cpumap_setcpu(a, map);
+            a++;
+        }
+    } while ( buflen && c == ',' );
+
+    return 0;
+ err_out:
+    errno = EINVAL;
+    return EXIT_FAILURE;
+}
+
+/**
+ * Figure out which of the CPU types the user has provided - either the hex
+ * variant, the cpu-list or 'all'. Once done set the CPU mask.
+ */
+static int figure_cpu_mask(void)
+{
+    int i, ret = EXIT_FAILURE;
+    xc_cpumap_t map;
+
+    map = xc_cpumap_alloc(xc_handle);
+    if ( !map )
+        goto out;
+
+    if ( strlen(opts.cpu_mask_str) < 1 )
+    {
+        errno = ENOSPC;
+        goto out;
+    }
+
+    ret = 0;
+    if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )
+    {
+        uint32_t v;
+
+        v = argtol(opts.cpu_mask_str, 0);
+        /*
+         * If mask is set, copy the bits out of it.  This still works for
+         * systems with more than 32 cpus, as the shift will just shift
+         * mask down to zero.
+         */
+        for ( i = 0; i < sizeof(uint32_t); i++ )
+            map[i] = (v >> (i * 8)) & 0xff;
+    }
+    else if ( strncmp("all", opts.cpu_mask_str, 3) == 0 )
+    {
+        for ( i = 0; i < xc_get_cpumap_size(xc_handle); i++ )
+            map[i] = 0xff;
+    }
+    else
+            ret = parse_cpumask_range(opts.cpu_mask_str, map);
+
+    if ( !ret )
+        ret = set_cpu_mask(map);
+ out:
+    /* We don't use them pass this point. */
+    free(map);
+    free(opts.cpu_mask_str);
+    return ret;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -981,10 +1110,9 @@ static void parse_args(int argc, char **argv)
             opts.poll_sleep = argtol(optarg, 0);
             break;
 
-        case 'c': /* set new cpu mask for filtering*/
-            opts.cpu_mask = argtol(optarg, 0);
+        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;
@@ -1047,7 +1175,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_str = NULL;
     opts.disk_rsvd = 0;
     opts.disable_tracing = 1;
     opts.start_disabled = 0;
@@ -1065,9 +1193,11 @@ int main(int argc, char **argv)
     if ( opts.evt_mask != 0 )
         set_evt_mask(opts.evt_mask);
 
-
-    if ( opts.cpu_mask != 0 )
-        set_cpu_mask(opts.cpu_mask);
+    if ( opts.cpu_mask_str )
+    {
+        if ( figure_cpu_mask() )
+            exit(EXIT_FAILURE);
+    }
 
     if ( opts.timeout != 0 ) 
         alarm(opts.timeout);
-- 
2.1.0

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

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 12496 bytes --]

On Fri, Apr 3, 2015 at 8:34 PM, Konrad Rzeszutek Wilk <konrad@darnok.org> wrote:
>> > +    if ( !map )
>> > +    {
>> > +        fprintf(stderr, "Out of memory!\n");
>> > +        return EXIT_FAILURE;
>> > +    }
>> > +    c = c_old = totaldigits = 0;
>> > +    s = arg;
>> > +    do {
>> > +        in_range = 0;
>> > +        a = b = 0;
>> > +        /* The buflen can become zero in the '} while(..) below. */
>> > +        while ( buflen )
>> > +        {
>> > +            c_old = c;
>> > +            c = *s++;
>> > +            buflen--;
>> > +
>> > +            if ( isspace(c) )
>> > +                continue;
>> > +
>> > +            if ( totaldigits && c && isspace(c_old) )
>> > +            {
>> > +                fprintf(stderr, "No embedded whitespaces allowed in: %s\n", arg);
>> > +                goto err_out;
>> > +            }
>>
>> Why are we even bothering to handle spaces here?  Who would put
>>   xentrace -c "    0-3,5" /tmp/foo
>> and why would we want to accept that input?
>
> This checks that and exits out. We would _not_ accept this input.

It doesn't look like it to me -- it looks like this whole "c_old"
stuff is meant to tolerate (i.e., skip) spaces at the beginning of the
cpumask string.  AFAIK the only way to actually do that practically
would be to add quotes with spaces, like above.

> Are you saying that before going in this function I should check
> that opts.cpu_mask_start starts with an space? But what if the string is:
>  xentrace -c "0-   3,5".
>
> This check will catch it.
>
> Or should I have an check before this function that does:
>
>  for ( i = 0; i < strlen(opts.cpu_mask_str); i++ )
>         if (isspace(opts.cpu_mask_str[i]))
>                 goto err_out;
>
> ?

No, that's the opposite of what I'm saying.  I'm saying we should
accept only the characters [0-9-,] in a cpumask string; the existence
of any other character -- even at the beginning -- should return an
error.  If we take out the whole "c_old" and "isspace()" checking
entirely, this will fall out naturally when it hits the "!is_digit(c)"
below.

>>
>> Not handling initial whitespace lets us get rid of totaldigits and c_old
>> as well.
>>
>> > +
>> > +            /* A '\0' or a ',' signal the end of a cpu# or range */
>> > +            if ( c == '\0' || c == ',' )
>> > +                break;
>>
>> Can c=='\0' ever be true here?  Isn't that why we do all the accounting
>
> Yes.
>> w/ buflen?
>
> At the start of the loop we decrease buflen by one which means it can
> get to zero - so on next iteration we will exit. However, if we do not
> have this check, then:
>
>  993             if ( !isdigit(c) )
>  994             {
>  995                 fprintf(stderr, "Only digits allowed in: %s\n", arg);
>  996                 goto err_out;
>  997             }
>
> hit that and error out. We could move this check higher up, but would
> have to special case the ",", "-". And we would still have the
> conditionals on those characters.

Right.  Well I think all that is unnecessarily hard to understand,
primarily because it's organized in a confusing way.

Rather than try to nitpick you into doing it the way that seems more
sensible to me, I've rewritten the patch a bit below.  This time I've
actually done some basic testing with it.  What do you think?

 -George

    xentrace: Implement cpu mask range parsing of human values (-c).

    Instead of just using -c 0x<some hex value> we can
    also use: -c <starting cpu>-<end cpu>, -c <cpu1>,<cpu2>, or a
    combination of them, or 'all' for all cpus.

    This new format can include just singular CPUs: -c <cpu1>,
    or ranges without an start or end (and xentrace will figure out
    the values), such as: -c -<cpu2> (which will include cpu0, cpu1,
    and cpu2) or -c <cpu2>- (which will include cpu2 and up to MAX_CPUS).

    That should make it easier to trace the right CPU if
    using this along with 'xl vcpu-list'.

    The code has been lifted from the Linux kernel, see file
    lib/bitmap.c, function __bitmap_parselist.

    To make the old behavior and the new function work, we check
    to see if the arguments have '0x' in them. If they do
    we use the old style parsing (limited to 32 CPUs). If that
    does not exist we use the new parsing.

    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
    ---
    v5:
     - Simplify parse_cpu_range
     - Rename "parse_cpu_mask" to parse_cpu_mask

diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index ac18e9f..7b3172b 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -36,8 +36,38 @@ 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|\fIall\fP]
+This can be: a hex value (of the form 0xNNNN...), or a set of cpu
+ranges as described below, or the string \fIall\fP. Hex values are limited
+to 32 bits. If not specified, the cpu-mask as set during bootup will be
+constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which
+may be specified as follows:
+
+.RS 4
+.ie n .IP """0-3""" 4
+.el .IP "``0-3''" 4
+.IX Item "0-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """0,2,5-7""" 4
+.el .IP "``0,2,5-7''" 4
+.IX Item "0,2,5-7"
+Trace only on CPUs 0, 2, and 5 through 7.
+.ie n .IP """-3""" 4
+.el .IP "``-3''" 4
+.IX Item "-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """-3,7""" 4
+.el .IP "``-3,7''" 4
+.IX Item "-3,7"
+Trace only on CPUs 0 through 3 and 7
+.ie n .IP """3-""" 4
+.el .IP "``3-''" 4
+.IX Item "-3-"
+Trace only on CPUs 3 up to maximum numbers of CPUs the host has.
+.RE
+.Sp
+
+If using \fIall\fP it will use all of the CPUs the host has.
 .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 8b40e88..1558496 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>

@@ -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;
+    char *cpu_mask_str;
     unsigned long tbuf_size;
     unsigned long disk_rsvd;
     unsigned long timeout;
@@ -542,33 +543,17 @@ void print_cpu_mask(xc_cpumap_t map)
    fprintf(stderr, "\n");
 }

-static void set_cpu_mask(uint32_t mask)
+static int set_cpu_mask(xc_cpumap_t map)
 {
-    int i, ret = 0;
-    xc_cpumap_t map;
-
-    map = xc_cpumap_alloc(xc_handle);
-    if ( !map )
-        goto out;
+    int ret = xc_tbuf_set_cpu_mask(xc_handle, map);

-    /*
-     * If mask is set, copy the bits out of it.  This still works for
-     * systems with more than 32 cpus, as the shift will just shift
-     * mask down to zero.
-     */
-    for ( i = 0; i < xc_get_cpumap_size(xc_handle); i++ )
-        map[i] = (mask >> (i * 8)) & 0xff;
-
-    ret = xc_tbuf_set_cpu_mask(xc_handle, map);
-    if ( ret != 0 )
-        goto out;
-
-    print_cpu_mask(map);
-    free(map);
-    return;
-out:
+    if ( ret == 0 )
+    {
+        print_cpu_mask(map);
+        return 0;
+    }
     PERROR("Failure to get trace buffer pointer from Xen and set the
new mask");
-    exit(EXIT_FAILURE);
+    return EXIT_FAILURE;
 }

 /**
@@ -819,7 +804,8 @@ 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, CPU ranges, or\n" \
+"                          for all CPUs\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" \
@@ -951,6 +937,134 @@ static int parse_evtmask(char *arg)
     return 0;
 }

+#define ZERO_DIGIT '0'
+
+#define is_terminator(c) ((c)=='\0' || (c)==',')
+
+static int parse_cpumask_range(const char *mask_str, xc_cpumap_t map)
+{
+    unsigned int a, b;
+    int nmaskbits;
+    char c;
+    int in_range;
+    const char *s;
+
+    nmaskbits = xc_get_max_cpus(xc_handle);
+    if ( nmaskbits <= 0 )
+    {
+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n",
nmaskbits);
+        return EXIT_FAILURE;
+    }
+
+    c = 0;
+    s = mask_str;
+    do {
+        in_range = 0;
+        a = b = 0;
+
+        /* Process until we find a range terminator */
+        for(c=*s++; !is_terminator(c); c=*s++)
+        {
+            if ( c == '-' )
+            {
+                if ( in_range )
+                        goto err_out;
+                b = 0;
+                in_range = 1;
+                continue;
+            }
+
+            if ( !isdigit(c) )
+            {
+                fprintf(stderr, "Invalid character in cpumask: %s\n",
mask_str);
+                goto err_out;
+            }
+
+            b = b * 10 + (c - ZERO_DIGIT);
+            if ( !in_range )
+                a = b;
+        }
+
+        /* Syntax: <digit>-[,] - expand to number of CPUs. */
+        if ( b == 0 && in_range )
+            b = nmaskbits-1;
+
+        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 )
+        {
+            xc_cpumap_setcpu(a, map);
+            a++;
+        }
+    } while ( c );
+
+    return 0;
+ err_out:
+    errno = EINVAL;
+    return EXIT_FAILURE;
+}
+
+/**
+ * Figure out which of the CPU types the user has provided - either the hex
+ * variant, the cpu-list, or 'all'. Once done set the CPU mask.
+ */
+static int parse_cpu_mask(void)
+{
+    int i, ret = EXIT_FAILURE;
+    xc_cpumap_t map;
+
+    map = xc_cpumap_alloc(xc_handle);
+    if ( !map )
+        goto out;
+
+    if ( strlen(opts.cpu_mask_str) < 1 )
+    {
+        errno = ENOSPC;
+        goto out;
+    }
+
+    ret = 0;
+    if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )
+    {
+        uint32_t v;
+
+        v = argtol(opts.cpu_mask_str, 0);
+        /*
+         * If mask is set, copy the bits out of it.  This still works for
+         * systems with more than 32 cpus, as the shift will just shift
+         * mask down to zero.
+         */
+        for ( i = 0; i < sizeof(uint32_t); i++ )
+            map[i] = (v >> (i * 8)) & 0xff;
+    }
+    else if ( strcmp("all", opts.cpu_mask_str) == 0 )
+    {
+        for ( i = 0; i < xc_get_cpumap_size(xc_handle); i++ )
+            map[i] = 0xff;
+    }
+    else
+        ret = parse_cpumask_range(opts.cpu_mask_str, map);
+
+    if ( !ret )
+        ret = set_cpu_mask(map);
+ out:
+    /* We don't use them pass this point. */
+    free(map);
+    free(opts.cpu_mask_str);
+    opts.cpu_mask_str = NULL;
+    return ret;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -981,10 +1095,9 @@ static void parse_args(int argc, char **argv)
             opts.poll_sleep = argtol(optarg, 0);
             break;

-        case 'c': /* set new cpu mask for filtering*/
-            opts.cpu_mask = argtol(optarg, 0);
+        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;
@@ -1047,7 +1160,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_str = NULL;
     opts.disk_rsvd = 0;
     opts.disable_tracing = 1;
     opts.start_disabled = 0;
@@ -1065,9 +1178,11 @@ int main(int argc, char **argv)
     if ( opts.evt_mask != 0 )
         set_evt_mask(opts.evt_mask);

-
-    if ( opts.cpu_mask != 0 )
-        set_cpu_mask(opts.cpu_mask);
+    if ( opts.cpu_mask_str )
+    {
+        if ( parse_cpu_mask() )
+            exit(EXIT_FAILURE);
+    }

     if ( opts.timeout != 0 )
         alarm(opts.timeout);

[-- Attachment #2: xentrace-cpu-mask.diff --]
[-- Type: text/plain, Size: 9386 bytes --]

commit ddf434fbe637f38b5031bec00872f5133cc93125
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Commit: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

    xentrace: Implement cpu mask range parsing of human values (-c).
    
    Instead of just using -c 0x<some hex value> we can
    also use: -c <starting cpu>-<end cpu>, -c <cpu1>,<cpu2>, or a
    combination of them, or 'all' for all cpus.
    
    This new format can include just singular CPUs: -c <cpu1>,
    or ranges without an start or end (and xentrace will figure out
    the values), such as: -c -<cpu2> (which will include cpu0, cpu1,
    and cpu2) or -c <cpu2>- (which will include cpu2 and up to MAX_CPUS).
    
    That should make it easier to trace the right CPU if
    using this along with 'xl vcpu-list'.
    
    The code has been lifted from the Linux kernel, see file
    lib/bitmap.c, function __bitmap_parselist.
    
    To make the old behavior and the new function work, we check
    to see if the arguments have '0x' in them. If they do
    we use the old style parsing (limited to 32 CPUs). If that
    does not exist we use the new parsing.
    
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
    ---
    v5:
     - Simplify parse_cpu_range
     - Rename "parse_cpu_mask" to parse_cpu_mask

diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8
index ac18e9f..7b3172b 100644
--- a/tools/xentrace/xentrace.8
+++ b/tools/xentrace/xentrace.8
@@ -36,8 +36,38 @@ 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|\fIall\fP]
+This can be: a hex value (of the form 0xNNNN...), or a set of cpu
+ranges as described below, or the string \fIall\fP. Hex values are limited
+to 32 bits. If not specified, the cpu-mask as set during bootup will be
+constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which
+may be specified as follows:
+
+.RS 4
+.ie n .IP """0-3""" 4
+.el .IP "``0-3''" 4
+.IX Item "0-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """0,2,5-7""" 4
+.el .IP "``0,2,5-7''" 4
+.IX Item "0,2,5-7"
+Trace only on CPUs 0, 2, and 5 through 7.
+.ie n .IP """-3""" 4
+.el .IP "``-3''" 4
+.IX Item "-3"
+Trace only on CPUs 0 through 3
+.ie n .IP """-3,7""" 4
+.el .IP "``-3,7''" 4
+.IX Item "-3,7"
+Trace only on CPUs 0 through 3 and 7
+.ie n .IP """3-""" 4
+.el .IP "``3-''" 4
+.IX Item "-3-"
+Trace only on CPUs 3 up to maximum numbers of CPUs the host has.
+.RE
+.Sp
+
+If using \fIall\fP it will use all of the CPUs the host has.
 .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 8b40e88..1558496 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>
 
@@ -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;
+    char *cpu_mask_str;
     unsigned long tbuf_size;
     unsigned long disk_rsvd;
     unsigned long timeout;
@@ -542,33 +543,17 @@ void print_cpu_mask(xc_cpumap_t map)
    fprintf(stderr, "\n");
 }
 
-static void set_cpu_mask(uint32_t mask)
+static int set_cpu_mask(xc_cpumap_t map)
 {
-    int i, ret = 0;
-    xc_cpumap_t map;
-
-    map = xc_cpumap_alloc(xc_handle);
-    if ( !map )
-        goto out;
+    int ret = xc_tbuf_set_cpu_mask(xc_handle, map);
 
-    /*
-     * If mask is set, copy the bits out of it.  This still works for
-     * systems with more than 32 cpus, as the shift will just shift
-     * mask down to zero.
-     */
-    for ( i = 0; i < xc_get_cpumap_size(xc_handle); i++ )
-        map[i] = (mask >> (i * 8)) & 0xff;
-
-    ret = xc_tbuf_set_cpu_mask(xc_handle, map);
-    if ( ret != 0 )
-        goto out;
-
-    print_cpu_mask(map);
-    free(map);
-    return;
-out:
+    if ( ret == 0 )
+    {
+        print_cpu_mask(map);
+        return 0;
+    }
     PERROR("Failure to get trace buffer pointer from Xen and set the new mask");
-    exit(EXIT_FAILURE);
+    return EXIT_FAILURE;
 }
 
 /**
@@ -819,7 +804,8 @@ 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, CPU ranges, or\n" \
+"                          for all CPUs\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" \
@@ -951,6 +937,134 @@ static int parse_evtmask(char *arg)
     return 0;
 }
 
+#define ZERO_DIGIT '0'
+
+#define is_terminator(c) ((c)=='\0' || (c)==',')
+
+static int parse_cpumask_range(const char *mask_str, xc_cpumap_t map)
+{
+    unsigned int a, b;
+    int nmaskbits;
+    char c;
+    int in_range;
+    const char *s;
+
+    nmaskbits = xc_get_max_cpus(xc_handle);
+    if ( nmaskbits <= 0 )
+    {
+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
+        return EXIT_FAILURE;
+    }
+
+    c = 0;
+    s = mask_str;
+    do {
+        in_range = 0;
+        a = b = 0;
+
+        /* Process until we find a range terminator */
+        for(c=*s++; !is_terminator(c); c=*s++)
+        {
+            if ( c == '-' )
+            {
+                if ( in_range )
+                        goto err_out;
+                b = 0;
+                in_range = 1;
+                continue;
+            }
+
+            if ( !isdigit(c) )
+            {
+                fprintf(stderr, "Invalid character in cpumask: %s\n", mask_str);
+                goto err_out;
+            }
+
+            b = b * 10 + (c - ZERO_DIGIT);
+            if ( !in_range )
+                a = b;
+        }
+
+        /* Syntax: <digit>-[,] - expand to number of CPUs. */
+        if ( b == 0 && in_range )
+            b = nmaskbits-1;
+
+        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 )
+        {
+            xc_cpumap_setcpu(a, map);
+            a++;
+        }
+    } while ( c );
+
+    return 0;
+ err_out:
+    errno = EINVAL;
+    return EXIT_FAILURE;
+}
+
+/**
+ * Figure out which of the CPU types the user has provided - either the hex
+ * variant, the cpu-list, or 'all'. Once done set the CPU mask.
+ */
+static int parse_cpu_mask(void)
+{
+    int i, ret = EXIT_FAILURE;
+    xc_cpumap_t map;
+
+    map = xc_cpumap_alloc(xc_handle);
+    if ( !map )
+        goto out;
+
+    if ( strlen(opts.cpu_mask_str) < 1 )
+    {
+        errno = ENOSPC;
+        goto out;
+    }
+
+    ret = 0;
+    if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )
+    {
+        uint32_t v;
+
+        v = argtol(opts.cpu_mask_str, 0);
+        /*
+         * If mask is set, copy the bits out of it.  This still works for
+         * systems with more than 32 cpus, as the shift will just shift
+         * mask down to zero.
+         */
+        for ( i = 0; i < sizeof(uint32_t); i++ )
+            map[i] = (v >> (i * 8)) & 0xff;
+    }
+    else if ( strcmp("all", opts.cpu_mask_str) == 0 )
+    {
+        for ( i = 0; i < xc_get_cpumap_size(xc_handle); i++ )
+            map[i] = 0xff;
+    }
+    else
+        ret = parse_cpumask_range(opts.cpu_mask_str, map);
+
+    if ( !ret )
+        ret = set_cpu_mask(map);
+ out:
+    /* We don't use them pass this point. */
+    free(map);
+    free(opts.cpu_mask_str);
+    opts.cpu_mask_str = NULL;
+    return ret;
+}
+
 /* parse command line arguments */
 static void parse_args(int argc, char **argv)
 {
@@ -981,10 +1095,9 @@ static void parse_args(int argc, char **argv)
             opts.poll_sleep = argtol(optarg, 0);
             break;
 
-        case 'c': /* set new cpu mask for filtering*/
-            opts.cpu_mask = argtol(optarg, 0);
+        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;
@@ -1047,7 +1160,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_str = NULL;
     opts.disk_rsvd = 0;
     opts.disable_tracing = 1;
     opts.start_disabled = 0;
@@ -1065,9 +1178,11 @@ int main(int argc, char **argv)
     if ( opts.evt_mask != 0 )
         set_evt_mask(opts.evt_mask);
 
-
-    if ( opts.cpu_mask != 0 )
-        set_cpu_mask(opts.cpu_mask);
+    if ( opts.cpu_mask_str )
+    {
+        if ( parse_cpu_mask() )
+            exit(EXIT_FAILURE);
+    }
 
     if ( opts.timeout != 0 ) 
         alarm(opts.timeout);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Konrad Rzeszutek Wilk]

. snip..
> Right.  Well I think all that is unnecessarily hard to understand,
> primarily because it's organized in a confusing way.

Looking at your implementation it is certainly easier (your way).

> 
> Rather than try to nitpick you into doing it the way that seems more
> sensible to me, I've rewritten the patch a bit below.  This time I've
> actually done some basic testing with it.  What do you think?

I like it! Only one minor comment which is style related:
.. snip..
> +        /* Process until we find a range terminator */
> +        for(c=*s++; !is_terminator(c); c=*s++)

This is missing spaces.

I've committed it in (with the update) - since you are the maintainer
of xentrace.c and it has your SoB on it I figured you would be OK with
it :-)

If I errnously did it - please revert it and accept my apologies!