LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Andrea Parri <andrea.parri@amarulasolutions.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>,
	Alan Stern <stern@rowland.harvard.edu>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	LKMM Maintainers -- Akira Yokosawa <akiyks@gmail.com>,
	Boqun Feng <boqun.feng@gmail.com>,
	Daniel Lustig <dlustig@nvidia.com>,
	David Howells <dhowells@redhat.com>,
	Jade Alglave <j.alglave@ucl.ac.uk>,
	Luc Maranget <luc.maranget@inria.fr>,
	Nicholas Piggin <npiggin@gmail.com>,
	Kernel development list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire
Date: Thu, 12 Jul 2018 13:52:49 +0200
Message-ID: <20180712115249.GA6201@andrea> (raw)
In-Reply-To: <20180712074040.GA4920@worktop.programming.kicks-ass.net>

On Thu, Jul 12, 2018 at 09:40:40AM +0200, Peter Zijlstra wrote:
> On Wed, Jul 11, 2018 at 02:34:21PM +0200, Andrea Parri wrote:
> > Simplicity is the eye of the beholder.  From my POV (LKMM maintainer), the
> > simplest solution would be to get rid of rfi-rel-acq and unlock-rf-lock-po
> > (or its analogous in v3) all together:
> 
> <snip>
> 
> > Among other things, this would immediately:
> > 
> >   1) Enable RISC-V to use their .aq/.rl annotations _without_ having to
> >      "worry" about tso or release/acquire fences; IOW, this will permit
> >      a partial revert of:
> > 
> >        0123f4d76ca6 ("riscv/spinlock: Strengthen implementations with fences")
> >        5ce6c1f3535f ("riscv/atomic: Strengthen implementations with fences")
> 
> But I feel this goes in the wrong direction. This weakens the effective
> memory model, where I feel we should strengthen it.
> 
> Currently PowerPC is the weakest here, and the above RISC-V changes
> (reverts) would make RISC-V weaker still.
> 
> Any any effective weakening makes me very uncomfortable -- who knows
> what will come apart this time. This memory ordering stuff causes
> horrible subtle bugs at best.

Indeed, what I was suggesting above is a weaking of the current model
(and I agree: I wouldn't say that bugs in this context are nice  ;-).

These changes would affect a specific area: (IMO,) the examples we've
been considering here aren't for the faint-hearted  ;-) and as Daniel
already suggested, everything would again be "nice and neat", if this
was all about locking/if every thread used lock-synchronization.


> 
> >   2) Resolve the above mentioned controversy (the inconsistency between
> >      - locking operations and atomic RMWs on one side, and their actual
> >      implementation in generic code on the other), thus enabling the use
> >      of LKMM _and_ its tools for the analysis/reviewing of the latter.
> 
> This is a good point; so lets see if there is something we can do to
> strengthen the model so it all works again.
> 
> And I think if we raise atomic*_acquire() to require TSO (but ideally
> raise it to RCsc) we're there.
> 
> The TSO archs have RCpc load-acquire and store-release, but fully
> ordered atomics. Most of the other archs have smp_mb() everything, with
> the exception of PPC, ARM64 and now RISC-V.
> 
> PPC has the RCpc TSO fence LWSYNC, ARM64 has the RCsc
> load-acquire/store-release. And RISC-V has a gazillion of options IIRC.
> 
> 
> So ideally atomic*_acquire() + smp_store_release() will be RCsc, and is
> with the notable exception of PPC, and ideally RISC-V would be RCsc
> here. But at the very least it should not be weaker than PPC.
> 
> By increasing atomic*_acquire() to TSO we also immediately get the
> proposed:
> 
>   P0()
>   {
> 	  WRITE_ONCE(X, 1);
> 	  spin_unlock(&s);
> 	  spin_lock(&s);
> 	  WRITE_ONCE(Y, 1);
>   }
> 
>   P1()
>   {
> 	  r1 = READ_ONCE(Y);
> 	  smp_rmb();
> 	  r2 = READ_ONCE(X);
>   }
> 
> behaviour under discussion; because the spin_lock() will imply the TSO
> ordering.

You mean: "when paired with a po-earlier release to the same memory
location", right?  I am afraid that neither arm64 nor riscv current
implementations would ensure "(r1 == 1 && r2 == 0) forbidden" if we
removed the po-earlier spin_unlock()...

AFAICT, the current implementation would work with that release: as
you remarked above, arm64 release->acquire is SC; riscv has an rw,w
fence in its spin_unlock() (hence an w,w fence between the stores),
or it could have a .tso fence ...

But again, these are stuble patterns, and my guess is that several/
most kernel developers really won't care about such guarantees (and
if some will do, they'll have the tools to figure out what they can
actually rely on ...)

OTOH (as I pointed out earlier) the strengthening we're configuring
will prevent some arch. (riscv being just the example of today!) to
go "full RCsc", and this will inevitably "complicate" both the LKMM
and the reviewing process of related changes (atomics, locking, ...;
c.f., this debate), apparently, just because you  ;-) want to "care"
about these guarantees.

Not yet convinced ...  :/

  Andrea


> 
> And note that this retains regular RCpc ACQUIRE for smp_load_acquire()
> and associated primitives -- as they have had since their introduction
> not too long ago.

  parent reply index

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-09 20:01 Alan Stern
2018-07-09 21:45 ` Paul E. McKenney
2018-07-10 13:57   ` Alan Stern
2018-07-10 16:25     ` Paul E. McKenney
     [not found]       ` <Pine.LNX.4.44L0.1807101416390.1449-100000@iolanthe.rowland.org>
