[PATCH] Cleanup rmb()/wmb() usage

[PATCH] Cleanup rmb()/wmb() usage

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

This is a pretty simple patch to use the read/write barriers defined in 
asm/system.h instead of using hardcoded versions in various places 
throughout Xen.

I've checked and using asm/system.h generates the same code on my 
system.  I also tested xcs and it seems to work fine.  I haven't tested 
blktap but again, it's generating the same code.

Right now, wmb() is defined as a NOP on any 386 architecture.  Some 
Intel clones require a non-NOP wmb().   Using asm/system.h ensures we do 
the right thing.

It's against xen-unstable as of today.

Regards,
Anthony Liguori

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>


[-- Attachment #2: rmb_wmb_cleanup.diff --]
[-- Type: text/x-patch, Size: 2621 bytes --]

diff -ur xen-unstable-orig/tools/blktap/blktaplib.h xen-unstable/tools/blktap/blktaplib.h
--- xen-unstable-orig/tools/blktap/blktaplib.h	2005-02-21 22:19:26.000000000 -0600
+++ xen-unstable/tools/blktap/blktaplib.h	2005-02-22 21:26:06.000000000 -0600
@@ -18,14 +18,9 @@
 typedef int16_t            s16;
 typedef int32_t            s32;
 typedef int64_t            s64;
-                                                                                
-#if defined(__i386__)
-#define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%esp)" : : : "memory" )
-#define wmb() __asm__ __volatile__ ( "" : : : "memory" )
-#else
-#error "Define barriers"
-#endif
-    
+
+#include <asm/system.h>
+
 #include <sys/user.h>
 #include <xen/xen.h>
 #include <xen/io/blkif.h>
diff -ur xen-unstable-orig/tools/python/xen/lowlevel/xu/xu.c xen-unstable/tools/python/xen/lowlevel/xu/xu.c
--- xen-unstable-orig/tools/python/xen/lowlevel/xu/xu.c	2005-02-21 22:19:25.000000000 -0600
+++ xen-unstable/tools/python/xen/lowlevel/xu/xu.c	2005-02-22 21:23:50.000000000 -0600
@@ -27,6 +27,8 @@
 #include <xen/io/domain_controller.h>
 #include <xen/linux/privcmd.h>
 
+#include <asm/system.h>
+
 #define XENPKG "xen.lowlevel.xu"
 
 /* Needed for Python versions earlier than 2.3. */
@@ -49,14 +51,6 @@
 /* Size of a machine page frame. */
 #define PAGE_SIZE 4096
 
-#if defined(__i386__)
-#define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%esp)" : : : "memory" )
-#define wmb() __asm__ __volatile__ ( "" : : : "memory" )
-#else
-#error "Define barriers"
-#endif
-
-
 /* Set the close-on-exec flag on a file descriptor.  Doesn't currently bother
  * to check for errors. */
 /*
diff -ur xen-unstable-orig/tools/xcs/xcs.h xen-unstable/tools/xcs/xcs.h
--- xen-unstable-orig/tools/xcs/xcs.h	2005-02-21 22:19:31.000000000 -0600
+++ xen-unstable/tools/xcs/xcs.h	2005-02-22 21:24:12.000000000 -0600
@@ -16,6 +16,7 @@
 #include <xen/io/domain_controller.h>
 #include <xen/linux/privcmd.h>
 #include <sys/time.h>
+#include <asm/system.h>
 #include "xcs_proto.h"
 
 /* ------[ Debug macros ]--------------------------------------------------*/
@@ -39,13 +40,6 @@
 /* Size of a machine page frame. */
 #define PAGE_SIZE 4096
 
-#if defined(__i386__)
-#define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%esp)" : : : "memory" )
-#define wmb() __asm__ __volatile__ ( "" : : : "memory" )
-#else
-#error "Define barriers"
-#endif
-
 #ifndef timersub /* XOPEN and __BSD don't cooperate well... */
 #define timersub(a, b, result)                                                \
   do {                                                                        \

Re: [PATCH] Cleanup rmb()/wmb() usage

[Keir Fraser]

On 23 Feb 2005, at 03:38, Anthony Liguori wrote:

> This is a pretty simple patch to use the read/write barriers defined 
> in asm/system.h instead of using hardcoded versions in various places 
> throughout Xen.
>
> I've checked and using asm/system.h generates the same code on my 
> system.  I also tested xcs and it seems to work fine.  I haven't 
> tested blktap but again, it's generating the same code.

asm/system.h is a private kernel header so should not be directly 
included from user space. Inlcuding it may not work for all versions of 
Linux, or for other architectures.

We should probably merge the barrier defs we have scattered in the 
tools directory into one low-level architectural header that we include 
everywhere, and extend for x86/64, ia64, and so on.

> Right now, wmb() is defined as a NOP on any 386 architecture.  Some 
> Intel clones require a non-NOP wmb().   Using asm/system.h ensures we 
> do the right thing.

Only Centaur clones that have been set up by the kernel to do write 
combining. Xen does not ever enable write-combining on normal RAM and 
so a barrier is never required.

  -- Keir



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: [PATCH] Cleanup rmb()/wmb() usage

[Anthony Liguori]

Keir Fraser wrote:

>
> On 23 Feb 2005, at 03:38, Anthony Liguori wrote:
>
> asm/system.h is a private kernel header so should not be directly 
> included from user space. Inlcuding it may not work for all versions 
> of Linux, or for other architectures.

Actually, asm/system.h is designed for inclusion in userspace apps (and 
kernel space).  Hence the __KERNEL__ guards around certain sections.

It's used in a few notable userspace apps including glibc and MySQL.   
That said, Xen should not depend on Linux if it doesn't have to.

> We should probably merge the barrier defs we have scattered in the 
> tools directory into one low-level architectural header that we 
> include everywhere, and extend for x86/64, ia64, and so on.

Agreed.  Any ideas on where it would live?  I didn't see any likely 
place which is why I used asm/system.  io/ring.h seemed the closest fit 
but that's not architecture specific.

> Only Centaur clones that have been set up by the kernel to do write 
> combining. Xen does not ever enable write-combining on normal RAM and 
> so a barrier is never required.

Ah, that makes sense.  It still would be nice to have the barriers in a 
single location.  Pick a place and I'll submit a patch.

Regards,

-- 
Anthony Liguori
anthony@codemonkey.ws



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: [PATCH] Cleanup rmb()/wmb() usage

[Keir Fraser]

On 23 Feb 2005, at 07:57, Anthony Liguori wrote:

>> We should probably merge the barrier defs we have scattered in the 
>> tools directory into one low-level architectural header that we 
>> include everywhere, and extend for x86/64, ia64, and so on.
>
> Agreed.  Any ideas on where it would live?  I didn't see any likely 
> place which is why I used asm/system.  io/ring.h seemed the closest 
> fit but that's not architecture specific.

I'd rather not further clutter the Xen headers with the defintions. 
libxc is a utility library used by pretty much all the tools. How about 
defining the barriers in xc.h?

  -- Keir



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click