make atomic_read() behave consistently on ia64

PATCH In review Chris Snook 6 participants 14 messages Aug 9, 2007 – Aug 13, 2007

Subsystems (4)

Networking Network drivers Generic include/asm header files Atomic infrastructure

lore

Share & tools

lore

Chris Snook

author

2007-08-09 13:51 UTC #0 lore raw

PATCH 9/24 make atomic_read() behave consistently on ia64

From: Chris Snook <[email protected]>

Purify volatile use for atomic[64]_t on ia64.

Signed-off-by Chris Snook <[email protected]>

include/asm-ia64/atomic.h +8 -8

@@ -17,18 +17,18 @@
 #include <asm/intrinsics.h>
 #include <asm/system.h>
 
-/*
- * On IA-64, counter must always be volatile to ensure that that the
- * memory accesses are ordered.
- */
-typedef struct { volatile __s32 counter; } atomic_t;
-typedef struct { volatile __s64 counter; } atomic64_t;
+typedef struct { __s32 counter; } atomic_t;
+typedef struct { __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 atomic64_read(v)	((v)->counter)
+/*
+ * Casting to volatile here minimizes the need for barriers,
+ * without having to declare the type itself as volatile.
+ */
+#define atomic_read(v)		(*(volatile __s32 *)&(v)->counter)
+#define atomic64_read(v)	(*(volatile __s64 *)&(v)->counter)
 
 #define atomic_set(v,i)		(((v)->counter) = (i))
 #define atomic64_set(v,i)	(((v)->counter) = (i))

Luck, Tony

2007-08-09 21:03 UTC #1 lore raw

include/asm-ia64/atomic.h line 34

Quoted 5 lines

include/asm-ia64/atomic.h +2 -0

+#define atomic_read(v)		(*(volatile __s32 *)&(v)->counter)
+#define atomic64_read(v)	(*(volatile __s64 *)&(v)->counter)
 
 #define atomic_set(v,i)		(((v)->counter) = (i))
 #define atomic64_set(v,i)	(((v)->counter) = (i))

Losing the volatile from the "set" variants definitely changes the code generated. Before the patch gcc would give us:

	st4.rel [r37]=r9

after

	st4 [r37]=r9

It is unclear whether anyone relies on (or even whether they should rely on) the release semantics that are provided by the current version of atomic.h. But making this change would require an audit of all the uses of atomic_set() to find an answer.

There is a more worrying difference in the generated code (this from the ancient and venerable gcc 3.4.6 that is on my build
machine). In rwsem_down_failed_common I see this change (after
disassembling vmlinux, I used sed to zap the low 32-bits of addresses to make the diff manageable ... that's why the addresses all end in xxxxxxxx):

712868,712873c712913,712921

< a0000001xxxxxxxx:                 adds r16=-1,r30
< a0000001xxxxxxxx:     [MII]       ld8.acq r33=[r32]
< a0000001xxxxxxxx:                 nop.i 0x0;;
< a0000001xxxxxxxx:                 add r42=r33,r16
< a0000001xxxxxxxx:     [MMI]       mov.m ar.ccv=r33;;
< a0000001xxxxxxxx:                 cmpxchg8.acq r34=[r32],r42,ar.ccv

---

Quoted 9 lines

a0000001xxxxxxxx:                 adds r16=-1,r31
a0000001xxxxxxxx:     [MMI]       ld4.acq r14=[r32];;
a0000001xxxxxxxx:                 nop.m 0x0
a0000001xxxxxxxx:                 sxt4 r34=r14
a0000001xxxxxxxx:     [MMI]       nop.m 0x0;;
a0000001xxxxxxxx:                 nop.m 0x0
a0000001xxxxxxxx:                 add r15=r34,r16
a0000001xxxxxxxx:     [MMI]       mov.m ar.ccv=r34;;
a0000001xxxxxxxx:                 cmpxchg8.acq r42=[r32],r15,ar.ccv

This code is probably from the rwsem_atomic_update(adjustment, sem) macro which is cpp'd to atomic64_add_return(). It looks really bad that the new code reads a 32-bit value and sign extends it rather than reading a 64-bit value (but I'm perplexed as to why this patch provoked this change in the generated code).

-Tony

Chris Snook

author

2007-08-10 19:51 UTC #2 lore raw

Quoted 45 lines · Tony

include/asm-ia64/atomic.h line 34
Quoted 5 lines
include/asm-ia64/atomic.h +2 -0
+#define atomic_read(v)		(*(volatile __s32 *)&(v)->counter)
+#define atomic64_read(v)	(*(volatile __s64 *)&(v)->counter)
 
 #define atomic_set(v,i)		(((v)->counter) = (i))
 #define atomic64_set(v,i)	(((v)->counter) = (i))
Losing the volatile from the "set" variants definitely changes the code generated. Before the patch gcc would give us:
	st4.rel [r37]=r9
after
	st4 [r37]=r9
It is unclear whether anyone relies on (or even whether they should rely on) the release semantics that are provided by the current version of atomic.h. But making this change would require an audit of all the uses of atomic_set() to find an answer.
There is a more worrying difference in the generated code (this from the ancient and venerable gcc 3.4.6 that is on my build
machine). In rwsem_down_failed_common I see this change (after
disassembling vmlinux, I used sed to zap the low 32-bits of addresses to make the diff manageable ... that's why the addresses all end in xxxxxxxx):
712868,712873c712913,712921
< a0000001xxxxxxxx:                 adds r16=-1,r30
< a0000001xxxxxxxx:     [MII]       ld8.acq r33=[r32]
< a0000001xxxxxxxx:                 nop.i 0x0;;
< a0000001xxxxxxxx:                 add r42=r33,r16
< a0000001xxxxxxxx:     [MMI]       mov.m ar.ccv=r33;;
< a0000001xxxxxxxx:                 cmpxchg8.acq r34=[r32],r42,ar.ccv
---
Quoted 9 lines
a0000001xxxxxxxx:                 adds r16=-1,r31
a0000001xxxxxxxx:     [MMI]       ld4.acq r14=[r32];;
a0000001xxxxxxxx:                 nop.m 0x0
a0000001xxxxxxxx:                 sxt4 r34=r14
a0000001xxxxxxxx:     [MMI]       nop.m 0x0;;
a0000001xxxxxxxx:                 nop.m 0x0
a0000001xxxxxxxx:                 add r15=r34,r16
a0000001xxxxxxxx:     [MMI]       mov.m ar.ccv=r34;;
a0000001xxxxxxxx:                 cmpxchg8.acq r42=[r32],r15,ar.ccv

Quoted 7 lines

This code is probably from the rwsem_atomic_update(adjustment, sem) macro which is cpp'd to atomic64_add_return(). It looks really bad that the new code reads a 32-bit value and sign extends it rather than reading a 64-bit value (but I'm perplexed as to why this patch provoked this change in the generated code).
-Tony

That's distressing. I'm about to resubmit with a volatile cast in atomic_set as well, since people expect that behavior and I've been shown a legitimate case where it could matter. Does the assembly look right with that cast in atomic_set() as well?

-- Chris

Luck, Tony

2007-08-10 21:19 UTC #3 lore raw

Quoted 4 lines

That's distressing. I'm about to resubmit with a volatile cast in atomic_set as well, since people expect that behavior and I've been shown a legitimate case where it could matter. Does the assembly look right with that cast in atomic_set() as well?

No. With the casts to volatile in atomic_set and atomic64_set I still see places where ld8 is changed to ld4 + sign-extend.

-Tony

Andreas Schwab

2007-08-10 21:43 UTC #4 lore raw

Quoted 7 lines · Tony

Quoted 4 lines
That's distressing. I'm about to resubmit with a volatile cast in atomic_set as well, since people expect that behavior and I've been shown a legitimate case where it could matter. Does the assembly look right with that cast in atomic_set() as well?
No. With the casts to volatile in atomic_set and atomic64_set I still see places where ld8 is changed to ld4 + sign-extend.

Use atomic64_read to read an atomic64_t.

Signed-off-by Andreas Schwab <[email protected]>

include/asm-ia64/atomic.h +2 -2

index 1fc3b83..50c2b83 100644
@@ -55,7 +55,7 @@ ia64_atomic64_add (__s64 i, atomic64_t *v)
 
 	do {
 		CMPXCHG_BUGCHECK(v);
-		old = atomic_read(v);
+		old = atomic64_read(v);
 		new = old + i;
 	} while (ia64_cmpxchg(acq, v, old, new, sizeof(atomic64_t)) != old);
 	return new;

@@ -83,7 +83,7 @@ ia64_atomic64_sub (__s64 i, atomic64_t *v)
 
 	do {
 		CMPXCHG_BUGCHECK(v);
-		old = atomic_read(v);
+		old = atomic64_read(v);
 		new = old - i;
 	} while (ia64_cmpxchg(acq, v, old, new, sizeof(atomic64_t)) != old);
 	return new;

Andreas.

Signature · 4 lines

Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Luck, Tony

2007-08-10 22:33 UTC #5 lore raw

Quoted 1 line

Use atomic64_read to read an atomic64_t.

Thanks Andreas!

Chris: This bug is why the 8-byte loads got changed to 4-byte + sign-extend by your change to atomic_read().

With this applied together with shuffling the volatile from the declaration to the usage (in both atomic_read() and atomic_set() the generated code *almost* reverts to the original.

There are some differences where ld4 have turned into ld8 though. Are these bugs in the use of atomic_add() and atomic_sub(). E.g. the first of these changes is in: ipc/msg.c:freeque() where we have:

	atomic_sub(msg->q_cbytes, &msg_bytes);

Now the type of msg->q_cbytes is "unsigned long" ... so it seems a poor idea to subtract such a large typed object from "msg_bytes" which is a mere slip of an atomic_t.

Or is there some other type-wrangling that needs to happen in include/asm-ia64/atomic.h? There are a total of nineteen of these ld4->ld8 transforms.

-Tony

Chris Snook

author

2007-08-10 22:43 UTC #6 lore raw

Quoted 6 lines · Tony

Quoted 1 line
Use atomic64_read to read an atomic64_t.
Thanks Andreas!
Chris: This bug is why the 8-byte loads got changed to 4-byte + sign-extend by your change to atomic_read().

I figured as much. Thanks for confirming this.

Quoted 10 lines

With this applied together with shuffling the volatile from the declaration to the usage (in both atomic_read() and atomic_set() the generated code *almost* reverts to the original.
There are some differences where ld4 have turned into ld8 though. Are these bugs in the use of atomic_add() and atomic_sub(). E.g. the first of these changes is in: ipc/msg.c:freeque() where we have:
	atomic_sub(msg->q_cbytes, &msg_bytes);

Quoted 7 lines

Now the type of msg->q_cbytes is "unsigned long" ... so it seems a poor idea to subtract such a large typed object from "msg_bytes" which is a mere slip of an atomic_t.
Or is there some other type-wrangling that needs to happen in include/asm-ia64/atomic.h? There are a total of nineteen of these ld4->ld8 transforms.

Possibly. Either that or we've uncovered some latent bugs. Maybe a combination of the two. Can you list those 19 changes so we can evaluate them? I'm told there were some *(volatile *) bugs fixed in gcc recently, so it's also possible your 3.4.6 is showing those. I can test that on a more recent gcc on ia64 if it's inconvenient for you to do so on your test box.

-- Chris

Luck, Tony

2007-08-10 22:59 UTC #7 lore raw

Quoted 2 lines

Possibly. Either that or we've uncovered some latent bugs. Maybe a combination of the two. Can you list those 19 changes so we can

evaluate them?

Here are the functions in which they occur in the object file. You may have to chase down some inlining to find the function that actually uses atomic_*().

freeque
do_msgrcv
sk_free
sock_wfree
sock_rfree
sock_kmalloc
sock_kfree_s
sock_setsockopt
skb_release_data
__sk_stream_mem_reclaim
sk_tream_mem_schedule
sk_stream_rfree
sk_attach_filter
ip_frag_destroy * 2
ip_frag_queue * 2
ip_frag_reasm * 2

-Tony

Linus Torvalds

2007-08-10 23:10 UTC #8 lore raw

Quoted 4 lines · Tony

Here are the functions in which they occur in the object file. You may have to chase down some inlining to find the function that actually uses atomic_*().

Could you just make the "atomic_read()" and "atomic_set()" functions be inline functions instead?

That way you get nice compiler warnings when you pass the wrong kind of object around. So

	static void atomic_set(atomic_t *p, int value)
	{
		*(volatile int *)&p->value = value;
	}

	static int atomic_read(atomic_t *p)
	{
		return *(volatile int *)&p->value;
	}

etc...

Linus

Chris Snook

author

2007-08-10 23:16 UTC #9 lore raw

Quoted 20 lines · Linus Torvalds

Quoted 3 lines · Tony
Here are the functions in which they occur in the object file. You may have to chase down some inlining to find the function that actually uses atomic_*().
Could you just make the "atomic_read()" and "atomic_set()" functions be inline functions instead?
That way you get nice compiler warnings when you pass the wrong kind of object around. So
	static void atomic_set(atomic_t *p, int value)
	{
		*(volatile int *)&p->value = value;
	}

	static int atomic_read(atomic_t *p)
	{
		return *(volatile int *)&p->value;
	}

Quoted 1 line

etc...

I'll do this for the whole patchset. Stay tuned for the resubmit.

-- Chris

Paul Mackerras

2007-08-11 00:36 UTC #10 lore raw

Quoted 1 line · Chris Snook

I'll do this for the whole patchset. Stay tuned for the resubmit.

Could you incorporate Segher's patch to turn atomic_{read,set} into asm on powerpc? Segher claims that using asm is really the only reliable way to ensure that gcc does what we want, and he seems to have a point.

Paul.

Chris Snook

author

2007-08-13 06:31 UTC #11 lore raw

Quoted 10 lines · Paul Mackerras

Quoted 1 line · Chris Snook
I'll do this for the whole patchset. Stay tuned for the resubmit.
Could you incorporate Segher's patch to turn atomic_{read,set} into asm on powerpc? Segher claims that using asm is really the only reliable way to ensure that gcc does what we want, and he seems to have a point.
Paul.

I haven't seen a patch yet. I'm going to resubmit with inline volatile-cast atomic[64]_[read|set] on all architectures as a reference point, and if anyone wants to go and implement some of them in assembly, that's between them and the relevant arch maintainers. I have no problem with (someone else) doing it in assembly. I just don't think it's necessary and won't let it hold up the effort to get consistent behavior on all architectures.

-- Chris

Geert Uytterhoeven

2007-08-13 07:40 UTC #12 lore raw

Quoted 9 lines · Chris Snook

Quoted 8 lines · Paul Mackerras
Quoted 1 line · Chris Snook
I'll do this for the whole patchset. Stay tuned for the resubmit.
Could you incorporate Segher's patch to turn atomic_{read,set} into asm on powerpc? Segher claims that using asm is really the only reliable way to ensure that gcc does what we want, and he seems to have a point.

Quoted 8 lines

Quoted 1 line
Paul.
I haven't seen a patch yet. I'm going to resubmit with inline volatile-cast atomic[64]_[read|set] on all architectures as a reference point, and if anyone wants to go and implement some of them in assembly, that's between them and the relevant arch maintainers. I have no problem with (someone else) doing it in assembly. I just don't think it's necessary and won't let it hold up the effort to get consistent behavior on all architectures.

http://lkml.org/lkml/2007/8/10/470

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Luck, Tony

2007-08-10 23:32 UTC #13 lore raw

In reply to Chris Snook

Quoted 3 lines

Here are the functions in which they occur in the object file. You may have to chase down some inlining to find the function that actually uses atomic_*().

Ignore this ... Andreas' patch was only two lines so I thought I'd "save time" by just hand-editing the source over on my build machine. I managed to goof that by editing the wrong function for one of the cases. :-(

New result. With Andreas's patch correctly applied, the generated vmlinux is identical with/without your patch.

-Tony