From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Hongyang Subject: Re: [PATCH v1 COLO Pre 03/12] tools/libxc: export xc_bitops.h Date: Thu, 4 Jun 2015 09:01:53 +0800 Message-ID: <556FA381.6070300@cn.fujitsu.com> References: <1433237180-21181-1-git-send-email-yanghy@cn.fujitsu.com> <1433237180-21181-4-git-send-email-yanghy@cn.fujitsu.com> <556D814A.7090909@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <556D814A.7090909@citrix.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: Andrew Cooper , 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 06/02/2015 06:11 PM, Andrew Cooper wrote: > 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. Thanks, will fix in next version. > >> --- >> 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 */ > > . > -- Thanks, Yang.