2018-07-10 19:58         ` [PATCH v3] " Paul E. McKenney
2018-07-10 20:24           ` Alan Stern
2018-07-10 20:31             ` Paul E. McKenney
2018-07-11  9:43         ` Will Deacon
2018-07-11 15:42           ` Paul E. McKenney
2018-07-11 16:17             ` Andrea Parri
2018-07-11 18:03               ` Paul E. McKenney
2018-07-11 16:34           ` Peter Zijlstra
2018-07-11 18:10             ` Paul E. McKenney
2018-07-10  9:38 ` [PATCH v2] " Andrea Parri
2018-07-10 14:48   ` Alan Stern
2018-07-10 15:24     ` Andrea Parri
2018-07-10 15:34       ` Alan Stern
2018-07-10 23:14         ` Andrea Parri
2018-07-11  9:43   ` Will Deacon
2018-07-11 12:34     ` Andrea Parri
2018-07-11 12:54       ` Andrea Parri
2018-07-11 15:57       ` Will Deacon
2018-07-11 16:28         ` Andrea Parri
2018-07-11 17:00         ` Peter Zijlstra
2018-07-11 17:50           ` Daniel Lustig
2018-07-12  8:34             ` Andrea Parri
2018-07-12  9:29             ` Peter Zijlstra
2018-07-12  7:40       ` Peter Zijlstra
2018-07-12  9:34         ` Peter Zijlstra
2018-07-12  9:45           ` Will Deacon
2018-07-13  2:17             ` Daniel Lustig
2018-07-12 11:52         ` Andrea Parri [this message]
2018-07-12 12:01           ` Andrea Parri
2018-07-12 12:11             ` Peter Zijlstra
2018-07-12 13:48           ` Peter Zijlstra
2018-07-12 16:19             ` Paul E. McKenney
2018-07-12 17:04             ` Alan Stern
2018-07-12 17:14               ` Will Deacon
2018-07-12 17:28               ` Paul E. McKenney
2018-07-12 18:05                 ` Peter Zijlstra
2018-07-12 18:10                   ` Linus Torvalds
2018-07-12 19:52                     ` Andrea Parri
2018-07-12 20:24                       ` Andrea Parri
2018-07-13  2:05                     ` Daniel Lustig
2018-07-13  4:03                       ` Paul E. McKenney
2018-07-13  9:07                       ` Andrea Parri
2018-07-13  9:35                         ` Will Deacon
2018-07-13 17:16                           ` Linus Torvalds
2018-07-13 19:06                             ` Andrea Parri
2018-07-14  1:51                               ` Alan Stern
2018-07-14  2:58                                 ` Linus Torvalds
2018-07-16  2:31                                   ` Paul E. McKenney
2018-07-13 11:08                     ` Peter Zijlstra
2018-07-13 13:15                       ` Michael Ellerman
2018-07-13 16:42                         ` Peter Zijlstra
2018-07-13 19:56                           ` Andrea Parri
2018-07-16 14:40                           ` Michael Ellerman
2018-07-16 19:01                             ` Peter Zijlstra
2018-07-16 19:30                             ` Linus Torvalds
2018-07-17 14:45                               ` Michael Ellerman
2018-07-17 16:19                                 ` Linus Torvalds
2018-07-17 18:33                                   ` Paul E. McKenney
2018-07-17 18:42                                     ` Peter Zijlstra
2018-07-17 19:40                                       ` Paul E. McKenney
2018-07-17 19:47                                       ` Alan Stern
2018-07-17 18:44                                     ` Linus Torvalds
2018-07-17 18:49                                       ` Linus Torvalds
2018-07-17 19:42                                         ` Paul E. McKenney
2018-07-17 19:37                                       ` Alan Stern
2018-07-17 20:13                                         ` Linus Torvalds
2018-07-17 19:38                                       ` Paul E. McKenney
2018-07-17 19:40                                     ` Andrea Parri
2018-07-17 19:52                                       ` Paul E. McKenney
2018-07-18 12:31                                   ` Michael Ellerman
2018-07-18 13:16                             ` Michael Ellerman
2018-07-12 17:52               ` Andrea Parri
2018-07-12 20:43                 ` Alan Stern
2018-07-12 21:13                   ` Andrea Parri
2018-07-12 21:23                     ` Andrea Parri
2018-07-12 18:33               ` Peter Zijlstra
2018-07-12 17:45             ` Andrea Parri
2018-07-10 16:56 ` Daniel Lustig
     [not found]   ` <Pine.LNX.4.44L0.1807101315140.1449-100000@iolanthe.rowland.org>
2018-07-10 23:31     ` Andrea Parri
2018-07-11 14:19       ` Alan Stern

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=20180712115249.GA6201@andrea \
    --to=andrea.parri@amarulasolutions.com \
    --cc=akiyks@gmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=dhowells@redhat.com \
    --cc=dlustig@nvidia.com \
    --cc=j.alglave@ucl.ac.uk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luc.maranget@inria.fr \
    --cc=npiggin@gmail.com \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=stern@rowland.harvard.edu \
    --cc=will.deacon@arm.com \
    /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
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.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