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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxxxx>

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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel


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