In atomic_set_mask, *addr should be both read and written.
Signed-off-by: Jike Song <[email protected]>
---
arch/x86/include/asm/atomic_32.h | 8 ++++----
arch/x86/include/asm/atomic_64.h | 7 ++++---
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index ad5b9f6..23a7c7f 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -242,12 +242,12 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
/* 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")
+ asm volatile(LOCK_PREFIX "andl %1, %0" \
+ : "+m" (*(addr)) : "r" (~(mask)) : "memory")
#define atomic_set_mask(mask, addr) \
- asm volatile(LOCK_PREFIX "orl %0,%1" \
- : : "r" (mask), "m" (*(addr)) : "memory")
+ asm volatile(LOCK_PREFIX "orl %1, %0" \
+ : "+m" (*(addr)) : "r" (mask) : "memory")
/* Atomic operations are already serializing on x86 */
#define smp_mb__before_atomic_dec() barrier()
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index fa59212..31b34f3 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -451,12 +451,13 @@ static inline void atomic_or_long(unsigned long *v1, unsigned long v2)
/* 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")
+ asm volatile(LOCK_PREFIX "andl %1, %0" \
+ : "+m" (*(addr)) : "r" (~(mask)) : "memory")
#define atomic_set_mask(mask, addr) \
- asm volatile(LOCK_PREFIX "orl %0,%1" \
+ asm volatile(LOCK_PREFIX "orl %1, %0" \
: : "r" ((unsigned)(mask)), "m" (*(addr)) \
+ : "+m" (*(addr)) : "r" ((unsigned)(mask)) \
: "memory")
/* Atomic operations are already serializing on x86 */
--
1.6.0.1
* Jike Song <[email protected]> wrote:
>
> --
that was an easy patch to act upon ;-)
Ingo
On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <[email protected]> wrote:
>
> * Jike Song <[email protected]> wrote:
>>
>> --
>
> that was an easy patch to act upon ;-)
>
> Ingo
>
Sorry Ingo, I missed your point. Do you mean this patch is trivial or
unnecessary?
Besides, by looking at the inline assembly in kernel, I found lots of
codes like this:
static inline void atomic_add(int i, atomic_t *v)
{
asm volatile(LOCK_PREFIX "addl %1,%0"
: "=m" (v->counter)
: "ir" (i), "m" (v->counter));
}
Yes, it works. But a little ugly.. Should this be cleaned-up with the
following?
: "+m" (v->counter)
: "ir" (i)
If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)
--
Thanks,
Jike
Jike Song wrote:
> On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <[email protected]> wrote:
>> * Jike Song <[email protected]> wrote:
>>> --
>> that was an easy patch to act upon ;-)
>>
>> Ingo
>>
> Sorry Ingo, I missed your point. Do you mean this patch is trivial or
> unnecessary?
Your first message was empty.
Jike Song wrote:
>
> Besides, by looking at the inline assembly in kernel, I found lots of
> codes like this:
>
> static inline void atomic_add(int i, atomic_t *v)
> {
> asm volatile(LOCK_PREFIX "addl %1,%0"
> : "=m" (v->counter)
> : "ir" (i), "m" (v->counter));
> }
>
>
> Yes, it works. But a little ugly.. Should this be cleaned-up with the
> following?
>
> : "+m" (v->counter)
> : "ir" (i)
>
> If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)
>
Please don't change them just to change them, if there is no actual
error. You never know when you're going to trigger a new bug in some
weird version of gcc.
-hpa
On Thu, Oct 30, 2008 at 11:54 AM, H. Peter Anvin <[email protected]> wrote:
> Jike Song wrote:
>>
>> Besides, by looking at the inline assembly in kernel, I found lots of
>> codes like this:
>>
>> static inline void atomic_add(int i, atomic_t *v)
>> {
>> asm volatile(LOCK_PREFIX "addl %1,%0"
>> : "=m" (v->counter)
>> : "ir" (i), "m" (v->counter));
>> }
>>
>>
>> Yes, it works. But a little ugly.. Should this be cleaned-up with the
>> following?
>>
>> : "+m" (v->counter)
>> : "ir" (i)
>>
>> If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)
>>
>
> Please don't change them just to change them, if there is no actual
> error. You never know when you're going to trigger a new bug in some
> weird version of gcc.
>
> -hpa
Yes, sometimes gcc did have bugs with its obscure inline asm
conventions. But I think the change of x86-64 atomic operations should
be OK. Anyway, the "+" constraint is more clear than a "=m" output and
a "m" input.
The 32-bit atomic ops were already changed to "+m".(commit
b862f3b099f3ea672c7438c0b282ce8201d39dfc)
--
Thanks,
Jike
Jike Song wrote:
>
> Yes, sometimes gcc did have bugs with its obscure inline asm
> conventions. But I think the change of x86-64 atomic operations should
> be OK. Anyway, the "+" constraint is more clear than a "=m" output and
> a "m" input.
>
> The 32-bit atomic ops were already changed to "+m".(commit
> b862f3b099f3ea672c7438c0b282ce8201d39dfc)
>
You *THINK*. It's very easy to *THINK* that gcc won't do something
utterly moronic, and you'd be wrong.
Just changing it for the sake of churn is pointless... if there is a
bug, then we have to take the risk anyway, but if it is already correct,
then there is no point in provoking a bug. Not *your* bug, because your
code is correct, but gcc's bug.
FWIW, the reason that code doesn't use "+m" is because a version of gcc
which we no longer support didn't handle it. That by itself isn't a
reason to keep it, but there is also no reason to just "tidy" it, IMNSHO.
-hpa
* Jike Song <[email protected]> wrote:
> On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <[email protected]> wrote:
> >
> > * Jike Song <[email protected]> wrote:
> >>
> >> --
> >
> > that was an easy patch to act upon ;-)
> >
> > Ingo
> >
> Sorry Ingo, I missed your point. Do you mean this patch is trivial or
> unnecessary?
nah, it was a joke - you sent two mails, one of them was empty ;-)
Ingo
Ingo Molnar wrote:
>
> nah, it was a joke - you sent two mails, one of them was empty ;-)
>
> Ingo
Yeah, I had problems with git-send-email, it always send two emails(the
first being empty) if I specify --compose ;-) Will take care next time;)
Jike