by Mathieu Desnoyers

[permalink] [raw]

Subject: Re: [PATCH 05/10] local_t : mips extension

Hi Ralf,

* Ralf Baechle ([email protected]) wrote:
> On Thu, Jan 25, 2007 at 11:16:12AM -0500, Mathieu Desnoyers wrote:
> > From: Mathieu Desnoyers <[email protected]>
> > To: [email protected]
> > Cc: Linus Torvalds <[email protected]>, Andrew Morton <[email protected]>,
> > Ingo Molnar <[email protected]>,
> > Greg Kroah-Hartman <[email protected]>,
> > Christoph Hellwig <[email protected]>, [email protected],
> > [email protected],
> > Douglas Niehaus <[email protected]>,
> > "Martin J. Bligh" <[email protected]>,
> > Thomas Gleixner <[email protected]>,
> > Paul Mackerras <[email protected]>,
> > Mathieu Desnoyers <[email protected]>
>
> How about copying the MIPS maintainer or linux-mips mailing list instead
> of a zillion people who probably don't care?
>

Although you are right about the correctness of sending the MIPS related
work to linux-mips, the other people I sent it to seemed interested in
my work. Thanks for the precision.

> > Subject: [PATCH 05/10] local_t : mips extension
> > Date: Thu, 25 Jan 2007 11:16:12 -0500
> >
> > local_t : mips extension
> >
> > Signed-off-by: Mathieu Desnoyers <[email protected]>
> > --- a/include/asm-mips/local.h
> > +++ b/include/asm-mips/local.h
> > @@ -1,60 +1,524 @@
> > -#ifndef _ASM_LOCAL_H
> > -#define _ASM_LOCAL_H
> > +#ifndef _ARCH_POWERPC_LOCAL_H
> > +#define _ARCH_POWERPC_LOCAL_H
>
> The subject claims this is a MIPS patch ;-)
>

"oops", will fix.

> > #include <linux/percpu.h>
> > #include <asm/atomic.h>
> >
> > -#ifdef CONFIG_32BIT
> > +typedef struct
> > +{
> > + local_long_t a;
> > +} local_t;
> >
> > -typedef atomic_t local_t;
> > +#define LOCAL_INIT(i) { local_LONG_INIT(i) }
> >
> > -#define LOCAL_INIT(i) ATOMIC_INIT(i)
> > -#define local_read(v) atomic_read(v)
> > -#define local_set(v,i) atomic_set(v,i)
> > +#define local_read(l) local_long_read(&(l)->a)
> > +#define local_set(l,i) local_long_set(&(l)->a, (i))
> >
> > -#define local_inc(v) atomic_inc(v)
> > -#define local_dec(v) atomic_dec(v)
> > -#define local_add(i, v) atomic_add(i, v)
> > -#define local_sub(i, v) atomic_sub(i, v)
> > +#define local_add(i,l) local_long_add((i),(&(l)->a))
> > +#define local_sub(i,l) local_long_sub((i),(&(l)->a))
> > +#define local_inc(l) local_long_inc(&(l)->a)
> > +#define local_dec(l) local_long_dec(&(l)->a)
> >
> > -#endif
> >
> > -#ifdef CONFIG_64BIT
> > +#ifndef CONFIG_64BITS
>
> There is no CONFIG_64BITS
>

Right, will fix (CONFIG_64BIT)

