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

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# 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.

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@aepfle.de>

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"

On 30/01/2012 00:36, "Olaf Hering" <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # 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@aepfle.de>
> 
> 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@lists.xensource.com
> http://lists.xensource.com/xen-devel

On Mon, Jan 30, Keir Fraser wrote:
> On 30/01/2012 00:36, "Olaf Hering" <olaf@aepfle.de> wrote:
> 
> > # HG changeset patch
> > # User Olaf Hering <olaf@aepfle.de>
> > # 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.
Will gcc use the right thing if the array is passed to the hyperviso,
and will it reload everything after the hypercall? If yes, the volatile
can indeed go.

Olaf

On 30/01/2012 09:10, "Olaf Hering" <olaf@aepfle.de> wrote:
>>> 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.
> 
> Will gcc use the right thing if the array is passed to the hyperviso,
> and will it reload everything after the hypercall? If yes, the volatile
> can indeed go.
Of course. Lots of things would fail to work if calls to outside the current
linkage unit didn''t flush/invalidate cached shared-data accesses.
That''s a
fundamental C compiler thing.

 -- Keir

On Mon, Jan 30, Keir Fraser wrote:
> On 30/01/2012 09:10, "Olaf Hering" <olaf@aepfle.de> wrote:
> > Will gcc use the right thing if the array is passed to the hyperviso,
> > and will it reload everything after the hypercall? If yes, the
volatile
> > can indeed go.
> 
> Of course. Lots of things would fail to work if calls to outside the
current
> linkage unit didn''t flush/invalidate cached shared-data accesses.
That''s a
> fundamental C compiler thing.
You are right, after thinking about it I came to the same conclusion.
I will send an updated patch.

Olaf