From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v1 COLO Pre 03/12] tools/libxc: export xc_bitops.h Date: Tue, 2 Jun 2015 11:11:22 +0100 Message-ID: <556D814A.7090909@citrix.com> References: <1433237180-21181-1-git-send-email-yanghy@cn.fujitsu.com> <1433237180-21181-4-git-send-email-yanghy@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1433237180-21181-4-git-send-email-yanghy@cn.fujitsu.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Yang Hongyang , xen-devel@lists.xen.org Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, wency@cn.fujitsu.com, guijianfeng@cn.fujitsu.com, yunhong.jiang@intel.com, eddie.dong@intel.com, rshriram@cs.ubc.ca, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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 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 > +#include > + > +#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 > -#include > - > -#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 */