[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 COLO Pre 03/12] tools/libxc: export xc_bitops.h
On 02/06/15 10:26, Yang Hongyang wrote: > When we are under COLO, we will send dirty page bitmap info from > secondary to primary at every checkpoint. So we need to get/test > the dirty page bitmap. We just expose xc_bitops.h for libxl use. > > NOTE: > Need to make clean and rerun configure to get it compiled. > > Signed-off-by: Yang Hongyang <yanghy@xxxxxxxxxxxxxx> I like this change, but lets take the opportunity to fix some of the issues in it. > --- > tools/libxc/include/xc_bitops.h | 76 > +++++++++++++++++++++++++++++++++++++++++ > tools/libxc/xc_bitops.h | 76 > ----------------------------------------- > 2 files changed, 76 insertions(+), 76 deletions(-) > create mode 100644 tools/libxc/include/xc_bitops.h > delete mode 100644 tools/libxc/xc_bitops.h > > diff --git a/tools/libxc/include/xc_bitops.h b/tools/libxc/include/xc_bitops.h > new file mode 100644 > index 0000000..cd749f4 > --- /dev/null > +++ b/tools/libxc/include/xc_bitops.h > @@ -0,0 +1,76 @@ > +#ifndef XC_BITOPS_H > +#define XC_BITOPS_H 1 No need for a 1 here > + > +/* bitmap operations for single threaded access */ > + > +#include <stdlib.h> > +#include <string.h> > + > +#define BITS_PER_LONG (sizeof(unsigned long) * 8) All defines like this need XC_ prefixes, and CHAR_BIT should be used in preference to 8. > +#define ORDER_LONG (sizeof(unsigned long) == 4 ? 5 : 6) This name is misleading, as it is in terms of bits not bytes. XC_BITMAP_SHIFT perhaps? > + > +#define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG] > +#define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG) I would recommend dropping these and open coding the few cases below. It would be far more clear. > + > +/* calculate required space for number of longs needed to hold nr_bits */ > +static inline int bitmap_size(int nr_bits) "int" has been inappropriate everywhere in this file. unsigned long please (or settle on unsigned int everywhere) > +{ > + int nr_long, nr_bytes; > + nr_long = (nr_bits + BITS_PER_LONG - 1) >> ORDER_LONG; This calculation can overflow. (nr_bits >> ORDER_LONG) + !!(nr_bits % BITS_PER_LONG) > + nr_bytes = nr_long * sizeof(unsigned long); > + return nr_bytes; > +} > + > +static inline unsigned long *bitmap_alloc(int nr_bits) > +{ > + return calloc(1, bitmap_size(nr_bits)); > +} > + > +static inline void bitmap_set(unsigned long *addr, int nr_bits) > +{ > + memset(addr, 0xff, bitmap_size(nr_bits)); > +} > + > +static inline void bitmap_clear(unsigned long *addr, int nr_bits) > +{ > + memset(addr, 0, bitmap_size(nr_bits)); > +} > + > +static inline int test_bit(int nr, unsigned long *addr) const *addr, as this is a read-only operation. > +{ > + return (BITMAP_ENTRY(nr, addr) >> BITMAP_SHIFT(nr)) & 1; > +} > + > +static inline void clear_bit(int nr, unsigned long *addr) > +{ > + BITMAP_ENTRY(nr, addr) &= ~(1UL << BITMAP_SHIFT(nr)); > +} > + > +static inline void set_bit(int nr, unsigned long *addr) > +{ > + BITMAP_ENTRY(nr, addr) |= (1UL << BITMAP_SHIFT(nr)); > +} It would be nice to be consistent on whether the bitmap pointer or the bit is the first parameter. Perhaps a second cleanup patch which makes this consistent and adjusts all current callers. ~Andrew > + > +static inline int test_and_clear_bit(int nr, unsigned long *addr) > +{ > + int oldbit = test_bit(nr, addr); > + clear_bit(nr, addr); > + return oldbit; > +} > + > +static inline int test_and_set_bit(int nr, unsigned long *addr) > +{ > + int oldbit = test_bit(nr, addr); > + set_bit(nr, addr); > + return oldbit; > +} > + > +static inline void bitmap_or(unsigned long *dst, const unsigned long *other, > + int nr_bits) > +{ > + int i, nr_longs = (bitmap_size(nr_bits) / sizeof(unsigned long)); > + for ( i = 0; i < nr_longs; ++i ) > + dst[i] |= other[i]; > +} > + > +#endif /* XC_BITOPS_H */ > diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h > deleted file mode 100644 > index cd749f4..0000000 > --- a/tools/libxc/xc_bitops.h > +++ /dev/null > @@ -1,76 +0,0 @@ > -#ifndef XC_BITOPS_H > -#define XC_BITOPS_H 1 > - > -/* bitmap operations for single threaded access */ > - > -#include <stdlib.h> > -#include <string.h> > - > -#define BITS_PER_LONG (sizeof(unsigned long) * 8) > -#define ORDER_LONG (sizeof(unsigned long) == 4 ? 5 : 6) > - > -#define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG] > -#define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG) > - > -/* calculate required space for number of longs needed to hold nr_bits */ > -static inline int bitmap_size(int nr_bits) > -{ > - int nr_long, nr_bytes; > - nr_long = (nr_bits + BITS_PER_LONG - 1) >> ORDER_LONG; > - nr_bytes = nr_long * sizeof(unsigned long); > - return nr_bytes; > -} > - > -static inline unsigned long *bitmap_alloc(int nr_bits) > -{ > - return calloc(1, bitmap_size(nr_bits)); > -} > - > -static inline void bitmap_set(unsigned long *addr, int nr_bits) > -{ > - memset(addr, 0xff, bitmap_size(nr_bits)); > -} > - > -static inline void bitmap_clear(unsigned long *addr, int nr_bits) > -{ > - memset(addr, 0, bitmap_size(nr_bits)); > -} > - > -static inline int test_bit(int nr, unsigned long *addr) > -{ > - return (BITMAP_ENTRY(nr, addr) >> BITMAP_SHIFT(nr)) & 1; > -} > - > -static inline void clear_bit(int nr, unsigned long *addr) > -{ > - BITMAP_ENTRY(nr, addr) &= ~(1UL << BITMAP_SHIFT(nr)); > -} > - > -static inline void set_bit(int nr, unsigned long *addr) > -{ > - BITMAP_ENTRY(nr, addr) |= (1UL << BITMAP_SHIFT(nr)); > -} > - > -static inline int test_and_clear_bit(int nr, unsigned long *addr) > -{ > - int oldbit = test_bit(nr, addr); > - clear_bit(nr, addr); > - return oldbit; > -} > - > -static inline int test_and_set_bit(int nr, unsigned long *addr) > -{ > - int oldbit = test_bit(nr, addr); > - set_bit(nr, addr); > - return oldbit; > -} > - > -static inline void bitmap_or(unsigned long *dst, const unsigned long *other, > - int nr_bits) > -{ > - int i, nr_longs = (bitmap_size(nr_bits) / sizeof(unsigned long)); > - for ( i = 0; i < nr_longs; ++i ) > - dst[i] |= other[i]; > -} > - > -#endif /* XC_BITOPS_H */ _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |