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

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