> > -typedef atomic64_t local_t;
> > +/*
> > + * Same as above, but return the result value
> > + */
> > +static __inline__ int local_add_return(int i, local_t * l)
> > +{
> > + unsigned long result;
> > +
> > + if (cpu_has_llsc && R10000_LLSC_WAR) {
>
> Missing #include <asm/war.h>.
>
ok, I will add it, but it worked because of :
#include <asm/atomic.h> -> #include <asm/war.h>

> > + unsigned long temp;
> > +
> > + __asm__ __volatile__(
> > + " .set mips3 \n"
> > + "1: ll %1, %2 # local_add_return \n"
> > + " addu %0, %1, %3 \n"
> > + " sc %0, %2 \n"
> > + " beqzl %0, 1b \n"
> > + " addu %0, %1, %3 \n"
> > + " .set mips0 \n"
> > + : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
> > + : "Ir" (i), "m" (&(l->a.counter))
> > + : "memory");
> > + } else if (cpu_has_llsc) {
> > + unsigned long temp;
> > +
> > + __asm__ __volatile__(
> > + " .set mips3 \n"
> > + "1: ll %1, %2 # local_add_return \n"
> > + " addu %0, %1, %3 \n"
> > + " sc %0, %2 \n"
> > + " beqz %0, 1b \n"
> > + " addu %0, %1, %3 \n"
> > + " .set mips0 \n"
> > + : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
> > + : "Ir" (i), "m" (&(l->a.counter))
> > + : "memory");
> > + } else {
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
> > + result = &(l->a.counter);
> > + local_irq_restore(flags);
> > + }
>
> Asigning some pointer value to an integer variable with no cast?
>

Yes, this whole file is wrong about this, I will change &(l->a.counter)
for l->a.counter (for variable assignment and for asm operands).

> > + result += i;
> > + &(l->a.counter) = result;
>
> Invalid lvalue in assignment.
>

Same problem as above.

> What I generally dislike about this patch is that several fairly large
> functions have been duplicated with only little change.
>

Yeah, I know. Until we find some way to share atomic operation code for
both operation on local and shared data, we have to duplicate this. We
could think about a header that would support multiple inclusion and
behave differently (different function prefix and LOCKing/memory
barriers) depending on defines set by the top level header.

Something like

asm/atomic.h
#define ATOMIC_SHARED
#include <asm/atomic-ops.h> /* shared */
#undef ATOMIC_SHARED
#include <asm/atomic-ops.h> /* local */

asm/atomic-ops.h
#ifdef ATOMIC_SHARED
#define ATOMIC_PREFIX atomic
#define ATOMIC_BARRIER() smp_mb()
#define ATOMIC_TYPE atomic_t
#define ATOMIC_VAR (v->counter)
#else
#define ATOMIC_PREFIX local
#define ATOMIC_BARRIER()
#define ATOMIC_TYPE local_t
#define ATOMIC_VAR (v->a.counter)
#endif

static __inline__ ATOMIC_PREFIX##_add_return(int i, ATOMIC_TYPE *v)
.....
#undef ATOMIC_PREFIX
#undef ATOMIC_BARRIER
#undef ATOMIC_TYPE
#undef ATOMIC_VAR

What do you think about this ?

Thanks!

Mathieu

Correction of MIPS variables and config options.

Signed-off-by: Mathieu Desnoyers <[email protected]>

--- a/include/asm-mips/local.h
+++ b/include/asm-mips/local.h
@@ -1,8 +1,9 @@
-#ifndef _ARCH_POWERPC_LOCAL_H
-#define _ARCH_POWERPC_LOCAL_H
+#ifndef _ARCH_MIPS_LOCAL_H
+#define _ARCH_MIPSRPC_LOCAL_H

#include <linux/percpu.h>
#include <asm/atomic.h>
+#include <asm/war.h>

typedef struct
{
@@ -20,7 +21,7 @@ typedef struct
#define local_dec(l) local_long_dec(&(l)->a)

-#ifndef CONFIG_64BITS
+#ifndef CONFIG_64BIT

/*
* Same as above, but return the result value
@@ -40,8 +41,8 @@ static __inline__ int local_add_return(int i, local_t * l)
" beqzl %0, 1b \n"
" addu %0, %1, %3 \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else if (cpu_has_llsc) {
unsigned long temp;
@@ -54,16 +55,16 @@ static __inline__ int local_add_return(int i, local_t * l)
" beqz %0, 1b \n"
" addu %0, %1, %3 \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else {
unsigned long flags;

local_irq_save(flags);
- result = &(l->a.counter);
+ result = l->a.counter;
result += i;
- &(l->a.counter) = result;
+ l->a.counter = result;
local_irq_restore(flags);
}

@@ -85,8 +86,8 @@ static __inline__ int local_sub_return(int i, local_t * l)
" beqzl %0, 1b \n"
" subu %0, %1, %3 \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else if (cpu_has_llsc) {
unsigned long temp;
@@ -99,16 +100,16 @@ static __inline__ int local_sub_return(int i, local_t * l)
" beqz %0, 1b \n"
" subu %0, %1, %3 \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else {
unsigned long flags;

local_irq_save(flags);
- result = &(l->a.counter);
+ result = l->a.counter;
result -= i;
- &(l->a.counter) = result;
+ l->a.counter = result;
local_irq_restore(flags);
}

@@ -142,8 +143,8 @@ static __inline__ int local_sub_if_positive(int i, local_t * l)
" .set reorder \n"
"1: \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else if (cpu_has_llsc) {
unsigned long temp;
@@ -160,17 +161,17 @@ static __inline__ int local_sub_if_positive(int i, local_t * l)
" .set reorder \n"
"1: \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else {
unsigned long flags;

local_irq_save(flags);
- result = &(l->a.counter);
+ result = l->a.counter;
result -= i;
if (result >= 0)
- &(l->a.counter) = result;
+ l->a.counter = result;
local_irq_restore(flags);
}

@@ -251,7 +252,7 @@ static __inline__ int local_sub_if_positive(int i, local_t * l)
*/
#define local_add_negative(i,l) (local_add_return(i, (l)) < 0)

-#else /* CONFIG_64BITS */
+#else /* CONFIG_64BIT */

/*
* Same as above, but return the result value
@@ -271,8 +272,8 @@ static __inline__ long local_add_return(long i, local_t * l)
" beqzl %0, 1b \n"
" addu %0, %1, %3 \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else if (cpu_has_llsc) {
unsigned long temp;
@@ -285,16 +286,16 @@ static __inline__ long local_add_return(long i, local_t * l)
" beqz %0, 1b \n"
" addu %0, %1, %3 \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else {
unsigned long flags;

local_irq_save(flags);
- result = &(l->a.counter);
+ result = l->a.counter;
result += i;
- &(l->a.counter) = result;
+ l->a.counter = result;
local_irq_restore(flags);
}

@@ -316,8 +317,8 @@ static __inline__ long local_sub_return(long i, local_t * l)
" beqzl %0, 1b \n"
" subu %0, %1, %3 \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else if (cpu_has_llsc) {
unsigned long temp;
@@ -330,16 +331,16 @@ static __inline__ long local_sub_return(long i, local_t * l)
" beqz %0, 1b \n"
" subu %0, %1, %3 \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else {
unsigned long flags;

local_irq_save(flags);
- result = &(l->a.counter);
+ result = l->a.counter;
result -= i;
- &(l->a.counter) = result;
+ l->a.counter = result;
local_irq_restore(flags);
}

@@ -373,8 +374,8 @@ static __inline__ long local_sub_if_positive(long i, local_t * l)
" .set reorder \n"
"1: \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else if (cpu_has_llsc) {
unsigned long temp;
@@ -391,17 +392,17 @@ static __inline__ long local_sub_if_positive(long i, local_t * l)
" .set reorder \n"
"1: \n"
" .set mips0 \n"
- : "=&r" (result), "=&r" (temp), "=m" (&(l->a.counter))
- : "Ir" (i), "m" (&(l->a.counter))
+ : "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
+ : "Ir" (i), "m" (l->a.counter)
: "memory");
} else {
unsigned long flags;

local_irq_save(flags);
- result = &(l->a.counter);
+ result = l->a.counter;
result -= i;
if (result >= 0)
- &(l->a.counter) = result;
+ l->a.counter = result;
local_irq_restore(flags);
}

@@ -483,7 +484,7 @@ static __inline__ long local_sub_if_positive(long i, local_t * l)
*/
#define local_add_negative(i,l) (local_add_return(i, (l)) < 0)

-#endif /* !CONFIG_64BITS */
+#endif /* !CONFIG_64BIT */

/* Use these for per-cpu local_t variables: on some archs they are
@@ -521,4 +522,4 @@ static __inline__ long local_sub_if_positive(long i, local_t * l)
#define __cpu_local_add(i, l) cpu_local_add((i), (l))
#define __cpu_local_sub(i, l) cpu_local_sub((i), (l))

-#endif /* _ARCH_POWERPC_LOCAL_H */
+#endif /* _ARCH_MIPS_LOCAL_H */

--
OpenPGP public key: http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68

2007-01-26 16:57:39

by Mathieu Desnoyers

[permalink] [raw]

Subject: Re: [PATCH 05/10] local_t : mips extension

* Mathieu Desnoyers ([email protected]) wrote:
> > What I generally dislike about this patch is that several fairly large
> > functions have been duplicated with only little change.
> >
>
> Yeah, I know. Until we find some way to share atomic operation code for
> both operation on local and shared data, we have to duplicate this. We
> could think about a header that would support multiple inclusion and
> behave differently (different function prefix and LOCKing/memory
> barriers) depending on defines set by the top level header.
>
> Something like
>
> asm/atomic.h
> #define ATOMIC_SHARED
> #include <asm/atomic-ops.h> /* shared */
> #undef ATOMIC_SHARED
> #include <asm/atomic-ops.h> /* local */
>
> asm/atomic-ops.h
> #ifdef ATOMIC_SHARED
> #define ATOMIC_PREFIX atomic
> #define ATOMIC_BARRIER() smp_mb()
> #define ATOMIC_TYPE atomic_t
> #define ATOMIC_VAR (v->counter)
> #else
> #define ATOMIC_PREFIX local
> #define ATOMIC_BARRIER()
> #define ATOMIC_TYPE local_t
> #define ATOMIC_VAR (v->a.counter)
> #endif
>
> static __inline__ ATOMIC_PREFIX##_add_return(int i, ATOMIC_TYPE *v)
> .....
> #undef ATOMIC_PREFIX
> #undef ATOMIC_BARRIER
> #undef ATOMIC_TYPE
> #undef ATOMIC_VAR
>

More prefisely, for this to work, we should change :
#define ATOMIC_PREFIX atomic
and
#define ATOMIC_PREFIX local
for

#define ATOMICF(fctname) atomic_##fctname
and
#define ATOMICF(fctname) local_##fctname

which would make de declaration look like :

static __inline__ ATOMICF(add_return)(int i, ATOMIC_TYPE *v)

Which starts to look a little odd to me, doesn't it ?

Regards,

Mathieu

--
OpenPGP public key: http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68