From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86: simplify non-atomic bitops Date: Mon, 19 Jan 2015 17:21:02 +0000 Message-ID: <54BD3CFE.1040200@citrix.com> References: <54BD3639020000780005692B@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YDG0h-0004iK-Sa for xen-devel@lists.xenproject.org; Mon, 19 Jan 2015 17:21:08 +0000 In-Reply-To: <54BD3639020000780005692B@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Keir Fraser List-Id: xen-devel@lists.xenproject.org On 19/01/15 15:52, Jan Beulich wrote: > - being non-atomic, their pointer arguments shouldn't be volatile- > qualified > - their (half fake) memory operands can be a single "+m" instead of > being both an output and an input > > Signed-off-by: Jan Beulich There is a note at the top of the file describing why "+m" is specifically not used. I have not looked into the reason yet, but a patch like this at leasts needs an adjustment to the comment if you believe it to be safe. ~Andrew > > --- a/xen/include/asm-x86/bitops.h > +++ b/xen/include/asm-x86/bitops.h > @@ -53,12 +53,9 @@ static inline void set_bit(int nr, volat > * If it's called on the same region of memory simultaneously, the effect > * may be that only one operation succeeds. > */ > -static inline void __set_bit(int nr, volatile void *addr) > +static inline void __set_bit(int nr, void *addr) > { > - asm volatile ( > - "btsl %1,%0" > - : "=m" (ADDR) > - : "Ir" (nr), "m" (ADDR) : "memory"); > + asm volatile ( "btsl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" ); > } > #define __set_bit(nr, addr) ({ \ > if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > @@ -93,12 +90,9 @@ static inline void clear_bit(int nr, vol > * If it's called on the same region of memory simultaneously, the effect > * may be that only one operation succeeds. > */ > -static inline void __clear_bit(int nr, volatile void *addr) > +static inline void __clear_bit(int nr, void *addr) > { > - asm volatile ( > - "btrl %1,%0" > - : "=m" (ADDR) > - : "Ir" (nr), "m" (ADDR) : "memory"); > + asm volatile ( "btrl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" ); > } > #define __clear_bit(nr, addr) ({ \ > if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > @@ -114,12 +108,9 @@ static inline void __clear_bit(int nr, v > * If it's called on the same region of memory simultaneously, the effect > * may be that only one operation succeeds. > */ > -static inline void __change_bit(int nr, volatile void *addr) > +static inline void __change_bit(int nr, void *addr) > { > - asm volatile ( > - "btcl %1,%0" > - : "=m" (ADDR) > - : "Ir" (nr), "m" (ADDR) : "memory"); > + asm volatile ( "btcl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" ); > } > #define __change_bit(nr, addr) ({ \ > if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > @@ -179,14 +170,13 @@ static inline int test_and_set_bit(int n > * If two examples of this operation race, one can appear to succeed > * but actually fail. You must protect multiple accesses with a lock. > */ > -static inline int __test_and_set_bit(int nr, volatile void *addr) > +static inline int __test_and_set_bit(int nr, void *addr) > { > int oldbit; > > asm volatile ( > "btsl %2,%1\n\tsbbl %0,%0" > - : "=r" (oldbit), "=m" (ADDR) > - : "Ir" (nr), "m" (ADDR) : "memory"); > + : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" ); > return oldbit; > } > #define __test_and_set_bit(nr, addr) ({ \ > @@ -226,14 +216,13 @@ static inline int test_and_clear_bit(int > * If two examples of this operation race, one can appear to succeed > * but actually fail. You must protect multiple accesses with a lock. > */ > -static inline int __test_and_clear_bit(int nr, volatile void *addr) > +static inline int __test_and_clear_bit(int nr, void *addr) > { > int oldbit; > > asm volatile ( > "btrl %2,%1\n\tsbbl %0,%0" > - : "=r" (oldbit), "=m" (ADDR) > - : "Ir" (nr), "m" (ADDR) : "memory"); > + : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" ); > return oldbit; > } > #define __test_and_clear_bit(nr, addr) ({ \ > @@ -242,14 +231,13 @@ static inline int __test_and_clear_bit(i > }) > > /* WARNING: non atomic and it can be reordered! */ > -static inline int __test_and_change_bit(int nr, volatile void *addr) > +static inline int __test_and_change_bit(int nr, void *addr) > { > int oldbit; > > asm volatile ( > "btcl %2,%1\n\tsbbl %0,%0" > - : "=r" (oldbit), "=m" (ADDR) > - : "Ir" (nr), "m" (ADDR) : "memory"); > + : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" ); > return oldbit; > } > #define __test_and_change_bit(nr, addr) ({ \ > > >