From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Andrea Parri <andrea.parri@amarulasolutions.com>
Cc: Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
linux-kernel@vger.kernel.org,
Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH for-4.17 2/2] powerpc: Remove smp_mb() from arch_spin_is_locked()
Date: Tue, 27 Mar 2018 22:33:06 +1100 [thread overview]
Message-ID: <1522150386.7364.53.camel@kernel.crashing.org> (raw)
In-Reply-To: <20180327102521.GA7347@andrea>
On Tue, 2018-03-27 at 12:25 +0200, Andrea Parri wrote:
> > I would rather wait until it is properly documented. Debugging that IPC
> > problem took a *LOT* of time and energy, I wouldn't want these issues
> > to come and bite us again.
>
> I understand. And I'm grateful for this debugging as well as for the (IMO)
> excellent account of it you provided in 51d7d5205d338.
>
> Said this ;) I cannot except myself from saying that I would probably have
> resisted that solution (adding an smp_mb() in my arch_spin_is_locked), and
> instead "blamed"/suggested that caller to fix his memory ordering...
This is debatable. I tend go for the conservative approach assuming
most people using fairly high level APIs such as spin_is_locked() are
not fully cognisant of the subtleties of our memory model.
After all, it works on x86 ...
The fact that the load can leak into the internals of spin_lock()
implementation and re-order with the lock word load itself is quite
hard to fathom for somebody who doesn't have years of experience
dealing with that sort of issue.
So people will get it wrong.
So unless it's very performance sensitive, I'd rather have things like
spin_is_locked be conservative by default and provide simpler ordering
semantics.
Cheers,
Ben.
next prev parent reply other threads:[~2018-03-27 11:33 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-26 10:37 [PATCH for-4.17 2/2] powerpc: Remove smp_mb() from arch_spin_is_locked() Andrea Parri
2018-03-27 0:06 ` Benjamin Herrenschmidt
2018-03-27 10:25 ` Andrea Parri
2018-03-27 11:33 ` Benjamin Herrenschmidt [this message]
2018-03-27 13:13 ` Andrea Parri
2018-03-27 21:51 ` Benjamin Herrenschmidt
2018-03-28 9:17 ` Andrea Parri
2018-03-28 5:25 ` Michael Ellerman
2018-03-28 11:04 ` Peter Zijlstra
2018-03-28 11:08 ` Peter Zijlstra
2018-04-04 10:28 ` Michael Ellerman
2018-04-04 10:28 ` Michael Ellerman
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=1522150386.7364.53.camel@kernel.crashing.org \
--to=benh@kernel.crashing.org \
--cc=andrea.parri@amarulasolutions.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
--cc=torvalds@linux-foundation.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).