Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1652566

Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops

From Dmitry Vyukov <dvyukov@google.com>
Newsgroups linux.kernel
Subject Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops
Date 2017-05-29 16:50 +0200
Message-ID <tMu9A-1fm-23@gated-at.bofh.it> (permalink)
References <tLsWd-1pQ-1@gated-at.bofh.it> <tLsWe-1pQ-19@gated-at.bofh.it> <tLTa2-2aI-11@gated-at.bofh.it> <tM2Q1-7s-15@gated-at.bofh.it> <tM2ZH-by-1@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw

On Sun, May 28, 2017 at 11:34 AM,  <hpa@zytor.com> wrote:
> On May 28, 2017 2:29:32 AM PDT, Dmitry Vyukov <dvyukov@google.com> wrote:
>>On Sun, May 28, 2017 at 1:02 AM,  <hpa@zytor.com> wrote:
>>> On May 26, 2017 12:09:04 PM PDT, Dmitry Vyukov <dvyukov@google.com>
>>wrote:
>>>>Some 64-bit atomic operations use 'long long' as operand/return type
>>>>(e.g. asm-generic/atomic64.h, arch/x86/include/asm/atomic64_32.h);
>>>>while others use 'long' (e.g. arch/x86/include/asm/atomic64_64.h).
>>>>This makes it impossible to write portable code.
>>>>For example, there is no format specifier that prints result of
>>>>atomic64_read() without warnings. atomic64_try_cmpxchg() is almost
>>>>impossible to use in portable fashion because it requires either
>>>>'long *' or 'long long *' as argument depending on arch.
>>>>
>>>>Switch arch/x86/include/asm/atomic64_64.h to 'long long'.
>>>>
>>>>Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>>>Cc: Mark Rutland <mark.rutland@arm.com>
>>>>Cc: Peter Zijlstra <peterz@infradead.org>
>>>>Cc: Will Deacon <will.deacon@arm.com>
>>>>Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
>>>>Cc: Ingo Molnar <mingo@redhat.com>
>>>>Cc: kasan-dev@googlegroups.com
>>>>Cc: linux-mm@kvack.org
>>>>Cc: linux-kernel@vger.kernel.org
>>>>Cc: x86@kernel.org
>>>>
>>>>---
>>>>Changes since v1:
>>>> - reverted stray s/long/long long/ replace in comment
>>>> - added arch/s390 changes to fix build errors/warnings
>>>>---

[snip]

>>> NAK - this is what u64/s64 is for.
>>
>>
>>Hi,
>>
>>Patch 3 adds atomic-instrumented.h which now contains:
>>
>>+static __always_inline long long atomic64_read(const atomic64_t *v)
>>+{
>>+       return arch_atomic64_read(v);
>>+}
>>
>>without this patch that will become
>>
>>+static __always_inline s64 atomic64_read(const atomic64_t *v)
>>
>>Right?
>
> Yes.


I see that s64 is not the same as long on x86_64 (long long vs long),
so it's still not possible to e.g. portably print a result of
atomic_read(). But it's a separate issue that we don't need to solve
now.

Also all wrappers like:

void atomic64_set(atomic64_t *v, s64 i)
{
    arch_atomic64_set(v, i);
}

lead to type conversions, but at least my compiler does not bark on it.

The only remaining problem is with atomic64_try_cmpxchg, which is
simply not possible to use now (not possible to declare the *old
type).
I will need something along the following lines to fix it (then
callers can use s64* for old).
Sounds good?


--- a/arch/x86/include/asm/atomic64_64.h
+++ b/arch/x86/include/asm/atomic64_64.h
@@ -177,7 +177,7 @@ static inline long
arch_atomic64_cmpxchg(atomic64_t *v, long old, long new)
 }

 #define arch_atomic64_try_cmpxchg arch_atomic64_try_cmpxchg
-static __always_inline bool arch_atomic64_try_cmpxchg(atomic64_t *v,
long *old, long new)
+static __always_inline bool arch_atomic64_try_cmpxchg(atomic64_t *v,
s64 *old, long new)
 {
        return try_cmpxchg(&v->counter, old, new);
 }
