Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
On Thu, 23 Aug 2007 21:29:41 +0200, Segher Boessenkool said:
> int f(atomic_t *x)
> {
> return atomic_read(x) + atomic_read(x);
> }
> ld r0,@r0
> slli r0,#1
> jmp lr
Looks like peephole optimization at work.
pgpB5VxTDd0mQ.pgp
Description: PGP signature
RE: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
> On Thu, 23 Aug 2007, Segher Boessenkool wrote: > The combining, which I've mentioned *multiple*times* is > > if (atomic_read(&x) <= 1) > > and dammit, if that doesn't result in a *single* instruction, the code > generation is pure and utter crap. > > It should result in > > cmpl $1,offset(reg) > > and nothing else. And there is no way in hell you are doing that with > "atomic_read()" being inline asm. Sorry, Linus, I don't agree. The whole point of 'volatile' is to say to the compiler, "DO NOT OPTIMIZE THIS. What you think is harmless may break things you do not and should not understand." The combination of the read and the compare into a single operation is a compiler optimization. While it would not be unreasonable to still allow this optimization (since I cannot think of any situations in which it could be harmful), it is just as reasonable not to. DS - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
On Thu, 23 Aug 2007, Segher Boessenkool wrote: > > This simply isn't true. The compiler *can* combine asm stuff: Please Segher, just shut up. The combining, which I've mentioned *multiple*times* is if (atomic_read(&x) <= 1) and dammit, if that doesn't result in a *single* instruction, the code generation is pure and utter crap. It should result in cmpl $1,offset(reg) and nothing else. And there is no way in hell you are doing that with "atomic_read()" being inline asm. So can you now just go away? Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
Segher Boessenkool <[EMAIL PROTECTED]> wrote:
> This simply isn't true. The compiler *can* combine asm stuff:
>
>
> typedef struct { int counter; } atomic_t;
>
> static inline __attribute__((pure)) int atomic_read(const atomic_t *v)
> {
> int x;
> asm("ld %0,@%1" : "=r"(x) : "r"(&v->counter), "m"(v->counter));
> return x;
> }
That's not precisely combining asm stuff. The compiler is ditching a whole
function because you've told it it can cache the result.
David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
The inline asm version has the EXACT SAME PROBLEM as the "volatile"
version has: it means that the compiler is unable to combine trivial
instructions.
This simply isn't true. The compiler *can* combine asm stuff:
typedef struct { int counter; } atomic_t;
static inline __attribute__((pure)) int atomic_read(const atomic_t *v)
{
int x;
asm("ld %0,@%1" : "=r"(x) : "r"(&v->counter), "m"(v->counter));
return x;
}
int f(atomic_t *x)
{
return atomic_read(x) + atomic_read(x);
}
int g(atomic_t *x)
{
return 0 * atomic_read(x);
}
generates
f:
ld r0,@r0
slli r0,#1
jmp lr
g:
ldi r0,#0
jmp lr
So why the *hell* you'd expect the asm version to be smaller, I can't
imagine.
I expect it to be smaller than the current code, which uses the
"big hammer" volatile version. We're talking about m32r here,
not x86. Even when using "volatile asm" it shouldn't generate
much bigger code.
Anyhow, I answered my own question: on m32r, you cannot use "m"
with ld/st insns, since autoincrement modes don't work there (the
assembler complains, at least). So you have to force the address
into a reg instead, and _that_ causes the size increase.
If the code needs barriers, the code should damn well add them.
Sure. I'm not suggesting otherwise.
Segher
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
On Wed, 22 Aug 2007, Segher Boessenkool wrote: > > > I also tried to rewrite it with inline asm code, but the kernel text size > > > bacame roughly 2kB larger. So, I prefer C version. > > Could you send me the source code diff between the two versions > you tested? 2kB difference is way too much, the asm version should > be smaller if anything. Segher, can you please drop out of this discussion? Your points are all wrong. The inline asm version has the EXACT SAME PROBLEM as the "volatile" version has: it means that the compiler is unable to combine trivial instructions. There's no way that is acceptable. So why the *hell* you'd expect the asm version to be smaller, I can't imagine. It's obvkously not going to be true. So here's a clue to everybody in this thread: THE CURRENT x86 BEHAVIOUR IS THE CORRECT ONE where we don't have any "volatile" and no inline asm and no barriers, and we let the compiler do the right thing. If the code needs barriers, the code should damn well add them. It's worked for the past 8 months, on the most common platform there is. Stop this *idiotic* thread already. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
I also tried to rewrite it with inline asm code, but the kernel text size bacame roughly 2kB larger. So, I prefer C version. Could you send me the source code diff between the two versions you tested? 2kB difference is way too much, the asm version should be smaller if anything. You're not the only arch maintainer who prefers doing it in C. If you trust your compiler (a big "if", apparently), inline asm only improves code generation if you have a whole bunch of general purpose registers for the optimizer to play with. No. The only real difference between the *(volatile *)& version and the volatile asm() version is that the volatile asm() version has defined semantics. There will be some code generation differences too, but they should be in the noise, unless GCC generates really bad code for either case. We know it sometimes does that for the *(volatile *)& thing; if the asm() version does something bad, I'd like to know about that too. Segher - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
Hirokazu Takata wrote: I think the parameter of atomic_read() should have "const" qualifier to avoid these warnings, and IMHO this modification might be worth applying on other archs. I agree. Here is an additional patch to revise the previous one for m32r. I'll incorporate this change if we get enough consensus to justify a re-re-re-submit. Since the patch is intended to be a functional no-op on m32r, I'm inclined to leave it alone at the moment. I also tried to rewrite it with inline asm code, but the kernel text size bacame roughly 2kB larger. So, I prefer C version. You're not the only arch maintainer who prefers doing it in C. If you trust your compiler (a big "if", apparently), inline asm only improves code generation if you have a whole bunch of general purpose registers for the optimizer to play with. -- Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
Hi, Chris,
From: Hirokazu Takata <[EMAIL PROTECTED]>
Date: Wed, 22 Aug 2007 10:56:54 +0900
> From: Chris Snook <[EMAIL PROTECTED]>
> Date: Mon, 13 Aug 2007 07:24:52 -0400
> > From: Chris Snook <[EMAIL PROTECTED]>
> >
> > Use volatile consistently in atomic.h on m32r.
> >
> > Signed-off-by: Chris Snook <[EMAIL PROTECTED]>
>
> Thanks,
>
> Acked-by: Hirokazu Takata <[EMAIL PROTECTED]>
Hmmm.. It seems my reply was overhasty.
Applying the above patch, I have many warning messages like this:
<-- snip -->
...
CC kernel/sched.o
In file included from
/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/netlink.h:139,
from
/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/genetlink.h:4,
from
/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/net/genetlink.h:4,
from
/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/taskstats_kern.h:12,
from
/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/delayacct.h:21,
from
/project/m32r-linux/kernel/work/linux-2.6_dev.git/kernel/sched.c:61:
/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/skbuff.h: In
function 'skb_shared':
/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/skbuff.h:521:
warning: passing argument 1 of 'atomic_read' discards qualifiers from pointer
target type
...
<-- snip -->
In this case, it is because stb_shared() is defined with a parameter with
"const" qualifier, in include/linux/skbuff.h.
static inline int skb_shared(const struct sk_buff *skb)
{
return atomic_read(&skb->users) != 1;
}
I think the parameter of atomic_read() should have "const"
qualifier to avoid these warnings, and IMHO this modification might be
worth applying on other archs.
Here is an additional patch to revise the previous one for m32r.
I also tried to rewrite it with inline asm code, but the kernel text size
bacame roughly 2kB larger. So, I prefer C version.
Thanks,
-- Takata
[PATCH] m32r: Add "const" qualifier to the parameter of atomic_read()
Update atomic_read() to avoid the following warning of gcc-4.1.x:
warning: passing argument 1 of 'atomic_read' discards qualifiers
from pointer target type
Signed-off-by: Hirokazu Takata <[EMAIL PROTECTED]>
Cc: Chris Snook <[EMAIL PROTECTED]>
---
include/asm-m32r/atomic.h |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/asm-m32r/atomic.h b/include/asm-m32r/atomic.h
index ba19689..9d46f86 100644
--- a/include/asm-m32r/atomic.h
+++ b/include/asm-m32r/atomic.h
@@ -32,7 +32,7 @@ typedef struct { int counter; } atomic_t;
*
* Atomically reads the value of @v.
*/
-static __inline__ int atomic_read(atomic_t *v)
+static __inline__ int atomic_read(const atomic_t *v)
{
return *(volatile int *)&v->counter;
}
--
1.5.2.4
--
Hirokazu Takata <[EMAIL PROTECTED]>
Linux/M32R Project: http://www.linux-m32r.org/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
From: Chris Snook <[EMAIL PROTECTED]> Date: Mon, 13 Aug 2007 07:24:52 -0400 > From: Chris Snook <[EMAIL PROTECTED]> > > Use volatile consistently in atomic.h on m32r. > > Signed-off-by: Chris Snook <[EMAIL PROTECTED]> Thanks, Acked-by: Hirokazu Takata <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r
From: Chris Snook <[EMAIL PROTECTED]>
Use volatile consistently in atomic.h on m32r.
Signed-off-by: Chris Snook <[EMAIL PROTECTED]>
--- linux-2.6.23-rc3-orig/include/asm-m32r/atomic.h 2007-07-08
19:32:17.0 -0400
+++ linux-2.6.23-rc3/include/asm-m32r/atomic.h 2007-08-13 05:42:09.0
-0400
@@ -22,7 +22,7 @@
* on us. We need to use _exactly_ the address the user gave us,
* not some alias that contains the same information.
*/
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
#define ATOMIC_INIT(i) { (i) }
@@ -32,7 +32,10 @@ typedef struct { volatile int counter; }
*
* Atomically reads the value of @v.
*/
-#define atomic_read(v) ((v)->counter)
+static __inline__ int atomic_read(atomic_t *v)
+{
+return *(volatile int *)&v->counter;
+}
/**
* atomic_set - set atomic variable
@@ -41,7 +44,10 @@ typedef struct { volatile int counter; }
*
* Atomically sets the value of @v to @i.
*/
-#define atomic_set(v,i)(((v)->counter) = (i))
+static __inline__ void atomic_set(atomic_t *v, int i)
+{
+*(volatile int *)&v->counter = i;
+}
/**
* atomic_add_return - add integer to atomic variable and return it
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
