Messages in this thread | | | Date | Mon, 12 Apr 2021 16:51:55 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] riscv: locks: introduce ticket-based spinlock implementation |
| |
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 :-)
| |