[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200213085849.GL14897@hirez.programming.kicks-ass.net>
Date: Thu, 13 Feb 2020 09:58:49 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Michal Simek <michal.simek@...inx.com>
Cc: linux-kernel@...r.kernel.org, monstr@...str.eu, git@...inx.com,
arnd@...db.de,
Stefan Asserhall load and store
<stefan.asserhall@...inx.com>, Boqun Feng <boqun.feng@...il.com>,
Will Deacon <will@...nel.org>, paulmck@...nel.org
Subject: Re: [PATCH 7/7] microblaze: Do atomic operations by using exclusive
ops
On Thu, Feb 13, 2020 at 09:06:24AM +0100, Michal Simek wrote:
> On 12. 02. 20 16:55, Peter Zijlstra wrote:
> > On Wed, Feb 12, 2020 at 04:42:29PM +0100, Michal Simek wrote:
> >> +static inline void atomic_set(atomic_t *v, int i)
> >> +{
> >> + int result, tmp;
> >> +
> >> + __asm__ __volatile__ (
> >> + /* load conditional address in %2 to %0 */
> >> + "1: lwx %0, %2, r0;\n"
> >> + /* attempt store */
> >> + " swx %3, %2, r0;\n"
> >> + /* checking msr carry flag */
> >> + " addic %1, r0, 0;\n"
> >> + /* store failed (MSR[C] set)? try again */
> >> + " bnei %1, 1b;\n"
> >> + /* Outputs: result value */
> >> + : "=&r" (result), "=&r" (tmp)
> >> + /* Inputs: counter address */
> >> + : "r" (&v->counter), "r" (i)
> >> + : "cc", "memory"
> >> + );
> >> +}
> >> +#define atomic_set atomic_set
> >
> > Uuuuhh.. *what* ?!?
> >
> > Are you telling me your LL/SC implementation is so bugger that
> > atomic_set() being a WRITE_ONCE() does not in fact work?
>
> Just keep in your mind that this code was written long time ago and
> there could be a lot of things/technique used at that time by IIRC
> powerpc and I hope that review process will fix these things and I
> really appreciation your comments.
I don't think I've ever seen Power do this, but I've not checked the git
history.
> Stefan is the right person to say if we really need to use exclusive
> loads/stores instructions or use what I see in include/linux/compiler.h.
>
> Please correct me if I am wrong.
> WRITE_ONCE is __write_once_size which is normal write in C which I
> expect will be converted in asm to non exclusive writes. And barrier is
> called only for cases above 8bytes.
>
> READ_ONCE is normal read follow by barrier all the time.
Right:
WRITE_ONCE() is something like:
*(volatile typeof(var)*)(&(var)) = val;
And should translate to just a regular store; the volatile just tells
the C compiler it should not do funny things with it.
READ_ONCE() is something like:
val = *(volatile typeof(var)*)(&(var));
And should translate to just a regular load; the volatile again tells
the compiler to not be funny about it.
No memory barriers what so ever, not even a compiler barrier as such.
The thing is, your bog standard LL/SC _SHOULD_ fail the SC if someone
else does a regular store to the same variable. See the example in
Documentation/atomic_t.txt.
That is, a competing SW/SWI should result in the interconnect responding
with something other than EXOKAY, the SWX should fail and MSR[C] <- 1.
> Also is there any testsuite I should run to verify all these atomics
> operations? That would really help but I haven't seen any tool (but also
> didn't try hard to find it out).
Will, Paul; can't this LKMM thing generate kernel modules to run? And do
we have a 'nice' collection of litmus tests that cover atomic_t ?
The one in atomic_t.txt should cover this one at least.
Powered by blists - more mailing lists