This patch fixes a number of issues with these specializations:
1. The memory operand inside the asm specification is erroneously
declared read-only instead of read-write.
2. There is no reason to require the 1st operand of andl/orl to be
inside a register; the 1st operand could also be an immediate operand.
So change its specification from "r" to "ir".
3. Since addr is supposed to be an atomic_t *, the memory operand
should be addr->counter and not *addr.
4. These specializations should be inline functions instead of macros.
5. Finally, the "memory" clobbers are unnecessary, so they should be
removed. (This is in line with the other atomic functions such as
atomic_add and atomic_sub, the likes of which do not have a "memory"
clobber.)
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Hans-Christian Egtvedt <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Pranith Kumar <[email protected]>
Signed-off-by: Patrick Palka <[email protected]>
---
arch/x86/include/asm/atomic.h | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
index 5e5cd12..83ae239 100644
--- a/arch/x86/include/asm/atomic.h
+++ b/arch/x86/include/asm/atomic.h
@@ -219,15 +219,19 @@ static inline short int atomic_inc_short(short int *v)
return *v;
}
-/* These are x86-specific, used by some header files */
-#define atomic_clear_mask(mask, addr) \
- asm volatile(LOCK_PREFIX "andl %0,%1" \
- : : "r" (~(mask)), "m" (*(addr)) : "memory")
+static inline void atomic_clear_mask(int mask, atomic_t *v)
+{
+ asm volatile(LOCK_PREFIX "andl %1, %0"
+ : "+m" (v->counter)
+ : "ir" (~mask));
+}
-#define atomic_set_mask(mask, addr) \
- asm volatile(LOCK_PREFIX "orl %0,%1" \
- : : "r" ((unsigned)(mask)), "m" (*(addr)) \
- : "memory")
+static inline void atomic_set_mask(int mask, atomic_t *v)
+{
+ asm volatile(LOCK_PREFIX "orl %1, %0"
+ : "+m" (v->counter)
+ : "ir" (mask));
+}
#ifdef CONFIG_X86_32
# include <asm/atomic64_32.h>
--
2.1.2.443.g670a3c1
On Wed, Oct 15, 2014 at 04:38:01PM -0400, Patrick Palka wrote:
> This patch fixes a number of issues with these specializations:
>
> 1. The memory operand inside the asm specification is erroneously
> declared read-only instead of read-write.
>
> 2. There is no reason to require the 1st operand of andl/orl to be
> inside a register; the 1st operand could also be an immediate operand.
> So change its specification from "r" to "ir".
>
> 3. Since addr is supposed to be an atomic_t *, the memory operand
> should be addr->counter and not *addr.
>
> 4. These specializations should be inline functions instead of macros.
>
> 5. Finally, the "memory" clobbers are unnecessary, so they should be
> removed. (This is in line with the other atomic functions such as
> atomic_add and atomic_sub, the likes of which do not have a "memory"
> clobber.)
No real problem with this, but I'm going to kill off these functions
when I get a little time :)
On Thu, Oct 16, 2014 at 12:49:43PM -0400, Patrick Palka wrote:
> On Oct 16, 2014 4:02 AM, "Peter Zijlstra" <[email protected]> wrote:
> >
> > On Wed, Oct 15, 2014 at 04:38:01PM -0400, Patrick Palka wrote:
> > > This patch fixes a number of issues with these specializations:
> > >
> > > 1. The memory operand inside the asm specification is erroneously
> > > declared read-only instead of read-write.
> > >
> > > 2. There is no reason to require the 1st operand of andl/orl to be
> > > inside a register; the 1st operand could also be an immediate operand.
> > > So change its specification from "r" to "ir".
> > >
> > > 3. Since addr is supposed to be an atomic_t *, the memory operand
> > > should be addr->counter and not *addr.
> > >
> > > 4. These specializations should be inline functions instead of macros.
> > >
> > > 5. Finally, the "memory" clobbers are unnecessary, so they should be
> > > removed. (This is in line with the other atomic functions such as
> > > atomic_add and atomic_sub, the likes of which do not have a "memory"
> > > clobber.)
> >
> > No real problem with this, but I'm going to kill off these functions
> > when I get a little time :)
>
> Hmm why's that?
because they're odd (inconsistent with the rest of the atomic
interfaces) and not implemented by all archs.
See: https://lkml.org/lkml/2014/2/6/196
3.18 will include up to 4/5 of that series and when I get a spare moment
I need cleanup/post the next arch sweep that will get us that 5/5 thing.
On Thu, Oct 16, 2014 at 4:24 PM, Peter Zijlstra <[email protected]> wrote:
> On Thu, Oct 16, 2014 at 12:49:43PM -0400, Patrick Palka wrote:
>> On Oct 16, 2014 4:02 AM, "Peter Zijlstra" <[email protected]> wrote:
>> >
>> > On Wed, Oct 15, 2014 at 04:38:01PM -0400, Patrick Palka wrote:
>> > > This patch fixes a number of issues with these specializations:
>> > >
>> > > 1. The memory operand inside the asm specification is erroneously
>> > > declared read-only instead of read-write.
>> > >
>> > > 2. There is no reason to require the 1st operand of andl/orl to be
>> > > inside a register; the 1st operand could also be an immediate operand.
>> > > So change its specification from "r" to "ir".
>> > >
>> > > 3. Since addr is supposed to be an atomic_t *, the memory operand
>> > > should be addr->counter and not *addr.
>> > >
>> > > 4. These specializations should be inline functions instead of macros.
>> > >
>> > > 5. Finally, the "memory" clobbers are unnecessary, so they should be
>> > > removed. (This is in line with the other atomic functions such as
>> > > atomic_add and atomic_sub, the likes of which do not have a "memory"
>> > > clobber.)
>> >
>> > No real problem with this, but I'm going to kill off these functions
>> > when I get a little time :)
>>
>> Hmm why's that?
>
> because they're odd (inconsistent with the rest of the atomic
> interfaces) and not implemented by all archs.
>
> See: https://lkml.org/lkml/2014/2/6/196
>
> 3.18 will include up to 4/5 of that series and when I get a spare moment
> I need cleanup/post the next arch sweep that will get us that 5/5 thing.
>
>
Cool! Perhaps atomic_inc_short() should be killed off too. It
currently has no callers and, despite what its name suggests, it is
not even atomic..