From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [vNUMA v2][PATCH 3/8] Basic cpumap utilities Date: Fri, 13 Aug 2010 17:25:26 +0200 Message-ID: <4C6563E6.6090109@amd.com> References: <1BEA8649F0C00540AB2811D7922ECB6C9338B4CD@orsmsx507.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Dulloor Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org Dulloor wrote: > Implement basic utility functions to manipulate bitmasks. Used in later patches. > > -dulloor > > Signed-off-by : Dulloor > 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 + 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 + 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