Re: [PATCH 9/24] make atomic_read() behave consistently on ia64

"Luck, Tony" [off-list ref] writes:

quoted

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 <redacted>

diff --git a/include/asm-ia64/atomic.h b/include/asm-ia64/atomic.h
index 1fc3b83..50c2b83 100644
--- a/include/asm-ia64/atomic.h
+++ b/include/asm-ia64/atomic.h

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

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
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."

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

Luck, Tony wrote:

quoted

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.

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.

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

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

On Fri, 10 Aug 2007, Luck, Tony wrote:

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

Linus Torvalds wrote:

On Fri, 10 Aug 2007, Luck, Tony wrote:

quoted

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

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

	-- Chris

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

Chris Snook writes:

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.

Paul Mackerras wrote:

Chris Snook writes:

quoted

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

On Mon, 13 Aug 2007, Chris Snook wrote:

Paul Mackerras wrote:

quoted

Chris Snook writes:

quoted

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.

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

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