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

[Andre Przywara <andre.przywara@amd.com>]

Dulloor wrote:
> Implement basic utility functions to manipulate bitmasks. Used in later patches.
> 
> -dulloor
> 
> Signed-off-by : Dulloor <dulloor@gmail.com>
> 
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