atomic64_t proposal

atomic64_t proposal

[Dean Nelson]

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 <davidm@hpl.hp.com>
  */
@@ -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))
 ==============================================================================

Re: atomic64_t proposal

[Andreas Schwab]

Dean Nelson <dcn@sgi.com> 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, schwab@suse.de
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."

Re: atomic64_t proposal

[Dean Nelson]

Andreas Schwab writes:
> 
> Dean Nelson <dcn@sgi.com> 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

Re: atomic64_t proposal

[Dean Nelson]

Andi Kleen writes:
> 
> Dean Nelson <dcn@sgi.com> 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

Re: atomic64_t proposal

[H. Peter Anvin]

Followup to:  <Pine.LNX.4.44.0208281040010.14946-100000@rmholt.homeip.net>
By author:    Robin Holt <holt@rmholt.homeip.net>
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
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt	<amsp@zytor.com>

Re: atomic64_t proposal

[Robin Holt]

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

Re: atomic64_t proposal

[Benjamin LaHaise]

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."

Re: atomic64_t proposal

[David S. Miller]

   From: Andi Kleen <ak@suse.de>
   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.

Re: atomic64_t proposal

[Andi Kleen]

Dean Nelson <dcn@sgi.com> 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