I'm proposing the creation of an atomic64_t variable, which is a 64-bit
version of atomic_t, and the usage of the __typeof__ keyword in macro versions
of the atomic operations to enable them to operate on either type (atomic_t and
atomic64_t).
I submitted the following patch to David Mosberger to be considered for
inclusion in the IA-64 linux kernel. He suggested that I bring the topic up
on this list so that the other 64-bit platform maintainers can commment.
Thanks,
Dean
==============================================================================
diff -Naur 2.4.18-ia64/linux/include/asm-ia64/atomic.h linux/linux/include/asm-ia64/atomic.h
--- 2.4.18-ia64/linux/include/asm-ia64/atomic.h Wed Jul 17 07:12:34 2002
+++ linux/linux/include/asm-ia64/atomic.h Wed Aug 21 18:12:13 2002
@@ -5,10 +5,6 @@
* Atomic operations that C can't guarantee us. Useful for
* resource counting etc..
*
- * NOTE: don't mess with the types below! The "unsigned long" and
- * "int" types were carefully placed so as to ensure proper operation
- * of the macros.
- *
* Copyright (C) 1998, 1999, 2002 Hewlett-Packard Co
* David Mosberger-Tang <[email protected]>
*/
@@ -21,63 +17,59 @@
* memory accesses are ordered.
*/
typedef struct { volatile __s32 counter; } atomic_t;
+typedef struct { volatile __s64 counter; } atomic64_t;
#define ATOMIC_INIT(i) ((atomic_t) { (i) })
+#define ATOMIC64_INIT(i) ((atomic64_t) { (i) })
#define atomic_read(v) ((v)->counter)
#define atomic_set(v,i) (((v)->counter) = (i))
-static __inline__ int
-ia64_atomic_add (int i, atomic_t *v)
-{
- __s32 old, new;
- CMPXCHG_BUGCHECK_DECL
-
- do {
- CMPXCHG_BUGCHECK(v);
- old = atomic_read(v);
- new = old + i;
- } while (ia64_cmpxchg("acq", v, old, old + i, sizeof(atomic_t)) != old);
- return new;
-}
-
-static __inline__ int
-ia64_atomic_sub (int i, atomic_t *v)
-{
- __s32 old, new;
- CMPXCHG_BUGCHECK_DECL
-
- do {
- CMPXCHG_BUGCHECK(v);
- old = atomic_read(v);
- new = old - i;
- } while (ia64_cmpxchg("acq", v, old, new, sizeof(atomic_t)) != old);
- return new;
-}
+#define ia64_atomic_add(i,v) \
+({ \
+ __typeof__((v)->counter) _old, _new; \
+ CMPXCHG_BUGCHECK_DECL \
+ \
+ do { \
+ CMPXCHG_BUGCHECK(v); \
+ _old = atomic_read(v); \
+ _new = _old + (i); \
+ } while (ia64_cmpxchg("acq", (v), _old, _new, sizeof(*(v))) != _old); \
+ (__typeof__((v)->counter)) _new; /* return new value */ \
+})
+
+#define ia64_atomic_sub(i,v) \
+({ \
+ __typeof__((v)->counter) _old, _new; \
+ CMPXCHG_BUGCHECK_DECL \
+ \
+ do { \
+ CMPXCHG_BUGCHECK(v); \
+ _old = atomic_read(v); \
+ _new = _old - (i); \
+ } while (ia64_cmpxchg("acq", (v), _old, _new, sizeof(*(v))) != _old); \
+ (__typeof__((v)->counter)) _new; /* return new value */ \
+})
/*
* Atomically add I to V and return TRUE if the resulting value is
* negative.
*/
-static __inline__ int
-atomic_add_negative (int i, atomic_t *v)
-{
- return ia64_atomic_add(i, v) < 0;
-}
+#define atomic_add_negative(i,v) (ia64_atomic_add((i), (v)) < 0)
#define atomic_add_return(i,v) \
((__builtin_constant_p(i) && \
( (i == 1) || (i == 4) || (i == 8) || (i == 16) \
|| (i == -1) || (i == -4) || (i == -8) || (i == -16))) \
? ia64_fetch_and_add(i, &(v)->counter) \
- : ia64_atomic_add(i, v))
+ : ia64_atomic_add((i), (v)))
#define atomic_sub_return(i,v) \
((__builtin_constant_p(i) && \
( (i == 1) || (i == 4) || (i == 8) || (i == 16) \
|| (i == -1) || (i == -4) || (i == -8) || (i == -16))) \
? ia64_fetch_and_add(-(i), &(v)->counter) \
- : ia64_atomic_sub(i, v))
+ : ia64_atomic_sub((i), (v)))
#define atomic_dec_return(v) atomic_sub_return(1, (v))
#define atomic_inc_return(v) atomic_add_return(1, (v))
==============================================================================
Dean Nelson <[email protected]> writes:
> I'm proposing the creation of an atomic64_t variable, which is a 64-bit
> version of atomic_t, and the usage of the __typeof__ keyword in macro versions
> of the atomic operations to enable them to operate on either type (atomic_t and
> atomic64_t).
>
> I submitted the following patch to David Mosberger to be considered for
> inclusion in the IA-64 linux kernel. He suggested that I bring the topic up
> on this list so that the other 64-bit platform maintainers can commment.
Wouldn't it be much cleaner to just define atomic64_add/sub/read etc. ?
That would make the macros much nicer.
On x86-64 it would be fine this way.
Is it supposed to only work on 64bit or do you plan to supply it for 32
bit too? If no, I don't see how drivers etc. should ever use it. linux
is supposed to have a common kernel api.
If yes, the implementation on 32bit could be a problem. e.g. some
archs need space in there for spinlocks, so it would be needed to limit
the usable range.
-Andi
Dean Nelson <[email protected]> writes:
|> +#define ia64_atomic_add(i,v) \
|> +({ \
|> + __typeof__((v)->counter) _old, _new; \
|> + CMPXCHG_BUGCHECK_DECL \
|> + \
|> + do { \
|> + CMPXCHG_BUGCHECK(v); \
|> + _old = atomic_read(v); \
|> + _new = _old + (i); \
|> + } while (ia64_cmpxchg("acq", (v), _old, _new, sizeof(*(v))) != _old); \
|> + (__typeof__((v)->counter)) _new; /* return new value */ \
What's the purpose of the cast here? The type of _new is already the
right one.
|> #define atomic_add_return(i,v) \
|> ((__builtin_constant_p(i) && \
|> ( (i == 1) || (i == 4) || (i == 8) || (i == 16) \
|> || (i == -1) || (i == -4) || (i == -8) || (i == -16))) \
|> ? ia64_fetch_and_add(i, &(v)->counter) \
|> - : ia64_atomic_add(i, v))
|> + : ia64_atomic_add((i), (v)))
The extra parens are useless.
Andreas.
--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
From: Andi Kleen <[email protected]>
Date: 27 Aug 2002 21:58:45 +0200
If yes, the implementation on 32bit could be a problem. e.g. some
archs need space in there for spinlocks, so it would be needed to limit
the usable range.
x86 would need 1 bit, some other 32-bit platforms would be ok
with just a byte (ie. 8 bits).
I don't like this whole transparent idea just like you Andi.
People should ask for the types they want, if they want 64-bits
of counter, they should explicitly use atomic64_t and use the
atomic64_t operations on that type.
On Tue, Aug 27, 2002 at 09:58:45PM +0200, Andi Kleen wrote:
> Is it supposed to only work on 64bit or do you plan to supply it for 32
> bit too? If no, I don't see how drivers etc. should ever use it. linux
> is supposed to have a common kernel api.
> If yes, the implementation on 32bit could be a problem. e.g. some
> archs need space in there for spinlocks, so it would be needed to limit
> the usable range.
There are a couple of options for implementations to use that don't
require space for a spinlock: a hash table of spinlocks can be used
to protect the data (parisc uses this technique). Andrea's lockless
reader locks could be useful in this case. Most x86es can use cmpxchg8,
and the 64 bit machines are already set. I suspect it would be a useful
addition to the kernel APIs.
-ben
--
"You will be reincarnated as a toad; and you will be much happier."
Andreas Schwab writes:
>
> Dean Nelson <[email protected]> writes:
>
> |> +#define ia64_atomic_add(i,v) \
> |> +({ \
> |> + __typeof__((v)->counter) _old, _new; \
> |> + CMPXCHG_BUGCHECK_DECL \
> |> + \
> |> + do { \
> |> + CMPXCHG_BUGCHECK(v); \
> |> + _old = atomic_read(v); \
> |> + _new = _old + (i); \
> |> + } while (ia64_cmpxchg("acq", (v), _old, _new, sizeof(*(v))) != _old); \
> |> + (__typeof__((v)->counter)) _new; /* return new value */ \
>
> What's the purpose of the cast here? The type of _new is already the
> right one.
You're right, the cast is meaningless and should be removed. It's an artifact
of a macro that had several iterations of development.
> |> #define atomic_add_return(i,v) \
> |> ((__builtin_constant_p(i) && \
> |> ( (i == 1) || (i == 4) || (i == 8) || (i == 16) \
> |> || (i == -1) || (i == -4) || (i == -8) || (i == -16))) \
> |> ? ia64_fetch_and_add(i, &(v)->counter) \
> |> - : ia64_atomic_add(i, v))
> |> + : ia64_atomic_add((i), (v)))
>
> The extra parens are useless.
Yep, they're useless. I had introduced them merely to be consistent with
how the other macros in asm-ia64/atomic.h were done (macros that I didn't
modify). But the parentheses can be removed.
Thanks for the corrections.
Dean
I do like the atomic_inc, atomic_dec, etc being able to handle either
type. While producing code, I can do a simple check at the beginning of
the block and define the appropriate type for a particular architecture.
For instance:
#if ARCH_WORD_SIZE == 8
atomic64_t my_atomic;
#else
atomic_t my_atomic;
#endif
Without it, I end up doing alot of other magic. I don't see a problem
with having the #defines versus the inlines. Why have two chunks of code
which do exactly the same operations with only type differences?
Robin Holt
Followup to: <[email protected]>
By author: Robin Holt <[email protected]>
In newsgroup: linux.dev.kernel
>
> I do like the atomic_inc, atomic_dec, etc being able to handle either
> type. While producing code, I can do a simple check at the beginning of
> the block and define the appropriate type for a particular architecture.
>
Great. How do you expect to implement atomic_inc() et al so that that
can actually be done? Consider that atomic64_t may very well need
full-blown spinlocks, whereas a 32-bit atomic_t may not.
-hpa
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt <[email protected]>
Andi Kleen writes:
>
> Dean Nelson <[email protected]> writes:
>
> > I'm proposing the creation of an atomic64_t variable, which is a 64-bit
> > version of atomic_t, and the usage of the __typeof__ keyword in macro versions
> > of the atomic operations to enable them to operate on either type (atomic_t and
> > atomic64_t).
> >
> > I submitted the following patch to David Mosberger to be considered for
> > inclusion in the IA-64 linux kernel. He suggested that I bring the topic up
> > on this list so that the other 64-bit platform maintainers can commment.
>
> Wouldn't it be much cleaner to just define atomic64_add/sub/read etc. ?
> That would make the macros much nicer.
>
> On x86-64 it would be fine this way.
>
> Is it supposed to only work on 64bit or do you plan to supply it for 32
> bit too? If no, I don't see how drivers etc. should ever use it. linux
> is supposed to have a common kernel api.
> If yes, the implementation on 32bit could be a problem. e.g. some
> archs need space in there for spinlocks, so it would be needed to limit
> the usable range.
Your point about a common kernel api (across all architectures) is valid
and leads me to reconsider the use of common macros for the two atomic types.
So I guess I would lean in the direction you suggested of separate macros
(atomic64_add/sub/read etc.) for the atomic64_t type.
But I'm wondering if it would be acceptable to have the atomic64_t implemented
(initially) on only one platform?
My original intent was to get atomic64_t into the IA-64 linux kernel.
Mosberger suggested that the other 64-bit architecture maintainers should
weigh in on this issue and that I send the proposal to lkml.
I have no plans on implementing this for anything but the IA-64 linux kernel.
But its api should be discussed and approved (or disapproved) by this list.
The implementations for the other platforms can come as other people feel
so moved to do them.
Dean