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