Implement basic utility functions to manipulate bitmasks. Used in later patches. -dulloor Signed-off-by : Dulloor <dulloor@gmail.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Implement basic utility functions to manipulate bitmasks. Used in later patches. -dulloor Signed-off-by : Dulloor <dulloor@gmail.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andre Przywara
2010-Aug-13 15:25 UTC
Re: [xen-devel][vNUMA v2][PATCH 3/8] Basic cpumap utilities
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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@amd.com> wrote:> 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 > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel