Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Geert Uytterhoeven]

Hi Peter,

On Tue, May 31, 2016 at 12:19 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> Implement FETCH-OP atomic primitives, these are very similar to the
> existing OP-RETURN primitives we already have, except they return the
> value of the atomic variable _before_ modification.
>
> This is especially useful for irreversible operations -- such as
> bitops (because it becomes impossible to reconstruct the state prior
> to modification).
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/m68k/include/asm/atomic.h |   53 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 4 deletions(-)
>
> --- a/arch/m68k/include/asm/atomic.h
> +++ b/arch/m68k/include/asm/atomic.h
> @@ -38,6 +38,13 @@ static inline void atomic_##op(int i, at
>
>  #ifdef CONFIG_RMW_INSNS
>
> +/*
> + * Am I reading these CAS loops right in that %2 is the old value and the first
> + * iteration uses an uninitialized value?
> + *
> + * Would it not make sense to add: tmp = atomic_read(v); to avoid this?
> + */
> +
>  #define ATOMIC_OP_RETURN(op, c_op, asm_op)                             \
>  static inline int atomic_##op##_return(int i, atomic_t *v)             \
>  {                                                                      \

Do we want the above comment in the code?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Peter Zijlstra]

On Thu, Jun 16, 2016 at 12:08:27PM +0200, Geert Uytterhoeven wrote:

> >  #ifdef CONFIG_RMW_INSNS
> >
> > +/*
> > + * Am I reading these CAS loops right in that %2 is the old value and the first
> > + * iteration uses an uninitialized value?
> > + *
> > + * Would it not make sense to add: tmp = atomic_read(v); to avoid this?
> > + */
> > +
> >  #define ATOMIC_OP_RETURN(op, c_op, asm_op)                             \
> >  static inline int atomic_##op##_return(int i, atomic_t *v)             \
> >  {                                                                      \
> 
> Do we want the above comment in the code?

I figured it would not hurt; is this indeed the case, do we want to fix
it? I can do a follow up patch clarifying the situation.

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Andreas Schwab]

Peter Zijlstra <peterz@infradead.org> writes:

> On Thu, Jun 16, 2016 at 12:08:27PM +0200, Geert Uytterhoeven wrote:
>
>> >  #ifdef CONFIG_RMW_INSNS
>> >
>> > +/*
>> > + * Am I reading these CAS loops right in that %2 is the old value and the first
>> > + * iteration uses an uninitialized value?
>> > + *
>> > + * Would it not make sense to add: tmp = atomic_read(v); to avoid this?
>> > + */
>> > +
>> >  #define ATOMIC_OP_RETURN(op, c_op, asm_op)                             \
>> >  static inline int atomic_##op##_return(int i, atomic_t *v)             \
>> >  {                                                                      \
>> 
>> Do we want the above comment in the code?
>
> I figured it would not hurt; is this indeed the case, do we want to fix
> it?

No, there is nothing to fix here.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Peter Zijlstra]

On Thu, Jun 16, 2016 at 02:43:29PM +0200, Andreas Schwab wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> >> > +/*
> >> > + * Am I reading these CAS loops right in that %2 is the old value and the first
> >> > + * iteration uses an uninitialized value?
> >> > + *
> >> > + * Would it not make sense to add: tmp = atomic_read(v); to avoid this?
> >> > + */

> No, there is nothing to fix here.

OK, care to elucidate? Clearly I need help reading this.

I'm more than happy to remove the comment, but I would like to better
understand.

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Andreas Schwab]

Peter Zijlstra <peterz@infradead.org> writes:

> On Thu, Jun 16, 2016 at 02:43:29PM +0200, Andreas Schwab wrote:
>> Peter Zijlstra <peterz@infradead.org> writes:
>> >> > +/*
>> >> > + * Am I reading these CAS loops right in that %2 is the old value and the first
>> >> > + * iteration uses an uninitialized value?
>> >> > + *
>> >> > + * Would it not make sense to add: tmp = atomic_read(v); to avoid this?
>> >> > + */
>
>> No, there is nothing to fix here.
>
> OK, care to elucidate? Clearly I need help reading this.

grep '2.*atomic_read'

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Peter Zijlstra]

On Thu, Jun 16, 2016 at 02:53:09PM +0200, Andreas Schwab wrote:
> Peter Zijlstra <peterz@infradead.org> writes:

> > OK, care to elucidate? Clearly I need help reading this.
> 
> grep '2.*atomic_read'

Much thanks to your detailed answer I found yet another obscure inline
asm syntax 'feature'.

So the "2" input operand actually sets the value of "=&d" (tmp), how
creative...

I would find:

#define ATOMIC_OP_RETURN(op, c_op, asm_op)                              \
static inline int atomic_##op##_return(int i, atomic_t *v)              \
{                                                                       \
        int t, tmp = atomic_read(v);                                    \
                                                                        \
        __asm__ __volatile__(                                           \
                        "1:     movel %2,%1\n"                          \
                        "       " #asm_op "l %3,%1\n"                   \
                        "       casl %2,%1,%0\n"                        \
                        "       jne 1b"                                 \
                        : "+m" (*v), "=&d" (t), "+d" (tmp)              \
                        : "g" (i));                                     \
        return t;                                                       \
}

Much more obvious.

But you're right, it seems to be sorted. I'll queue a patch removing
that comment.

Thanks!

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Andreas Schwab]

Peter Zijlstra <peterz@infradead.org> writes:

> So the "2" input operand actually sets the value of "=&d" (tmp), how
> creative...

That was the only way to do it when this was written.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Peter Zijlstra]

On Thu, Jun 16, 2016 at 04:37:36PM +0200, Andreas Schwab wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > So the "2" input operand actually sets the value of "=&d" (tmp), how
> > creative...
> 
> That was the only way to do it when this was written.

Fair enough; do you still support a compiler old enough to require this?
If not, do you want me to 'fix' this or just remove the comment?

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Andreas Schwab]

Peter Zijlstra <peterz@infradead.org> writes:

> If not, do you want me to 'fix' this or just remove the comment?

It's not broken, so nothing to fix.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Peter Zijlstra]

On Thu, Jun 16, 2016 at 05:04:24PM +0200, Andreas Schwab wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > If not, do you want me to 'fix' this or just remove the comment?
> 
> It's not broken, so nothing to fix.

Its non obvious code, that's usually plenty reason to change it.

Geert, you maintain this stuff, what say you? Is there still a good
reason (like supporting ancient compilers that don't do "+d" for
example) to keep the code as is?

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Andreas Schwab]

Peter Zijlstra <peterz@infradead.org> writes:

> On Thu, Jun 16, 2016 at 05:04:24PM +0200, Andreas Schwab wrote:
>> Peter Zijlstra <peterz@infradead.org> writes:
>> 
>> > If not, do you want me to 'fix' this or just remove the comment?
>> 
>> It's not broken, so nothing to fix.
>
> Its non obvious code, that's usually plenty reason to change it.

You appear to be the only one who has a problem with that documented
construct.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Geert Uytterhoeven]

Hi Peter,

On Thu, Jun 16, 2016 at 7:44 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Jun 16, 2016 at 05:04:24PM +0200, Andreas Schwab wrote:
>> Peter Zijlstra <peterz@infradead.org> writes:
>>
>> > If not, do you want me to 'fix' this or just remove the comment?
>>
>> It's not broken, so nothing to fix.
>
> Its non obvious code, that's usually plenty reason to change it.
>
> Geert, you maintain this stuff, what say you? Is there still a good
> reason (like supporting ancient compilers that don't do "+d" for
> example) to keep the code as is?

I don't know when support for "+d" was introduced.
But given people regularly use old compilers, I'm not inclined to change it,
unless there's a very good reason.

BTW, what's the failure mode if an old compiler not supporting "+d"
encounters it?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Peter Zijlstra]

Could either of you comment on the below patch?

All atomic functions that return a value should imply full memory
barrier semantics -- this very much includes a compiler barrier / memory
clobber.



---

 arch/m68k/include/asm/atomic.h  | 19 ++++++++++++-------
 arch/m68k/include/asm/cmpxchg.h |  9 ++++++---
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/arch/m68k/include/asm/atomic.h b/arch/m68k/include/asm/atomic.h
index 3e03de7ae33b..062a60417cb9 100644
--- a/arch/m68k/include/asm/atomic.h
+++ b/arch/m68k/include/asm/atomic.h
@@ -56,7 +56,8 @@ static inline int atomic_##op##_return(int i, atomic_t *v)		\
 			"	casl %2,%1,%0\n"			\
 			"	jne 1b"					\
 			: "+m" (*v), "=&d" (t), "=&d" (tmp)		\
-			: "g" (i), "2" (atomic_read(v)));		\
+			: "g" (i), "2" (atomic_read(v))			\
+			: "memory");					\
 	return t;							\
 }
 
@@ -71,7 +72,8 @@ static inline int atomic_fetch_##op(int i, atomic_t *v)			\
 			"	casl %2,%1,%0\n"			\
 			"	jne 1b"					\
 			: "+m" (*v), "=&d" (t), "=&d" (tmp)		\
-			: "g" (i), "2" (atomic_read(v)));		\
+			: "g" (i), "2" (atomic_read(v))			\
+			: "memory");					\
 	return tmp;							\
 }
 
