Re: [PATCH 7/7] microblaze: Do atomic operations by using exclusive ops

From: Peter Zijlstra <peterz@infradead.org>
To: Michal Simek <michal.simek@xilinx.com>
Cc: linux-kernel@vger.kernel.org, monstr@monstr.eu, git@xilinx.com,
	arnd@arndb.de,
	Stefan Asserhall load and store  <stefan.asserhall@xilinx.com>,
	Boqun Feng <boqun.feng@gmail.com>, Will Deacon <will@kernel.org>,
	paulmck@kernel.org
Subject: Re: [PATCH 7/7] microblaze: Do atomic operations by using exclusive ops
Date: Thu, 13 Feb 2020 09:58:49 +0100	[thread overview]
Message-ID: <20200213085849.GL14897@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <4b46b33e-14ad-7097-f0db-2915ac772f15@xilinx.com>

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.