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