From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v3 2/3] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t Date: Mon, 30 Mar 2015 12:54:11 -0400 Message-ID: <20150330165411.GC13141@l.oracle.com> References: <1427211559-15185-1-git-send-email-konrad.wilk@oracle.com> <1427211559-15185-3-git-send-email-konrad.wilk@oracle.com> <5519755D.9000103@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Yccx8-00067N-8D for xen-devel@lists.xenproject.org; Mon, 30 Mar 2015 16:54:18 +0000 Content-Disposition: inline In-Reply-To: <5519755D.9000103@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org 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 > > Acked-by: Ian Campbell > > [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); > +}