@@ -198,7 +198,7 @@ static inline long arch_atomic64_xchg(atomic64_t
*v, long new)
  */
 static inline bool arch_atomic64_add_unless(atomic64_t *v, long a, long u)
 {
-       long c = arch_atomic64_read(v);
+       s64 c = arch_atomic64_read(v);
        do {
                if (unlikely(c == u))
                        return false;
@@ -217,7 +217,7 @@ static inline bool
arch_atomic64_add_unless(atomic64_t *v, long a, long u)
  */
 static inline long arch_atomic64_dec_if_positive(atomic64_t *v)
 {
-       long dec, c = arch_atomic64_read(v);
+       s64 dec, c = arch_atomic64_read(v);
        do {
                dec = c - 1;
                if (unlikely(dec < 0))
@@ -236,7 +236,7 @@ static inline void arch_atomic64_and(long i, atomic64_t *v)

 static inline long arch_atomic64_fetch_and(long i, atomic64_t *v)
 {
-       long val = arch_atomic64_read(v);
+       s64 val = arch_atomic64_read(v);

        do {
        } while (!arch_atomic64_try_cmpxchg(v, &val, val & i));
@@ -253,7 +253,7 @@ static inline void arch_atomic64_or(long i, atomic64_t *v)

 static inline long arch_atomic64_fetch_or(long i, atomic64_t *v)
 {
-       long val = arch_atomic64_read(v);
+       s64 val = arch_atomic64_read(v);

        do {
        } while (!arch_atomic64_try_cmpxchg(v, &val, val | i));
@@ -270,7 +270,7 @@ static inline void arch_atomic64_xor(long i, atomic64_t *v)

 static inline long arch_atomic64_fetch_xor(long i, atomic64_t *v)
 {
-       long val = arch_atomic64_read(v);
+       s64 val = arch_atomic64_read(v);

        do {
        } while (!arch_atomic64_try_cmpxchg(v, &val, val ^ i));
diff --git a/arch/x86/include/asm/cmpxchg.h b/arch/x86/include/asm/cmpxchg.h
index e8cf95908fe5..9e2faa85eb02 100644
--- a/arch/x86/include/asm/cmpxchg.h
+++ b/arch/x86/include/asm/cmpxchg.h
@@ -157,7 +157,7 @@ extern void __add_wrong_size(void)
 #define __raw_try_cmpxchg(_ptr, _pold, _new, size, lock)               \
 ({                                                                     \
        bool success;                                                   \
-       __typeof__(_ptr) _old = (_pold);                                \
+       __typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);              \
        __typeof__(*(_ptr)) __old = *_old;                              \
        __typeof__(*(_ptr)) __new = (_new);                             \
        switch (size) {                                                 \

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2 0/7] x86, kasan: add KASAN checks to atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-05-26 21:20 +0200
  [PATCH v2 6/7] asm-generic: add KASAN instrumentation to atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-05-26 21:20 +0200
  [PATCH v2 3/7] asm-generic: add atomic-instrumented.h Dmitry Vyukov <dvyukov@google.com> - 2017-05-26 21:20 +0200
  [PATCH v2 1/7] x86: un-macro-ify atomic ops implementation Dmitry Vyukov <dvyukov@google.com> - 2017-05-26 21:20 +0200
  [PATCH v2 7/7] asm-generic, x86: add comments for atomic instrumentation Dmitry Vyukov <dvyukov@google.com> - 2017-05-26 21:20 +0200
  [PATCH v2 4/7] x86: switch atomic.h to use atomic-instrumented.h Dmitry Vyukov <dvyukov@google.com> - 2017-05-26 21:20 +0200
  [PATCH v2 5/7] kasan: allow kasan_check_read/write() to accept pointers to volatiles Dmitry Vyukov <dvyukov@google.com> - 2017-05-26 21:20 +0200
  [PATCH v2 2/7] x86: use long long for 64-bit atomic ops Dmitry Vyukov <dvyukov@google.com> - 2017-05-26 21:20 +0200
    Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops hpa@zytor.com - 2017-05-28 01:20 +0200
      Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops Dmitry Vyukov <dvyukov@google.com> - 2017-05-28 11:40 +0200
        Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops hpa@zytor.com - 2017-05-28 11:50 +0200
          Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops Dmitry Vyukov <dvyukov@google.com> - 2017-05-29 16:50 +0200
            Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops Dmitry Vyukov <dvyukov@google.com> - 2017-06-06 12:20 +0200
    Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-05-29 12:50 +0200
      Re: [PATCH v2 2/7] x86: use long long for 64-bit atomic ops Dmitry Vyukov <dvyukov@google.com> - 2017-05-29 13:10 +0200

csiph-web