Olaf Hering
2012-Jan-30 00:36 UTC
[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"
Keir Fraser
2012-Jan-30 07:32 UTC
Re: [PATCH] tools/libxc: make volatile keyword for bitmap operations optional
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
Olaf Hering
2012-Jan-30 09:10 UTC
Re: [PATCH] tools/libxc: make volatile keyword for bitmap operations optional
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
Keir Fraser
2012-Jan-30 09:30 UTC
Re: [PATCH] tools/libxc: make volatile keyword for bitmap operations optional
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
Olaf Hering
2012-Jan-30 10:26 UTC
Re: [PATCH] tools/libxc: make volatile keyword for bitmap operations optional
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