LKML Archive on lore.kernel.org
 help / color / Atom feed
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
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.

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-12 15:42 [PATCH 0/7] microblaze: Define SMP safe operations Michal Simek
2020-02-12 15:42 ` [PATCH 1/7] microblaze: timer: Don't use cpu timer setting Michal Simek
2020-02-12 15:42 ` [PATCH 2/7] microblaze: Make cpuinfo structure SMP aware Michal Simek
2020-02-12 20:42   ` Arnd Bergmann
2020-02-12 15:42 ` [PATCH 3/7] microblaze: Define SMP safe bit operations Michal Simek
2020-02-12 15:53   ` Peter Zijlstra
2020-02-13  8:42     ` Michal Simek
2020-02-13  9:01       ` Stefan Asserhall
2020-02-13  9:11         ` Peter Zijlstra
2020-02-13  9:24           ` Stefan Asserhall
2020-02-12 15:42 ` [PATCH 4/7] microblaze: Add SMP implementation of xchg and cmpxchg Michal Simek
2020-02-12 15:42 ` [PATCH 5/7] microblaze: Remove disabling IRQ while pte_update() run Michal Simek
2020-02-12 15:42 ` [PATCH 6/7] microblaze: Implement architecture spinlock Michal Simek
2020-02-12 15:47   ` Peter Zijlstra
2020-02-13  7:51     ` Michal Simek
2020-02-13  8:00       ` Peter Zijlstra
2020-02-12 15:42 ` [PATCH 7/7] microblaze: Do atomic operations by using exclusive ops Michal Simek
2020-02-12 15:55   ` Peter Zijlstra
2020-02-13  8:06     ` Michal Simek
2020-02-13  8:58       ` Peter Zijlstra [this message]
2020-02-13  9:16         ` Peter Zijlstra
2020-02-13 10:04           ` Will Deacon
2020-02-13 10:14             ` Stefan Asserhall
2020-02-13 10:20               ` Will Deacon
2020-02-13 10:15             ` Peter Zijlstra
2020-02-13 11:34         ` Boqun Feng
2020-02-13 11:38           ` Boqun Feng
2020-02-13 13:51             ` Andrea Parri
2020-02-13 14:01               ` Andrea Parri
2020-02-12 16:08 ` [PATCH 0/7] microblaze: Define SMP safe operations Peter Zijlstra
2020-02-12 16:38   ` Peter Zijlstra
2020-02-13  7:49   ` Michal Simek
2020-02-13  8:11     ` Peter Zijlstra
2020-02-13  8:12       ` Michal Simek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200213085849.GL14897@hirez.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=arnd@arndb.de \
    --cc=boqun.feng@gmail.com \
    --cc=git@xilinx.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=monstr@monstr.eu \
    --cc=paulmck@kernel.org \
    --cc=stefan.asserhall@xilinx.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git