@@ -141,7 +143,7 @@ static inline void atomic_dec(atomic_t *v)
 static inline int atomic_dec_and_test(atomic_t *v)
 {
 	char c;
-	__asm__ __volatile__("subql #1,%1; seq %0" : "=d" (c), "+m" (*v));
+	__asm__ __volatile__("subql #1,%1; seq %0" : "=d" (c), "+m" (*v) : : "memory");
 	return c != 0;
 }
 
@@ -151,14 +153,15 @@ static inline int atomic_dec_and_test_lt(atomic_t *v)
 	__asm__ __volatile__(
 		"subql #1,%1; slt %0"
 		: "=d" (c), "=m" (*v)
-		: "m" (*v));
+		: "m" (*v)
+		: "memory");
 	return c != 0;
 }
 
 static inline int atomic_inc_and_test(atomic_t *v)
 {
 	char c;
-	__asm__ __volatile__("addql #1,%1; seq %0" : "=d" (c), "+m" (*v));
+	__asm__ __volatile__("addql #1,%1; seq %0" : "=d" (c), "+m" (*v) : : "memory");
 	return c != 0;
 }
 
@@ -204,7 +207,8 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 	char c;
 	__asm__ __volatile__("subl %2,%1; seq %0"
 			     : "=d" (c), "+m" (*v)
-			     : ASM_DI (i));
+			     : ASM_DI (i)
+			     : "memory");
 	return c != 0;
 }
 
@@ -213,7 +217,8 @@ static inline int atomic_add_negative(int i, atomic_t *v)
 	char c;
 	__asm__ __volatile__("addl %2,%1; smi %0"
 			     : "=d" (c), "+m" (*v)
-			     : ASM_DI (i));
+			     : ASM_DI (i)
+			     : "memory");
 	return c != 0;
 }
 
diff --git a/arch/m68k/include/asm/cmpxchg.h b/arch/m68k/include/asm/cmpxchg.h
index 83b1df80f0ac..d8b3d2b48785 100644
--- a/arch/m68k/include/asm/cmpxchg.h
+++ b/arch/m68k/include/asm/cmpxchg.h
@@ -98,17 +98,20 @@ static inline unsigned long __cmpxchg(volatile void *p, unsigned long old,
 	case 1:
 		__asm__ __volatile__ ("casb %0,%2,%1"
 				      : "=d" (old), "=m" (*(char *)p)
-				      : "d" (new), "0" (old), "m" (*(char *)p));
+				      : "d" (new), "0" (old), "m" (*(char *)p)
+				      : "memory");
 		break;
 	case 2:
 		__asm__ __volatile__ ("casw %0,%2,%1"
 				      : "=d" (old), "=m" (*(short *)p)
-				      : "d" (new), "0" (old), "m" (*(short *)p));
+				      : "d" (new), "0" (old), "m" (*(short *)p)
+				      : "memory");
 		break;
 	case 4:
 		__asm__ __volatile__ ("casl %0,%2,%1"
 				      : "=d" (old), "=m" (*(int *)p)
-				      : "d" (new), "0" (old), "m" (*(int *)p));
+				      : "d" (new), "0" (old), "m" (*(int *)p)
+				      : "memory");
 		break;
 	default:
 		old = __invalid_cmpxchg_size(p, old, new, size);

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Andreas Schwab]

Peter Zijlstra <peterz@infradead.org> writes:

> Could either of you comment on the below patch?
>
> All atomic functions that return a value should imply full memory
> barrier semantics -- this very much includes a compiler barrier / memory
> clobber.

I wonder if it is possible to find a case where this makes a real
difference, ie. where the compiler erroneously reused a value due to
the missing barrier.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH -v2 14/33] locking,m68k: Implement atomic_fetch_{add,sub,and,or,xor}()

[Finn Thain]

On Mon, 20 Jun 2016, Andreas Schwab wrote:

> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > Could either of you comment on the below patch?
> >
> > All atomic functions that return a value should imply full memory 
> > barrier semantics -- this very much includes a compiler barrier / 
> > memory clobber.
> 
> I wonder if it is possible to find a case where this makes a real 
> difference, ie. where the compiler erroneously reused a value due to the 
> missing barrier.

What the compiler does erroneously is a compiler bug by definition. But I 
think that was not what you meant.

Perhaps you're asking whether gcc in particular does what you expect, 
despite ambiguous source code. But what about other tools like static 
analyzers?

Ambiguous code is likely to attract patches like this for as long as it 
remains ambiguous. That's a waste of everyone's time, if patches like this 
could be written and reviewed just once.

-- 

> 
> Andreas.
> 
>