| Message ID | 5242498F.8060407@asianux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote: > atomic* value is signed value, and atomic* functions need also process > signed value (parameter value, and return value), so 32-bit arm need > use 'long long' instead of 'u64'. > > After replacement, it will also fix a bug for atomic64_add_negative(): > "u64 is never less than 0". > > The modifications are: > > in vim, use "1,% s/\<u64\>/long long/g" command. > remove '__aligned(8)' which is useless for 64-bit. > be sure of 80 column limitation after replacement. > > > Signed-off-by: Chen Gang <gang.chen@asianux.com> Looks better to me, thanks. While you're here, we could also replace the use of `unsigned long' with `int' for the 32-bit atomics, then the whole header is consistent with the generic types. Will
On 09/26/2013 12:07 AM, Will Deacon wrote: > On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote: >> atomic* value is signed value, and atomic* functions need also process >> signed value (parameter value, and return value), so 32-bit arm need >> use 'long long' instead of 'u64'. >> >> After replacement, it will also fix a bug for atomic64_add_negative(): >> "u64 is never less than 0". >> >> The modifications are: >> >> in vim, use "1,% s/\<u64\>/long long/g" command. >> remove '__aligned(8)' which is useless for 64-bit. >> be sure of 80 column limitation after replacement. >> >> >> Signed-off-by: Chen Gang <gang.chen@asianux.com> > > Looks better to me, thanks. While you're here, we could also replace the use > of `unsigned long' with `int' for the 32-bit atomics, then the whole header > is consistent with the generic types. > > Will > > Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in another patch. Firstly, this 'fix' is not belong to API, and either, it has no related 'standard' 'demo' in "asm-generic/" (it is architecture independent, so no related inline assembly code). After a random check, another 3 architectures (maybe more) are also may using 'unsigned long': "arm64/include/asm/atomic.h", "sh/include/asm/atomic-llsc.h", and "xtensa/include/asm/atomic.h". And as far as I know, for a register related variable, 'unsigned long' is also a common using way for both 32-bit and 64-bit (at least, it is a simple way to prevent issues). Thanks.
On Thu, Sep 26, 2013 at 03:00:30AM +0100, Chen Gang wrote: > On 09/26/2013 12:07 AM, Will Deacon wrote: > > On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote: > >> atomic* value is signed value, and atomic* functions need also process > >> signed value (parameter value, and return value), so 32-bit arm need > >> use 'long long' instead of 'u64'. > >> > >> After replacement, it will also fix a bug for atomic64_add_negative(): > >> "u64 is never less than 0". > >> > >> The modifications are: > >> > >> in vim, use "1,% s/\<u64\>/long long/g" command. > >> remove '__aligned(8)' which is useless for 64-bit. > >> be sure of 80 column limitation after replacement. > >> > >> > >> Signed-off-by: Chen Gang <gang.chen@asianux.com> > > > > Looks better to me, thanks. While you're here, we could also replace the use > > of `unsigned long' with `int' for the 32-bit atomics, then the whole header > > is consistent with the generic types. > > > > Will > > > > > > Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in > another patch. Sure, it can be a follow-up to this one. > Firstly, this 'fix' is not belong to API, and either, it has no related > 'standard' 'demo' in "asm-generic/" (it is architecture independent, so > no related inline assembly code). I was simply going by the types of atomic_t and atomic64_t, which are both constructed using signed types. > After a random check, another 3 architectures (maybe more) are also may > using 'unsigned long': "arm64/include/asm/atomic.h", > "sh/include/asm/atomic-llsc.h", and "xtensa/include/asm/atomic.h". > > And as far as I know, for a register related variable, 'unsigned long' > is also a common using way for both 32-bit and 64-bit (at least, it is a > simple way to prevent issues). Maybe, but atomic_t is always 32-bit and atomic64_t is always 64-bit, so I don't think unsigned long buys you anything with regards to sizing. Will
On 09/26/2013 06:04 PM, Will Deacon wrote: > On Thu, Sep 26, 2013 at 03:00:30AM +0100, Chen Gang wrote: >> On 09/26/2013 12:07 AM, Will Deacon wrote: >>> On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote: >>>> atomic* value is signed value, and atomic* functions need also process >>>> signed value (parameter value, and return value), so 32-bit arm need >>>> use 'long long' instead of 'u64'. >>>> >>>> After replacement, it will also fix a bug for atomic64_add_negative(): >>>> "u64 is never less than 0". >>>> >>>> The modifications are: >>>> >>>> in vim, use "1,% s/\<u64\>/long long/g" command. >>>> remove '__aligned(8)' which is useless for 64-bit. >>>> be sure of 80 column limitation after replacement. >>>> >>>> >>>> Signed-off-by: Chen Gang <gang.chen@asianux.com> >>> >>> Looks better to me, thanks. While you're here, we could also replace the use >>> of `unsigned long' with `int' for the 32-bit atomics, then the whole header >>> is consistent with the generic types. >>> >>> Will >>> >>> >> >> Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in >> another patch. > > Sure, it can be a follow-up to this one. > OK, if it is really a useful patch too, I will follow-up. >> Firstly, this 'fix' is not belong to API, and either, it has no related >> 'standard' 'demo' in "asm-generic/" (it is architecture independent, so >> no related inline assembly code). > > I was simply going by the types of atomic_t and atomic64_t, which are both > constructed using signed types. > So, it is not quite necessary (but still better) to let it 'consistent' with 'generic' types (in fact, I feel, for a register related variable, 'unsigned long' is more 'generic' than 'int'). Since it is not belong to API, if some architectures have some reasons (or excuse) to keep 'inconsistent' with 'generic' types, it is still acceptable. >> After a random check, another 3 architectures (maybe more) are also may >> using 'unsigned long': "arm64/include/asm/atomic.h", >> "sh/include/asm/atomic-llsc.h", and "xtensa/include/asm/atomic.h". >> >> And as far as I know, for a register related variable, 'unsigned long' >> is also a common using way for both 32-bit and 64-bit (at least, it is a >> simple way to prevent issues). > > Maybe, but atomic_t is always 32-bit and atomic64_t is always 64-bit, so I > don't think unsigned long buys you anything with regards to sizing. > 'unsigned long' can be used as a register related variable, it is always 32-bit under 32-bit machine, and always 64-bit under 64-bit machine. So can use it for both arm and arm64, for arm, it can not cause issue, and for arm64, it is also OK (if changed to 'int' under arm64, may cause real issue). So I feel, current arm/arm64 implementation for 'unsigned long' is well done, not need additional improvement. > Will > > Thanks.
On Thu, Sep 26, 2013 at 12:03:43PM +0100, Chen Gang wrote: > On 09/26/2013 06:04 PM, Will Deacon wrote: > > On Thu, Sep 26, 2013 at 03:00:30AM +0100, Chen Gang wrote: > >> On 09/26/2013 12:07 AM, Will Deacon wrote: > >>> On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote: > >>>> atomic* value is signed value, and atomic* functions need also process > >>>> signed value (parameter value, and return value), so 32-bit arm need > >>>> use 'long long' instead of 'u64'. > >>>> > >>>> After replacement, it will also fix a bug for atomic64_add_negative(): > >>>> "u64 is never less than 0". > >>>> > >>>> The modifications are: > >>>> > >>>> in vim, use "1,% s/\<u64\>/long long/g" command. > >>>> remove '__aligned(8)' which is useless for 64-bit. > >>>> be sure of 80 column limitation after replacement. > >>>> > >>>> > >>>> Signed-off-by: Chen Gang <gang.chen@asianux.com> > >>> > >>> Looks better to me, thanks. While you're here, we could also replace the use > >>> of `unsigned long' with `int' for the 32-bit atomics, then the whole header > >>> is consistent with the generic types. > >>> > >>> Will > >>> > >>> > >> > >> Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in > >> another patch. > > > > Sure, it can be a follow-up to this one. > > > > OK, if it is really a useful patch too, I will follow-up. It's a cosmetic change to bring the signedness of our 32-bit atomics in-line with your proposal for 64-bit atomics. > 'unsigned long' can be used as a register related variable, it is always > 32-bit under 32-bit machine, and always 64-bit under 64-bit machine. Not necessarily (c.f. ILP32). > So can use it for both arm and arm64, for arm, it can not cause issue, > and for arm64, it is also OK (if changed to 'int' under arm64, may cause > real issue). arm and arm64 have different instructions sets. There's no way the inline assembly used to implement the atomic operations can be made portable between them. Will
On 09/27/2013 07:06 PM, Will Deacon wrote: > On Thu, Sep 26, 2013 at 12:03:43PM +0100, Chen Gang wrote: >> On 09/26/2013 06:04 PM, Will Deacon wrote: >>> On Thu, Sep 26, 2013 at 03:00:30AM +0100, Chen Gang wrote: >>>> On 09/26/2013 12:07 AM, Will Deacon wrote: >>>>> On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote: >>>>>> atomic* value is signed value, and atomic* functions need also process >>>>>> signed value (parameter value, and return value), so 32-bit arm need >>>>>> use 'long long' instead of 'u64'. >>>>>> >>>>>> After replacement, it will also fix a bug for atomic64_add_negative(): >>>>>> "u64 is never less than 0". >>>>>> >>>>>> The modifications are: >>>>>> >>>>>> in vim, use "1,% s/\<u64\>/long long/g" command. >>>>>> remove '__aligned(8)' which is useless for 64-bit. >>>>>> be sure of 80 column limitation after replacement. >>>>>> >>>>>> >>>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com> >>>>> >>>>> Looks better to me, thanks. While you're here, we could also replace the use >>>>> of `unsigned long' with `int' for the 32-bit atomics, then the whole header >>>>> is consistent with the generic types. >>>>> >>>>> Will >>>>> >>>>> >>>> >>>> Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in >>>> another patch. >>> >>> Sure, it can be a follow-up to this one. >>> >> >> OK, if it is really a useful patch too, I will follow-up. > > It's a cosmetic change to bring the signedness of our 32-bit atomics in-line > with your proposal for 64-bit atomics. > >> 'unsigned long' can be used as a register related variable, it is always >> 32-bit under 32-bit machine, and always 64-bit under 64-bit machine. > > Not necessarily (c.f. ILP32). > >> So can use it for both arm and arm64, for arm, it can not cause issue, >> and for arm64, it is also OK (if changed to 'int' under arm64, may cause >> real issue). > > arm and arm64 have different instructions sets. There's no way the inline > assembly used to implement the atomic operations can be made portable > between them. > Hmm... if you like, I will send a follow-up patch for it, the reason is: It is belong to internal implementation, not belong to API, so if it is harmless, better to follow related maintainers' 'hobby' or 'tastes', it will let the related code more clearer. :-) I will wait 1-2 days to let another reviewers check, if no reply, I will send 2 patches for it. (if you already applied the "u64 -> long long" patch, please let me know, and I will send one patch enough). > Will > > Thanks.
It contents 2 patches:
Patch 1: use 'long long' instead of 'u64' for within atomic.h.
Patch 2: use 'int' instead of 'unsigned long' for normal register
variables within atomic.h
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
arch/arm/include/asm/atomic.h | 77
+++++++++++++++++++++--------------------
1 files changed, 39 insertions(+), 38 deletions(-)
diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h index da1c77d..a715ac0 100644 --- a/arch/arm/include/asm/atomic.h +++ b/arch/arm/include/asm/atomic.h @@ -238,15 +238,15 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) #ifndef CONFIG_GENERIC_ATOMIC64 typedef struct { - u64 __aligned(8) counter; + long long counter; } atomic64_t; #define ATOMIC64_INIT(i) { (i) } #ifdef CONFIG_ARM_LPAE -static inline u64 atomic64_read(const atomic64_t *v) +static inline long long atomic64_read(const atomic64_t *v) { - u64 result; + long long result; __asm__ __volatile__("@ atomic64_read\n" " ldrd %0, %H0, [%1]" @@ -257,7 +257,7 @@ static inline u64 atomic64_read(const atomic64_t *v) return result; } -static inline void atomic64_set(atomic64_t *v, u64 i) +static inline void atomic64_set(atomic64_t *v, long long i) { __asm__ __volatile__("@ atomic64_set\n" " strd %2, %H2, [%1]" @@ -266,9 +266,9 @@ static inline void atomic64_set(atomic64_t *v, u64 i) ); } #else -static inline u64 atomic64_read(const atomic64_t *v) +static inline long long atomic64_read(const atomic64_t *v) { - u64 result; + long long result; __asm__ __volatile__("@ atomic64_read\n" " ldrexd %0, %H0, [%1]" @@ -279,9 +279,9 @@ static inline u64 atomic64_read(const atomic64_t *v) return result; } -static inline void atomic64_set(atomic64_t *v, u64 i) +static inline void atomic64_set(atomic64_t *v, long long i) { - u64 tmp; + long long tmp; __asm__ __volatile__("@ atomic64_set\n" "1: ldrexd %0, %H0, [%2]\n" @@ -294,9 +294,9 @@ static inline void atomic64_set(atomic64_t *v, u64 i) } #endif -static inline void atomic64_add(u64 i, atomic64_t *v) +static inline void atomic64_add(long long i, atomic64_t *v) { - u64 result; + long long result; unsigned long tmp; __asm__ __volatile__("@ atomic64_add\n" @@ -311,9 +311,9 @@ static inline void atomic64_add(u64 i, atomic64_t *v) : "cc"); } -static inline u64 atomic64_add_return(u64 i, atomic64_t *v) +static inline long long atomic64_add_return(long long i, atomic64_t *v) { - u64 result; + long long result; unsigned long tmp; smp_mb(); @@ -334,9 +334,9 @@ static inline u64 atomic64_add_return(u64 i, atomic64_t *v) return result; } -static inline void atomic64_sub(u64 i, atomic64_t *v) +static inline void atomic64_sub(long long i, atomic64_t *v) { - u64 result; + long long result; unsigned long tmp; __asm__ __volatile__("@ atomic64_sub\n" @@ -351,9 +351,9 @@ static inline void atomic64_sub(u64 i, atomic64_t *v) : "cc"); } -static inline u64 atomic64_sub_return(u64 i, atomic64_t *v) +static inline long long atomic64_sub_return(long long i, atomic64_t *v) { - u64 result; + long long result; unsigned long tmp; smp_mb(); @@ -374,9 +374,10 @@ static inline u64 atomic64_sub_return(u64 i, atomic64_t *v) return result; } -static inline u64 atomic64_cmpxchg(atomic64_t *ptr, u64 old, u64 new) +static inline long long atomic64_cmpxchg(atomic64_t *ptr, long long old, + long long new) { - u64 oldval; + long long oldval; unsigned long res; smp_mb(); @@ -398,9 +399,9 @@ static inline u64 atomic64_cmpxchg(atomic64_t *ptr, u64 old, u64 new) return oldval; } -static inline u64 atomic64_xchg(atomic64_t *ptr, u64 new) +static inline long long atomic64_xchg(atomic64_t *ptr, long long new) { - u64 result; + long long result; unsigned long tmp; smp_mb(); @@ -419,9 +420,9 @@ static inline u64 atomic64_xchg(atomic64_t *ptr, u64 new) return result; } -static inline u64 atomic64_dec_if_positive(atomic64_t *v) +static inline long long atomic64_dec_if_positive(atomic64_t *v) { - u64 result; + long long result; unsigned long tmp; smp_mb(); @@ -445,9 +446,9 @@ static inline u64 atomic64_dec_if_positive(atomic64_t *v) return result; } -static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u) +static inline int atomic64_add_unless(atomic64_t *v, long long a, long long u) { - u64 val; + long long val; unsigned long tmp; int ret = 1;
atomic* value is signed value, and atomic* functions need also process signed value (parameter value, and return value), so 32-bit arm need use 'long long' instead of 'u64'. After replacement, it will also fix a bug for atomic64_add_negative(): "u64 is never less than 0". The modifications are: in vim, use "1,% s/\<u64\>/long long/g" command. remove '__aligned(8)' which is useless for 64-bit. be sure of 80 column limitation after replacement. Signed-off-by: Chen Gang <gang.chen@asianux.com> --- arch/arm/include/asm/atomic.h | 49 +++++++++++++++++++++-------------------- 1 files changed, 25 insertions(+), 24 deletions(-)