Re: [Xen-devel][PATCH] libxc bitmap utils and vcpu-affinity

[Dulloor <dulloor@xxxxxxxxx>]

> No changeset comment. No signed-off-by line.
Sorry, I forgot. Will do once we finalize on the patch.

> It actually bloats the libraries by a net 650 LOC
> (747 added, 87 deleted according to diffstat).
In the patch, we have used the library only for vcpu get/set affinity.
There are clearly other opportunities (right now and in future) to use
most of the functions provided by the library, which will offset this.
Also, this provides a cleaner/standard way of using the cpumap
structure in libxc.

> And below I append the very first function I read: it doesn't inspire
> confidence that the implementation is over-complicated/long and
> unnecessarily handles 16-bit values.
I guess you mean 8-bit values. The library works with byte-based
bitmap structures, instead of "uint32_t or uint64_t" bitmap
structures, so that we don't need to convert when using the
bitmap-library with xenctl_cpumap. Please let know if you would rather
that we keep the bitmap utilities separate from xenctl_cpumap and
provide for conversion functions. That would be a small change.

The function that you quote is inspired from linux kernel
implementation (as mentioned) and is a simple generic function. And I
have tested the library thoroughly.

> Why should I show your patch some love?
We can work towards that ;)

-dulloor

On Tue, Mar 30, 2010 at 11:16 AM, Keir Fraser <keir.fraser@xxxxxxxxxxxxx> wrote:
> No changeset comment. No signed-off-by line. It actually bloats the
> libraries by a net 650 LOC (747 added, 87 deleted according to diffstat).
> And below I append the very first function I read: it doesn't inspire
> confidence that the implementation is over-complicated/long and
> unnecessarily handles 16-bit values. Why should I show your patch some love?
>
>  -- Keir
>
> +static inline int __xc_ffs(uint8_t byte)
> +{
> +       int num = 0;
> +
> +       if ((byte & 0xff) == 0) {
> +               num += 8;
> +               byte >>= 8;
> +       }
> +       if ((byte & 0xf) == 0) {
> +               num += 4;
> +               byte >>= 4;
> +       }
> +       if ((byte & 0x3) == 0) {
> +               num += 2;
> +               byte >>= 2;
> +       }
> +       if ((byte & 0x1) == 0)
> +               num += 1;
> +       return num;
> +}
>
> On 30/03/2010 15:42, "Dulloor" <dulloor@xxxxxxxxx> wrote:
>
>> Resubmitting the patch.
>>
>> -dulloor
>>
>> ---------- Forwarded message ----------
>> From: Dulloor <dulloor@xxxxxxxxx>
>> Date: Tue, Mar 23, 2010 at 12:55 PM
>> Subject: Re: [Xen-devel][PATCH] libxc bitmap utils and vcpu-affinity
>> To: Keir Fraser <keir.fraser@xxxxxxxxxxxxx>
>> Cc: Jan Beulich <JBeulich@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx"
>> <xen-devel@xxxxxxxxxxxxxxxxxxx>
>>
>>
>> Please use this patch, in which length of bitmap is
>> (physinfo.max_cpu_id+1), rather than (physinfo.nr_cpus).
>>
>> -dulloor
>>
>> On Tue, Mar 23, 2010 at 12:41 PM, Dulloor <dulloor@xxxxxxxxx> wrote:
>>> I meant utils for **xenctl_cpumap**
>>>
>>> On Tue, Mar 23, 2010 at 12:40 PM, Dulloor <dulloor@xxxxxxxxx> wrote:
>>>> Fine, I agree with you both. Attached is a patch adding utils for
>>>> xenctl_bitmap (to libxc) and using the same in vcpu_(get|set)affinity.
>>>> For the guest-numa interface, I will see if I can use xenctl_cpumap.
>>>>
>>>> -dulloor
>>>>
>>>> On Tue, Mar 23, 2010 at 7:05 AM, Keir Fraser <keir.fraser@xxxxxxxxxxxxx>
>>>> wrote:
>>>>> On 23/03/2010 10:10, "Jan Beulich" <JBeulich@xxxxxxxxxx> wrote:
>>>>>
>>>>>>>>> Dulloor <dulloor@xxxxxxxxx> 22.03.10 18:44 >>>
>>>>>>> Motivation for using xenctl_cpumask in Xen interfaces :
>>>>>>> - xenctl_cpumap is just 4 bytes smaller than static xenctl_cpumask for
>>>>>>> 128 cpus (128 would be good for quite some time). However, the new
>>>>>>
>>>>>> I don't buy this (we're already building for 256 CPUs, looking forward
>>>>>> to further bump this in the not too distant future), and I'm generally
>>>>>> opposed to introducing hard coded limits in a public interface.
>>>>>
>>>>> We should use xenctl_cpumask everywhere for specifying physical CPU
>>>>> bitmaps,
>>>>> even into guest NUMA interfaces if appropriate. I don't really care if it
>>>>> is
>>>>> a bit harder to use than a static bitmap.
>>>>>
>>>>>  -- Keir
>>>>>
>>>>>
>>>>>
>>>>
>>>
>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel