Re: [xen-devel][vNUMA v2][PATCH 3/8] Basic cpumap utilities

[Dulloor <dulloor@xxxxxxxxx>]

Hi Andre,

Thanks for the reviews. I am going to be very busy this week, so I
will take this up immediately after that.
Also, as you mentioned elsewhere, we can have this code checked-in in
an acceptable state and you could
push your rebased changes on that.

thanks
dulloor

On Fri, Aug 13, 2010 at 8:25 AM, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> Dulloor wrote:
>>
>> Implement basic utility functions to manipulate bitmasks. Used in later
>> patches.
>>
>> -dulloor
>>
>> Signed-off-by : Dulloor <dulloor@xxxxxxxxx>
>>
> In general this looks OK, although a bit too sophisticated for my
> personal taste. Only some minor comments:
>
> It seems that these functions are somewhat generic, so it may be worth
> to create a generic interface instead and somehow tie the connection
> to xenctl_cpumap later with the instantiation. The generic functions
> could be prefixed with xc_bitmap_*.
> QEMU is about to also get a generic bitmap library:
> http://lists.gnu.org/archive/html/qemu-devel/2010-08/msg00517.html
> maybe one could leverage this.
>
>> +++ b/tools/libxc/xc_cpumap.c
>> +void __xc_cpumap_or(struct xenctl_cpumap *dstp,
>> +        struct xenctl_cpumap *src1p, struct xenctl_cpumap *src2p)
>> +{
>> +    uint8_t *dp = xc_cpumap_bits(dstp);
>> +    uint8_t *s1p = xc_cpumap_bits(src1p);
>> +    uint8_t *s2p = xc_cpumap_bits(src2p);
>> +    int nr = XC_BITS_TO_BYTES(xc_cpumap_len(dstp));
>> +    int k;
>> +    for (k=0; k<nr; k++)
>> +        dp[k] = s1p[k] | s2p[k];
>> +}
>
> Shouldn't we observe the special case with different source length here? If
> one bitmap contains garbage after it's end, then the result would be bogus.
> I think the bitmap or'ing should be stopped after both input bitmaps came to
> an end. I think xc_cpumap_setall() does it correct.
>
>> +
>> +static inline uint8_t hweight8(uint8_t w)
>> +{
>> +    uint8_t res = (w & 0x55) + ((w >> 1) & 0x55);
>> +    res = (res & 0x33) + ((res >> 2) & 0x33);
>> +    return (res & 0x0F) + ((res >> 4) & 0x0F);
>> +}
>> +
>> +int __xc_cpumap_weight(struct xenctl_cpumap *srcp)
>> +{
>> +    const uint8_t *sp = xc_cpumap_bits(srcp);
>> +    int k, w = 0, lim = XC_BITS_TO_BYTES(xc_cpumap_len(srcp));
>> +    for (k=0; k <lim; k++)
>> +        w += hweight8(sp[k]);
>> +    return w;
>> +}
>
> We should stop counting after hitting the maximum specified length,
> otherwise possible garbage bits would be counted in.
>
>> +
>> +/* xenctl_cpumap print function */
>> +#define CHUNKSZ        8
>> +#define roundup_power2(val,modulus)    (((val) + (modulus) - 1) &
>> ~((modulus) - 1))
>> +
>> +int __xc_cpumap_snprintf(char *buf, unsigned int buflen,
>> +                                        const struct xenctl_cpumap
>> *cpumap)
>> +{
>> +    const uint8_t *maskp = xc_cpumap_bits(cpumap);
>> +    int nmaskbits = xc_cpumap_len(cpumap);
>> +       int i, word, bit, len = 0;
>> +       unsigned long val;
>> +       const char *sep = "";
>> +       int chunksz;
>> +       uint8_t chunkmask;
>> +
>> +       chunksz = nmaskbits & (CHUNKSZ - 1);
>> +       if (chunksz == 0)
>> +               chunksz = CHUNKSZ;
>> +
>> +       i = roundup_power2(nmaskbits, CHUNKSZ) - CHUNKSZ;
>> +       for (; i >= 0; i -= CHUNKSZ) {
>> +               chunkmask = ((1ULL << chunksz) - 1);
>> +               word = i / XC_BITS_PER_BYTE;
>> +               bit = i % XC_BITS_PER_BYTE;
>> +               val = (maskp[word] >> bit) & chunkmask;
>> +               len += snprintf(buf+len, buflen-len, "%s%0*lx", sep,
>> +                       (chunksz+3)/4, val);
>> +               chunksz = CHUNKSZ;
>
> Isn't that line redundant?
>>
>> +               sep = ",";
>> +       }
>> +       return len;
>> +}
>
> Regards,
> Andre.
>
> --
> Andre Przywara
> AMD-Operating System Research Center (OSRC), Dresden, Germany
> Tel: +49 351 448-3567-12
>
>

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