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 <konrad.wilk@oracle.com>]

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