Another ldcw inline assembler patch

Another ldcw inline assembler patch

[John David Anglin]

Below is my take on how to write the ldcw asm.  I believe that Helge's
patch was essentially correct.

There are two reasons to expose the memory *a in the asm:

1) To prevent the compiler from discarding a preceeding write to *a, and
2) to prevent it from caching *a in a register over the asm.

The change has had a few days testing with a SMP build of 2.6.22.19
running on a rp3440.

Signed-off-by: Dave Anglin <dave.anglin@nrc-cnrc.gc.ca>

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

diff --git a/include/asm-parisc/system.h b/include/asm-parisc/system.h
index ee80c92..d91357b 100644
--- a/include/asm-parisc/system.h
+++ b/include/asm-parisc/system.h
@@ -168,8 +168,8 @@ static inline void set_eiem(unsigned long val)
 /* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.  */
 #define __ldcw(a) ({						\
 	unsigned __ret;						\
-	__asm__ __volatile__(__LDCW " 0(%1),%0"			\
-		: "=r" (__ret) : "r" (a));			\
+	__asm__ __volatile__(__LDCW " 0(%2),%0"			\
+		: "=r" (__ret), "+m" (*(a)) : "r" (a));		\
 	__ret;							\
 })
 

Re: Another ldcw inline assembler patch

[Matthew Wilcox]

On Sat, Jun 28, 2008 at 06:07:57PM -0400, John David Anglin wrote:
> There are two reasons to expose the memory *a in the asm:
> 
> 1) To prevent the compiler from discarding a preceeding write to *a, and
> 2) to prevent it from caching *a in a register over the asm.

Do either of those scenarios apply, given that every usage of this is
preceded by an asm clobbering memory?

I believe the correct thing to do is to take out the two mb()s in the
various spin_lock routines and make the __ldcw() macro itself clobber
memory.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Another ldcw inline assembler patch

[John David Anglin]

> On Sat, Jun 28, 2008 at 06:07:57PM -0400, John David Anglin wrote:
> > There are two reasons to expose the memory *a in the asm:
> > 
> > 1) To prevent the compiler from discarding a preceeding write to *a, and
> > 2) to prevent it from caching *a in a register over the asm.
> 
> Do either of those scenarios apply, given that every usage of this is
> preceded by an asm clobbering memory?

Probably not, I was just concerned about the correctness of the
__ldcw() macro itself.  I think the use of the macro should be
confined to small inline functions to try to limit the effect
of clobbering memory on GCC's optimization of loads and stores.

> I believe the correct thing to do is to take out the two mb()s in the
> various spin_lock routines and make the __ldcw() macro itself clobber
> memory.

This seems reasonable if you want __ldcw() to be a memory barrier.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Another ldcw inline assembler patch

[Grant Grundler]

On Sat, Jun 28, 2008 at 05:34:07PM -0600, Matthew Wilcox wrote:
> On Sat, Jun 28, 2008 at 06:07:57PM -0400, John David Anglin wrote:
> > There are two reasons to expose the memory *a in the asm:
> > 
> > 1) To prevent the compiler from discarding a preceeding write to *a, and
> > 2) to prevent it from caching *a in a register over the asm.
> 
> Do either of those scenarios apply, given that every usage of this is
> preceded by an asm clobbering memory?
> 
> I believe the correct thing to do is to take out the two mb()s in the
> various spin_lock routines and make the __ldcw() macro itself clobber
> memory.

I agree. Do you want jda to submit another patch or did you want kyle to
take jda's patch and apply a second one to remove the mb()'s?

thanks.
grant

> 
> -- 
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."
> --
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Another ldcw inline assembler patch

[James Bottomley]

On Sun, 2008-06-29 at 14:55 -0600, Grant Grundler wrote:
> On Sat, Jun 28, 2008 at 05:34:07PM -0600, Matthew Wilcox wrote:
> > On Sat, Jun 28, 2008 at 06:07:57PM -0400, John David Anglin wrote:
> > > There are two reasons to expose the memory *a in the asm:
> > > 
> > > 1) To prevent the compiler from discarding a preceeding write to *a, and
> > > 2) to prevent it from caching *a in a register over the asm.
> > 
> > Do either of those scenarios apply, given that every usage of this is
> > preceded by an asm clobbering memory?
> > 
> > I believe the correct thing to do is to take out the two mb()s in the
> > various spin_lock routines and make the __ldcw() macro itself clobber
> > memory.
> 
> I agree. Do you want jda to submit another patch or did you want kyle to
> take jda's patch and apply a second one to remove the mb()'s?

I really wouldn't do that.  Parisc is fairly unique in that we have C
spinlocks (most other architectures have asm coded ones).  The
requirement of the spinlock routines are that they be atomic memory
clobbers (as in the sequence of statements that does one shouldn't be
moved by the compiler)---which is why our C ones have mb before and
after.

if you make __ldcw() a memory clobber, that will pretty much cover the
atomic memory clobber requirements of __raw_spin_lock_flags() ... but
look at the rest of them ... it won't cover for them.  If we have to
have these mb()s in the rest, it makes sense to have them in
__raw_spin_lock_flags() as well, just to avoid problems if someone tries
to optimise again.  There probably should be a comment in this file to
that effect.

James