Re: [Xen-devel] [PATCH] tools/libxc: make volatile keyword for bitmap operations optional

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 30/01/2012 00:36, "Olaf Hering" <olaf@xxxxxxxxx> wrote:

> # HG changeset patch
> # User Olaf Hering <olaf@xxxxxxxxx>
> # Date 1327883694 -3600
> # Node ID 5da4c273e819d5a1d42f3186f0829f8e00d83132
> # Parent  b06617b0398c73c63ce153f39464fd1edac788e5
> tools/libxc: make volatile keyword for bitmap operations optional
> 
> Except for xc_save, all bitmaps maintained by xc_bitops.h are used in single
> threaded applications. So nothing will change the bitmaps content, adding
> volatile adds just unneeded memory reloads.

The bitops aren't threadsafe anyway, as none of them use atomic rmw
instructions. I suspect the volatile declarations are completely pointless
and can just be removed.

 -- Keir

> Adjust the bitmap helpers to make the volatile keyword optional. Users of
> xc_bitops.h can define XC_BITMAPS_VOLATILE before inclusion to declare all
> bitmaps as volatile.
> 
> xc_save passes the bitmap to the hypervisor which in turn modifies it. To be
> safe, declare the used bitmaps as volatile.
> 
> xenpaging uses bitmaps alot and using non-volatile versions will slightly
> improve performance.
> 
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> 
> diff -r b06617b0398c -r 5da4c273e819 tools/libxc/xc_bitops.h
> --- a/tools/libxc/xc_bitops.h
> +++ b/tools/libxc/xc_bitops.h
> @@ -3,6 +3,13 @@
>  
>  /* bitmap operations for single threaded access */
>  
> +/* Declare bitmaps passed to the hypervisor as volatile */
> +#ifdef XC_BITMAPS_VOLATILE
> +#define __XC_BITMAP_VOLATILE volatile
> +#else
> +#define __XC_BITMAP_VOLATILE
> +#endif
> +
>  #include <stdlib.h>
>  #include <string.h>
>  
> @@ -31,29 +38,29 @@ static inline void bitmap_clear(unsigned
>      memset(addr, 0, bitmap_size(nr_bits));
>  }
>  
> -static inline int test_bit(int nr, volatile unsigned long *addr)
> +static inline int test_bit(int nr, __XC_BITMAP_VOLATILE unsigned long *addr)
>  {
>      return (BITMAP_ENTRY(nr, addr) >> BITMAP_SHIFT(nr)) & 1;
>  }
>  
> -static inline void clear_bit(int nr, volatile unsigned long *addr)
> +static inline void clear_bit(int nr, __XC_BITMAP_VOLATILE unsigned long
> *addr)
>  {
>      BITMAP_ENTRY(nr, addr) &= ~(1UL << BITMAP_SHIFT(nr));
>  }
>  
> -static inline void set_bit(int nr, volatile unsigned long *addr)
> +static inline void set_bit(int nr, __XC_BITMAP_VOLATILE unsigned long *addr)
>  {
>      BITMAP_ENTRY(nr, addr) |= (1UL << BITMAP_SHIFT(nr));
>  }
>  
> -static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
> +static inline int test_and_clear_bit(int nr, __XC_BITMAP_VOLATILE unsigned
> long *addr)
>  {
>      int oldbit = test_bit(nr, addr);
>      clear_bit(nr, addr);
>      return oldbit;
>  }
>  
> -static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
> +static inline int test_and_set_bit(int nr, __XC_BITMAP_VOLATILE unsigned long
> *addr)
>  {
>      int oldbit = test_bit(nr, addr);
>      set_bit(nr, addr);
> diff -r b06617b0398c -r 5da4c273e819 tools/libxc/xc_domain_save.c
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -27,6 +27,7 @@
>  #include <sys/time.h>
>  
>  #include "xc_private.h"
> +#define XC_BITMAPS_VOLATILE
>  #include "xc_bitops.h"
>  #include "xc_dom.h"
>  #include "xg_private.h"
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel