Received-SPF: pass (google.com: domain of linux-kernel+bounces-212431-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240613001215.648829-1-mjguzik@gmail.com> <20240613001215.648829-2-mjguzik@gmail.com>
In-Reply-To: <20240613001215.648829-2-mjguzik@gmail.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 12 Jun 2024 18:23:18 -0700
Message-ID: <CAHk-=wgX9UZXWkrhnjcctM8UpDGQqWyt3r=KZunKV3+00cbF9A@mail.gmail.com>
Subject: Re: [PATCH 1/2] lockref: speculatively spin waiting for the lock to
 be released
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: brauner@kernel.org, viro@zeniv.linux.org.uk, jack@suse.cz, 
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Content-Type: multipart/mixed; boundary="00000000000007a12d061abb598e"

--00000000000007a12d061abb598e
Content-Type: text/plain; charset="UTF-8"

On Wed, 12 Jun 2024 at 17:12, Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> While I did not try to figure out who transiently took the lock (it was
> something outside of the benchmark), I devised a trivial reproducer
> which triggers the problem almost every time: merely issue "ls" of the
> directory containing the tested file (in this case: "ls /tmp").

So I have no problem with your patch 2/2 - moving the lockref data
structure away from everything else that can be shared read-only makes
a ton of sense independently of anything else.

Except you also randomly increased a retry count in there, which makes no sense.

But this patch 1/2 makes me go "Eww, hacky hacky".

We already *have* the retry loop, it's just that currently it only
covers the cmpxchg failures.

The natural thing to do is to just make the "wait for unlocked" be
part of the same loop.

In fact, I have this memory of trying this originally, and it not
mattering and just making the code uglier, but that may be me
confusing myself. It's a *loong* time ago.

With the attached patch, lockref_get() (to pick one random case) ends
up looking like this:

        mov    (%rdi),%rax
        mov    $0x64,%ecx
  loop:
        test   %eax,%eax
        jne    locked
        mov    %rax,%rdx
        sar    $0x20,%rdx
        add    $0x1,%edx
        shl    $0x20,%rdx
        lock cmpxchg %rdx,(%rdi)
        jne    fail
        // SUCCESS
        ret
  locked:
        pause
        mov    (%rdi),%rax
  fail:
        sub    $0x1,%ecx
        jne    loop

(with the rest being the "take lock and go slow" case).

It seems much better to me to have *one* retry loop that handles both
the causes of failures.

Entirely untested, I only looked at the generated code and it looked
reasonable. The patch may be entirely broken for some random reason I
didn't think of.

And in case you wonder, that 'lockref_locked()' macro I introduce is
purely to make the code more readable. Without it, that one
conditional line ends up being insanely long, the macro is there just
to break things up into slightly more manageable chunks.

Mind testing this approach instead?

                 Linus

--00000000000007a12d061abb598e
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_lxckssrl0>
X-Attachment-Id: f_lxckssrl0

IGxpYi9sb2NrcmVmLmMgfCAxNCArKysrKysrKysrLS0tLQogMSBmaWxlIGNoYW5nZWQsIDEwIGlu
c2VydGlvbnMoKyksIDQgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvbGliL2xvY2tyZWYuYyBi
L2xpYi9sb2NrcmVmLmMKaW5kZXggMmFmZTRjNWQ4OTE5Li43MGYzODYyMTkwMWIgMTAwNjQ0Ci0t
LSBhL2xpYi9sb2NrcmVmLmMKKysrIGIvbGliL2xvY2tyZWYuYwpAQCAtNCw2ICs0LDkgQEAKIAog
I2lmIFVTRV9DTVBYQ0hHX0xPQ0tSRUYKIAorI2RlZmluZSBsb2NrcmVmX2xvY2tlZChsKSBcCisJ
dW5saWtlbHkoIWFyY2hfc3Bpbl92YWx1ZV91bmxvY2tlZCgobCkubG9jay5ybG9jay5yYXdfbG9j
aykpCisKIC8qCiAgKiBOb3RlIHRoYXQgdGhlICJjbXB4Y2hnKCkiIHJlbG9hZHMgdGhlICJvbGQi
IHZhbHVlIGZvciB0aGUKICAqIGZhaWx1cmUgY2FzZS4KQEAgLTEzLDcgKzE2LDEyIEBACiAJc3Ry
dWN0IGxvY2tyZWYgb2xkOwkJCQkJCQlcCiAJQlVJTERfQlVHX09OKHNpemVvZihvbGQpICE9IDgp
OwkJCQkJCVwKIAlvbGQubG9ja19jb3VudCA9IFJFQURfT05DRShsb2NrcmVmLT5sb2NrX2NvdW50
KTsJCQlcCi0Jd2hpbGUgKGxpa2VseShhcmNoX3NwaW5fdmFsdWVfdW5sb2NrZWQob2xkLmxvY2su
cmxvY2sucmF3X2xvY2spKSkgeyAgCVwKKwlkbyB7CQkJCQkJCQkJXAorCQlpZiAobG9ja3JlZl9s
b2NrZWQob2xkKSkgewkJCQkJXAorCQkJY3B1X3JlbGF4KCk7CQkJCQkJXAorCQkJb2xkLmxvY2tf
Y291bnQgPSBSRUFEX09OQ0UobG9ja3JlZi0+bG9ja19jb3VudCk7CVwKKwkJCWNvbnRpbnVlOwkJ
CQkJCVwKKwkJfQkJCQkJCQkJXAogCQlzdHJ1Y3QgbG9ja3JlZiBuZXcgPSBvbGQ7CQkJCQlcCiAJ
CUNPREUJCQkJCQkJCVwKIAkJaWYgKGxpa2VseSh0cnlfY21weGNoZzY0X3JlbGF4ZWQoJmxvY2ty
ZWYtPmxvY2tfY291bnQsCQlcCkBAIC0yMSw5ICsyOSw3IEBACiAJCQkJCQkgbmV3LmxvY2tfY291
bnQpKSkgewkJXAogCQkJU1VDQ0VTUzsJCQkJCQlcCiAJCX0JCQkJCQkJCVwKLQkJaWYgKCEtLXJl
dHJ5KQkJCQkJCQlcCi0JCQlicmVhazsJCQkJCQkJXAotCX0JCQkJCQkJCQlcCisJfSB3aGlsZSAo
LS1yZXRyeSk7CQkJCQkJCVwKIH0gd2hpbGUgKDApCiAKICNlbHNlCg==
--00000000000007a12d061abb598e--