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 :-)
WARNING: multiple messages have this Message-ID (diff)
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 :-) _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2021-04-12 14:52 UTC|newest] Thread overview: 98+ 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 10:14 ` guoren 2021-03-24 11:09 ` Peter Zijlstra 2021-03-24 11:09 ` Peter Zijlstra 2021-03-24 12:10 ` Guo Ren 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:23 ` Peter Zijlstra 2021-03-24 12:24 ` Guo Ren 2021-03-24 12:24 ` Guo Ren 2021-03-24 12:31 ` Peter Zijlstra 2021-03-24 12:31 ` Peter Zijlstra 2021-03-24 12:28 ` Anup Patel 2021-03-24 12:28 ` Anup Patel 2021-03-24 12:37 ` Peter Zijlstra 2021-03-24 12:37 ` Peter Zijlstra 2021-03-24 12:53 ` Anup Patel 2021-03-24 12:53 ` Anup Patel 2021-04-11 21:11 ` Palmer Dabbelt 2021-04-11 21:11 ` Palmer Dabbelt 2021-04-12 13:32 ` Christoph Müllner 2021-04-12 13:32 ` Christoph Müllner 2021-04-12 14:51 ` Peter Zijlstra [this message] 2021-04-12 14:51 ` Peter Zijlstra 2021-04-12 21:21 ` Christoph Müllner 2021-04-12 21:21 ` Christoph Müllner 2021-04-12 17:33 ` Palmer Dabbelt 2021-04-12 17:33 ` Palmer Dabbelt 2021-04-12 21:54 ` Christoph Müllner 2021-04-12 21:54 ` Christoph Müllner 2021-04-13 8:03 ` Peter Zijlstra 2021-04-13 8:03 ` Peter Zijlstra 2021-04-13 8:17 ` Peter Zijlstra 2021-04-13 8:17 ` Peter Zijlstra 2021-04-14 2:26 ` Guo Ren 2021-04-14 2:26 ` Guo Ren 2021-04-14 7:08 ` Peter Zijlstra 2021-04-14 7:08 ` Peter Zijlstra 2021-04-14 9:05 ` 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 10:16 ` Peter Zijlstra 2021-04-14 12:39 ` Guo Ren 2021-04-14 12:39 ` Guo Ren 2021-04-14 12:55 ` Peter Zijlstra 2021-04-14 12:55 ` Peter Zijlstra 2021-04-14 13:08 ` Peter Zijlstra 2021-04-14 13:08 ` Peter Zijlstra 2021-04-14 15:59 ` David Laight 2021-04-14 15:59 ` David Laight 2021-04-14 12:45 ` Peter Zijlstra 2021-04-14 12:45 ` Peter Zijlstra 2021-04-14 21:02 ` Stafford Horne 2021-04-14 21:02 ` Stafford Horne 2021-04-14 20:47 ` Stafford Horne 2021-04-14 20:47 ` Stafford Horne 2021-04-15 8:09 ` Peter Zijlstra 2021-04-15 8:09 ` Peter Zijlstra 2021-04-15 9:02 ` Catalin Marinas 2021-04-15 9:02 ` Catalin Marinas 2021-04-15 9:22 ` Will Deacon 2021-04-15 9:22 ` Will Deacon 2021-04-15 9:24 ` Peter Zijlstra 2021-04-15 9:24 ` Peter Zijlstra 2021-04-19 17:35 ` Will Deacon 2021-04-19 17:35 ` Will Deacon 2021-04-23 6:44 ` Palmer Dabbelt 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:22 ` Christoph Müllner 2021-04-13 9:30 ` Catalin Marinas 2021-04-13 9:30 ` Catalin Marinas 2021-04-13 9:55 ` Christoph Müllner 2021-04-13 9:55 ` Christoph Müllner 2021-04-14 0:23 ` Guo Ren 2021-04-14 0:23 ` Guo Ren 2021-04-14 9:17 ` Catalin Marinas 2021-04-14 9:17 ` Catalin Marinas 2021-04-13 9:35 ` Peter Zijlstra 2021-04-13 9:35 ` Peter Zijlstra 2021-04-13 10:25 ` Christoph Müllner 2021-04-13 10:25 ` Christoph Müllner 2021-04-13 10:45 ` Catalin Marinas 2021-04-13 10:45 ` Catalin Marinas 2021-04-13 10:54 ` David Laight 2021-04-13 10:54 ` David Laight 2021-04-14 5:54 ` Guo Ren 2021-04-14 5:54 ` Guo Ren 2021-04-13 11:04 ` Christoph Müllner 2021-04-13 11:04 ` Christoph Müllner 2021-04-13 13:19 ` Guo Ren 2021-04-13 13:19 ` Guo Ren 2021-09-19 16:53 guoren 2021-09-19 16:53 ` guoren 2021-09-25 14:47 ` Guo Ren 2021-09-25 14:47 ` Guo Ren 2021-10-21 13:13 ` Peter Zijlstra 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.