From: Peter Zijlstra <peterz@infradead.org>
To: "Christoph Müllner" <christophm30@gmail.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
Anup Patel <anup@brainfault.org>, Guo Ren <guoren@kernel.org>,
linux-riscv <linux-riscv@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Guo Ren <guoren@linux.alibaba.com>,
catalin.marinas@arm.com, will.deacon@arm.com,
Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH] riscv: locks: introduce ticket-based spinlock implementation
Date: Mon, 12 Apr 2021 16:51:55 +0200 [thread overview]
Message-ID: <YHRei8/8cIe6BNtE@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <CAHB2gtS9J09VaY9ZxDJYVo2fTgS-u6p7e89aLCnwOHnYEOJR=g@mail.gmail.com>
Please fix your mailer to properly flow text. Reflowed it for you.
On Mon, Apr 12, 2021 at 03:32:27PM +0200, Christoph Müllner wrote:
> This discussion came up again a few weeks ago because I've been
> stumbling over the test-and-set implementation and was wondering if
> nobody cared to improve that yet.
> Then I saw, that there have been a few attempts so far, but they did
> not land. So I brought this up in RVI's platform group meeting and
> the attendees showed big interest to get at least fairness. I assume
> Guo sent out his new patchset as a reaction to this call (1 or 2 days
> later).
>
> We have the same situation on OpenSBI, where we've agreed (with Anup)
> to go for a ticket lock implementation. A series for that can be
> found here (the implementation was tested in the kernel):
> http://lists.infradead.org/pipermail/opensbi/2021-April/000789.html
>
> In the mentioned RVI call, I've asked the question if ticket locks or
> MCF locks are preferred. And the feedback was to go for
> qspinlock/qrwlock. One good argument to do so would be, to not have to
> maintain an RV-specific implementation, but to use a well-tested
> in-kernel mechanism.
qrwlock does not depend on qspinlock; any fair spinlock implementation
works, including ticket.
> The feedback in the call is also aligned with the previous attempts to
> enable MCF-locks on RV. However, the kernel's implementation requires
> sub-word atomics. And here is, where we are. The discussion last week
> was about changing the generic kernel code to loosen its requirements
> (not accepted because of performance regressions on e.g. x86) and if
> RV actually can provide sub-word atomics in form of LL/SC loops (yes,
> that's possible).
So qspinlock is a complex and fickle beast. Yes it works on x86 and
arm64 (Will and Catalin put a _lot_ of effort into that), but IMO using
such a complex thing needs to be provably better than the trivial and
simple thing (tickets, test-and-set).
Part of that is fwd progress, if you don't have that, stay with
test-and-set. Will fixed a number of issues there, and -RT actually hit
at least one.
Debugging non-deterministic lock behaviour is not on the fun list.
Having something simple (ticket) to fall back to to prove it's your lock
going 'funneh' is very useful.
> Providing sub-word xchg() can be done within a couple of hours (some
> solutions are already on the list), but that was not enough from the
> initial patchset from Michael on (e.g. Christoph Hellwig asked back
> then for moving arch-independent parts into lib, which is a good idea
> given other archs do the same). So I expect this might require some
> more time until there is a solution, that's accepted by a broad range
> of maintainers.
Using a lib/ cmpxchg based xchg16 is daft. Per the very same arguments I
made elsewhere in the thread. cmpxchg() based loops have very difficult
fwd progress guarantees, esp. so on LL/SC architectures.
What I would do is create a common inline helper to compute that {addr,
mask, val} setup with a comment on how to use it.
(As is, we have at least 2 different ways of dealing with ENDIAN-ness)
> I've been working on a new MCF-lock series last week. It is working,
> but I did not publish it yet, because I wanted to go through the 130+
> emails on the riscv-linux list and check for overseen review comments
> and validate the patch authors.
> You can find the current state here:
> https://github.com/cmuellner/linux/pull/new/riscv-spinlocks
That's not public. But if that's not qspinlock, how are you justifying a
complex spinlock implementation? Does it perform better than ticket?
> So, if you insist on ticket locks, then let's coordinate who will do
> that and how it will be tested (RV32/RV64, qemu vs real hw).
Real hardware is all that counts :-)
next prev parent reply other threads:[~2021-04-12 14:52 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-24 10:14 [PATCH] riscv: locks: introduce ticket-based spinlock implementation guoren
2021-03-24 11:09 ` Peter Zijlstra
2021-03-24 12:10 ` Guo Ren
[not found] ` <CAM4kBBK7_s9U2vJbq68yC8WdDEfPQTaCOvn1xds3Si5B-Wpw+A@mail.gmail.com>
2021-03-24 12:23 ` Peter Zijlstra
2021-03-24 12:24 ` Guo Ren
2021-03-24 12:31 ` Peter Zijlstra
2021-03-24 12:28 ` Anup Patel
2021-03-24 12:37 ` Peter Zijlstra
2021-03-24 12:53 ` Anup Patel
2021-04-11 21:11 ` Palmer Dabbelt
2021-04-12 13:32 ` Christoph Müllner
2021-04-12 14:51 ` Peter Zijlstra [this message]
2021-04-12 21:21 ` Christoph Müllner
2021-04-12 17:33 ` Palmer Dabbelt
2021-04-12 21:54 ` Christoph Müllner
2021-04-13 8:03 ` Peter Zijlstra
2021-04-13 8:17 ` Peter Zijlstra
2021-04-14 2:26 ` Guo Ren
2021-04-14 7:08 ` Peter Zijlstra
2021-04-14 9:05 ` Peter Zijlstra
2021-04-14 10:16 ` [RFC][PATCH] locking: Generic ticket-lock Peter Zijlstra
2021-04-14 12:39 ` Guo Ren
2021-04-14 12:55 ` Peter Zijlstra
2021-04-14 13:08 ` Peter Zijlstra
2021-04-14 15:59 ` David Laight
2021-04-14 12:45 ` Peter Zijlstra
2021-04-14 21:02 ` Stafford Horne
2021-04-14 20:47 ` Stafford Horne
2021-04-15 8:09 ` Peter Zijlstra
2021-04-15 9:02 ` Catalin Marinas
2021-04-15 9:22 ` Will Deacon
2021-04-15 9:24 ` Peter Zijlstra
2021-04-19 17:35 ` Will Deacon
2021-04-23 6:44 ` Palmer Dabbelt
2021-04-13 9:22 ` [PATCH] riscv: locks: introduce ticket-based spinlock implementation Christoph Müllner
2021-04-13 9:30 ` Catalin Marinas
2021-04-13 9:55 ` Christoph Müllner
2021-04-14 0:23 ` Guo Ren
2021-04-14 9:17 ` Catalin Marinas
2021-04-13 9:35 ` Peter Zijlstra
2021-04-13 10:25 ` Christoph Müllner
2021-04-13 10:45 ` Catalin Marinas
2021-04-13 10:54 ` David Laight
2021-04-14 5:54 ` Guo Ren
2021-04-13 11:04 ` Christoph Müllner
2021-04-13 13:19 ` Guo Ren
2021-09-19 16:53 guoren
2021-09-25 14:47 ` Guo Ren
2021-10-21 13:13 ` Peter Zijlstra
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=YHRei8/8cIe6BNtE@hirez.programming.kicks-ass.net \
--to=peterz@infradead.org \
--cc=anup@brainfault.org \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=christophm30@gmail.com \
--cc=guoren@kernel.org \
--cc=guoren@linux.alibaba.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--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
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).