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 <george.dunlap@eu.citrix.com>]

[-- 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