[PATCH] mini-os: fix bit ops comments and memory clobbers.

[PATCH] mini-os: fix bit ops comments and memory clobbers.

[Samuel Thibault]

This fixes comments about test_and_clear_bit, set_bit and clear_bit,
which are actually not atomic, can be reordered and are not memory
barriers.

This also drops the empty LOCK and LOCK_PREFIX macros which bring to the
confusion.

This also adds missing memory clobbers to set_bit and clear_bit.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

diff --git a/extras/mini-os/include/x86/os.h b/extras/mini-os/include/x86/os.h
index f193865..510c632 100644
--- a/extras/mini-os/include/x86/os.h
+++ b/extras/mini-os/include/x86/os.h
@@ -152,8 +152,6 @@ do {									\
 #endif
 
 
-#define LOCK_PREFIX ""
-#define LOCK ""
 #define ADDR (*(volatile long *) addr)
 /*
  * Make sure gcc doesn't try to be clever and move things around
@@ -200,15 +198,13 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
  * @nr: Bit to clear
  * @addr: Address to count from
  *
- * This operation is atomic and cannot be reordered.
- * It can be reorderdered on other architectures other than x86.
- * It also implies a memory barrier.
+ * This operation is not atomic and can be reordered.
  */
 static inline int test_and_clear_bit(int nr, volatile unsigned long * addr)
 {
 	int oldbit;
 
-	__asm__ __volatile__( LOCK
+	__asm__ __volatile__(
 		"btrl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
 		:"Ir" (nr) : "memory");
@@ -241,22 +237,17 @@ static inline int variable_test_bit(int nr, const volatile unsigned long * addr)
  * @nr: the bit to set
  * @addr: the address to start counting from
  *
- * This function is atomic and may not be reordered.  See __set_bit()
- * if you do not require the atomic guarantees.
- *
- * Note: there are no guarantees that this function will not be reordered
- * on non x86 architectures, so if you are writting portable code,
- * make sure not to rely on its reordering guarantees.
+ * This function is not atomic and may be reordered.
  *
  * Note that @nr may be almost arbitrarily large; this function is not
  * restricted to acting on a single-word quantity.
  */
 static inline void set_bit(int nr, volatile unsigned long * addr)
 {
-	__asm__ __volatile__( LOCK
+	__asm__ __volatile__(
 		"btsl %1,%0"
 		:"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr): "memory");
 }
 
 /**
@@ -271,10 +262,10 @@ static inline void set_bit(int nr, volatile unsigned long * addr)
  */
 static inline void clear_bit(int nr, volatile unsigned long * addr)
 {
-	__asm__ __volatile__( LOCK
+	__asm__ __volatile__(
 		"btrl %1,%0"
 		:"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr): "memory");
 }
 
 /**
@@ -347,14 +338,13 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
  * @nr: Bit to clear
  * @addr: Address to count from
  *
- * This operation is atomic and cannot be reordered.  
- * It also implies a memory barrier.
+ * This operation is not atomic and can be reordered.
  */
 static __inline__ int test_and_clear_bit(int nr, volatile void * addr)
 {
 	int oldbit;
 
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__(
 		"btrl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
 		:"dIr" (nr) : "memory");
@@ -388,14 +378,14 @@ static __inline__ int variable_test_bit(int nr, volatile const void * addr)
  * @nr: the bit to set
  * @addr: the address to start counting from
  *
- * This function is atomic and may not be reordered.  See __set_bit()
- * if you do not require the atomic guarantees.
+ * This function is not atomic and may be reordered.
+ *
  * Note that @nr may be almost arbitrarily large; this function is not
  * restricted to acting on a single-word quantity.
  */
 static __inline__ void set_bit(int nr, volatile void * addr)
 {
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__(
 		"btsl %1,%0"
 		:"=m" (ADDR)
 		:"dIr" (nr) : "memory");
@@ -406,17 +396,14 @@ static __inline__ void set_bit(int nr, volatile void * addr)
  * @nr: Bit to clear
  * @addr: Address to start counting from
  *
- * clear_bit() is atomic and may not be reordered.  However, it does
- * not contain a memory barrier, so if it is used for locking purposes,
- * you should call smp_mb__before_clear_bit() and/or smp_mb__after_clear_bit()
- * in order to ensure changes are visible on other processors.
+ * clear_bit() is not atomic and may be reordered.
  */
 static __inline__ void clear_bit(int nr, volatile void * addr)
 {
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__(
 		"btrl %1,%0"
 		:"=m" (ADDR)
-		:"dIr" (nr));
+		:"dIr" (nr): "memory");
 }
 
 /**
@@ -512,6 +499,8 @@ static inline unsigned long __synch_cmpxchg(volatile void *ptr,
     return old;
 }
 
+/* These are atomic and may not be reordered. They also imply a full memory
+ * barrier. */
 
 static __inline__ void synch_set_bit(int nr, volatile void * addr)
 {

Re: [PATCH] mini-os: fix bit ops comments and memory clobbers.

[Andrew Cooper]

On 18/06/14 09:29, Samuel Thibault wrote:
> This fixes comments about test_and_clear_bit, set_bit and clear_bit,
> which are actually not atomic, can be reordered and are not memory
> barriers.
>
> This also drops the empty LOCK and LOCK_PREFIX macros which bring to the
> confusion.
>
> This also adds missing memory clobbers to set_bit and clear_bit.
>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Do you mean reordered by the compiler or reordered by the pipeline?  The
memory clobbers prevent the compiler reordering, but lack of [lsm]fence
instructions allow the pipeline to reorder the reads/writes if it chooses.

~Andrew

>
> diff --git a/extras/mini-os/include/x86/os.h b/extras/mini-os/include/x86/os.h
> index f193865..510c632 100644
> --- a/extras/mini-os/include/x86/os.h
> +++ b/extras/mini-os/include/x86/os.h
> @@ -152,8 +152,6 @@ do {									\
>  #endif
>  
>  
> -#define LOCK_PREFIX ""
> -#define LOCK ""
>  #define ADDR (*(volatile long *) addr)
>  /*
>   * Make sure gcc doesn't try to be clever and move things around
> @@ -200,15 +198,13 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
>   * @nr: Bit to clear
>   * @addr: Address to count from
>   *
> - * This operation is atomic and cannot be reordered.
> - * It can be reorderdered on other architectures other than x86.
> - * It also implies a memory barrier.
> + * This operation is not atomic and can be reordered.
>   */
>  static inline int test_and_clear_bit(int nr, volatile unsigned long * addr)
>  {
>  	int oldbit;
>  
> -	__asm__ __volatile__( LOCK
> +	__asm__ __volatile__(
>  		"btrl %2,%1\n\tsbbl %0,%0"
>  		:"=r" (oldbit),"=m" (ADDR)
>  		:"Ir" (nr) : "memory");
> @@ -241,22 +237,17 @@ static inline int variable_test_bit(int nr, const volatile unsigned long * addr)
>   * @nr: the bit to set
>   * @addr: the address to start counting from
>   *
> - * This function is atomic and may not be reordered.  See __set_bit()
> - * if you do not require the atomic guarantees.
> - *
> - * Note: there are no guarantees that this function will not be reordered
> - * on non x86 architectures, so if you are writting portable code,
> - * make sure not to rely on its reordering guarantees.
> + * This function is not atomic and may be reordered.
>   *
>   * Note that @nr may be almost arbitrarily large; this function is not
>   * restricted to acting on a single-word quantity.
>   */
>  static inline void set_bit(int nr, volatile unsigned long * addr)
>  {
> -	__asm__ __volatile__( LOCK
> +	__asm__ __volatile__(
>  		"btsl %1,%0"
>  		:"=m" (ADDR)
> -		:"Ir" (nr));
> +		:"Ir" (nr): "memory");
>  }
>  
>  /**
> @@ -271,10 +262,10 @@ static inline void set_bit(int nr, volatile unsigned long * addr)
>   */
>  static inline void clear_bit(int nr, volatile unsigned long * addr)
>  {
> -	__asm__ __volatile__( LOCK
> +	__asm__ __volatile__(
>  		"btrl %1,%0"
>  		:"=m" (ADDR)
> -		:"Ir" (nr));
> +		:"Ir" (nr): "memory");
>  }
>  
>  /**
> @@ -347,14 +338,13 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
>   * @nr: Bit to clear
>   * @addr: Address to count from
>   *
> - * This operation is atomic and cannot be reordered.  
> - * It also implies a memory barrier.
> + * This operation is not atomic and can be reordered.
>   */
>  static __inline__ int test_and_clear_bit(int nr, volatile void * addr)
>  {
>  	int oldbit;
>  
> -	__asm__ __volatile__( LOCK_PREFIX
> +	__asm__ __volatile__(
>  		"btrl %2,%1\n\tsbbl %0,%0"
>  		:"=r" (oldbit),"=m" (ADDR)
>  		:"dIr" (nr) : "memory");
> @@ -388,14 +378,14 @@ static __inline__ int variable_test_bit(int nr, volatile const void * addr)
>   * @nr: the bit to set
>   * @addr: the address to start counting from
>   *
> - * This function is atomic and may not be reordered.  See __set_bit()
> - * if you do not require the atomic guarantees.
> + * This function is not atomic and may be reordered.
> + *
>   * Note that @nr may be almost arbitrarily large; this function is not
>   * restricted to acting on a single-word quantity.
>   */
>  static __inline__ void set_bit(int nr, volatile void * addr)
>  {
> -	__asm__ __volatile__( LOCK_PREFIX
> +	__asm__ __volatile__(
>  		"btsl %1,%0"
>  		:"=m" (ADDR)
>  		:"dIr" (nr) : "memory");
> @@ -406,17 +396,14 @@ static __inline__ void set_bit(int nr, volatile void * addr)
>   * @nr: Bit to clear
>   * @addr: Address to start counting from
>   *
> - * clear_bit() is atomic and may not be reordered.  However, it does
> - * not contain a memory barrier, so if it is used for locking purposes,
> - * you should call smp_mb__before_clear_bit() and/or smp_mb__after_clear_bit()
> - * in order to ensure changes are visible on other processors.
> + * clear_bit() is not atomic and may be reordered.
>   */
>  static __inline__ void clear_bit(int nr, volatile void * addr)
>  {
> -	__asm__ __volatile__( LOCK_PREFIX
> +	__asm__ __volatile__(
>  		"btrl %1,%0"
>  		:"=m" (ADDR)
> -		:"dIr" (nr));
> +		:"dIr" (nr): "memory");
>  }
>  
>  /**
> @@ -512,6 +499,8 @@ static inline unsigned long __synch_cmpxchg(volatile void *ptr,
>      return old;
>  }
>  
> +/* These are atomic and may not be reordered. They also imply a full memory
> + * barrier. */
>  
>  static __inline__ void synch_set_bit(int nr, volatile void * addr)
>  {
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] mini-os: fix bit ops comments and memory clobbers.

[Samuel Thibault]

Andrew Cooper, le Wed 18 Jun 2014 10:30:03 +0100, a écrit :
> On 18/06/14 09:29, Samuel Thibault wrote:
> > This fixes comments about test_and_clear_bit, set_bit and clear_bit,
> > which are actually not atomic, can be reordered and are not memory
> > barriers.
> >
> > This also drops the empty LOCK and LOCK_PREFIX macros which bring to the
> > confusion.
> >
> > This also adds missing memory clobbers to set_bit and clear_bit.
> >
> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> Do you mean reordered by the compiler or reordered by the pipeline?

Yes.

> The memory clobbers prevent the compiler reordering, but lack of
> [lsm]fence instructions allow the pipeline to reorder the reads/writes
> if it chooses.

Ah, actually I was adding the memory clobbers to tell the compiler that
we are modifying memory, but here we already properly tell so through
"=m". Let me redo the patch without it.

Samuel

Re: [PATCH] mini-os: fix bit ops comments and memory clobbers.

[Samuel Thibault]

Samuel Thibault, le Wed 18 Jun 2014 11:33:05 +0200, a écrit :
> Andrew Cooper, le Wed 18 Jun 2014 10:30:03 +0100, a écrit :
> > On 18/06/14 09:29, Samuel Thibault wrote:
> > > This fixes comments about test_and_clear_bit, set_bit and clear_bit,
> > > which are actually not atomic, can be reordered and are not memory
> > > barriers.
> > >
> > > This also drops the empty LOCK and LOCK_PREFIX macros which bring to the
> > > confusion.
> > >
> > > This also adds missing memory clobbers to set_bit and clear_bit.
> > >
> > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > 
> > Do you mean reordered by the compiler or reordered by the pipeline?
> 
> Yes.
> 
> > The memory clobbers prevent the compiler reordering, but lack of
> > [lsm]fence instructions allow the pipeline to reorder the reads/writes
> > if it chooses.
> 
> Ah, actually I was adding the memory clobbers to tell the compiler that
> we are modifying memory, but here we already properly tell so through
> "=m". Let me redo the patch without it.

Actually no, the patch is right: these functions can take an arbitrary
bit position, so the assembly snippet may modify in an arbitrary
position, and thus we have to use the clobber to tell the compiler to
assume that the memory has changed in unpredictable ways.

Samuel

Re: [PATCH] mini-os: fix bit ops comments and memory clobbers.

[Ian Campbell]

On Wed, 2014-06-18 at 10:29 +0200, Samuel Thibault wrote:
> This fixes comments about test_and_clear_bit, set_bit and clear_bit,
> which are actually not atomic, can be reordered and are not memory
> barriers.
> 
> This also drops the empty LOCK and LOCK_PREFIX macros which bring to the
> confusion.

It took me a little while to work out that the reason these aren't
needed is that you have synch_set_bit for the cases where the lock is
actually needed.

> This also adds missing memory clobbers to set_bit and clear_bit.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH] mini-os: fix bit ops comments and memory clobbers.

[Ian Campbell]

On Wed, 2014-06-18 at 15:58 +0100, Ian Campbell wrote:
> On Wed, 2014-06-18 at 10:29 +0200, Samuel Thibault wrote:
> > This fixes comments about test_and_clear_bit, set_bit and clear_bit,
> > which are actually not atomic, can be reordered and are not memory
> > barriers.
> > 
> > This also drops the empty LOCK and LOCK_PREFIX macros which bring to the
> > confusion.
> 
> It took me a little while to work out that the reason these aren't
> needed is that you have synch_set_bit for the cases where the lock is
> actually needed.
> 
> > This also adds missing memory clobbers to set_bit and clear_bit.
> > 
> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

But:

<build>/stubdom/../extras/mini-os/include/x86/mini-os/arch_spinlock.h: In function ‘_raw_spin_lock’:
<build>/stubdom/../extras/mini-os/include/x86/mini-os/arch_spinlock.h:83:10: error: expected ‘:’ or ‘)’ before ‘LOCK’
<build>/stubdom/../extras/mini-os/include/x86/mini-os/arch_spinlock.h: In function ‘_raw_spin_lock_flags’:
<build>/stubdom/../extras/mini-os/include/x86/mini-os/arch_spinlock.h:90:10: error: expected ‘:’ or ‘)’ before ‘LOCK’

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel