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

[Andre Przywara <andre.przywara@xxxxxxx>]

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