Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190429145159.GA29076@hc>
In-Reply-To: <20190429145159.GA29076@hc>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 1 May 2019 09:41:08 -0700
Message-ID: <CAHk-=wjPqcPYkiWKFc=R3+18DXqEhV+Nfbo=JWa32Xp8Nze67g@mail.gmail.com>
Subject: Re: [RFC] Disable lockref on arm64
To:     Jan Glauber <jglauber@marvell.com>
Cc:     "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "will.deacon@arm.com" <will.deacon@arm.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jayachandran Chandrasekharan Nair <jnair@marvell.com>
Content-Type: multipart/mixed; boundary="0000000000003645400587d6305c"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--0000000000003645400587d6305c
Content-Type: text/plain; charset="UTF-8"

On Mon, Apr 29, 2019 at 7:52 AM Jan Glauber <jglauber@marvell.com> wrote:
>
> It turned out the issue we have on ThunderX2 is the file open-close sequence
> with small read sizes. If the used files are opened read-only the
> lockref code (enabled by ARCH_USE_CMPXCHG_LOCKREF) is used.
>
> The lockref CMPXCHG_LOOP uses an unbound (as long as the associated
> spinlock isn't taken) while loop to change the lock count. This behaves
> badly under heavy contention

Ok, excuse me when I rant a bit.

Since you're at Marvell, maybe you can forward this rant to the proper
guilty parties?

Who was the absolute *GENIUS* who went

 Step 1: "Oh, we have a middling CPU that isn't world-class on its own"

 Step 2: "BUT! We can put a lot of them on a die, because that's 'easy'"

 Step 3: "But let's make sure the interconnect isn't all that special,
because that would negate the the whole 'easy' part, and really strong
interconnects are even harder than CPU's and use even more power, so
that wouldn't work"

 Step 4: "I wonder why this thing scales badly?"

Seriously. Why are you guys doing this? Has nobody ever looked at the
fundamental thought process above and gone "Hmm"?

If you try to compensate for a not-great core by putting twice the
number of them in a system, you need a cache system and interconnect
between them that is more than twice as good as the competition.

And honestly, from everything that I hear, you don't have it. The
whole chip is designed for "throughput when there is no contention".
Is it really a huge surprise that it then falls flat on its face when
there's something fancy going on?

So now you want to penalize everybody else in the ARM community
because you have a badly balanced system?

Ok, rant over.

The good news is that we can easily fix _this_ particular case by just
limiting the CMPXCHG_LOOP to a maximum number of retries, since the
loop is already designed to fail quickly if the spin lock value isn't
unlocked, and all the lockref code is already organized to fall back
to spinlocks.

So the attached three-liner patch may just work for you. Once _one_
thread hits the maximum retry case and goes into the spinlocked case,
everybody else will also fall back to spinlocks because they now see
that the lockref is contended. So the "retry" value probably isn't all
that important, but let's make it big enough that it probably never
happens on a well-balanced system.

But seriously: the whole "let's just do lots of CPU cores because it's
easy" needs to stop. It's fine if you have a network processor and
you're doing independent things, but it's not a GP processor approach.

Your hardware people need to improve on your CPU core (maybe the
server version of Cortex A76 is starting to approach being good
enough?) and your interconnect (seriously!) instead of just slapping
32 cores on a die and calling it a day.

                Linus "not a fan of the flock of chickens" Torvalds

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

IGxpYi9sb2NrcmVmLmMgfCAzICsrKwogMSBmaWxlIGNoYW5nZWQsIDMgaW5zZXJ0aW9ucygrKQoK
ZGlmZiAtLWdpdCBhL2xpYi9sb2NrcmVmLmMgYi9saWIvbG9ja3JlZi5jCmluZGV4IDNkNDY4YjUz
ZDRjOS4uYTY3NjJmOGY0NWM5IDEwMDY0NAotLS0gYS9saWIvbG9ja3JlZi5jCisrKyBiL2xpYi9s
b2NrcmVmLmMKQEAgLTksNiArOSw3IEBACiAgKiBmYWlsdXJlIGNhc2UuCiAgKi8KICNkZWZpbmUg
Q01QWENIR19MT09QKENPREUsIFNVQ0NFU1MpIGRvIHsJCQkJCVwKKwlpbnQgcmV0cnkgPSAxNTsJ
CS8qIEd1YXJhbnRlZWQgcmFuZG9tIG51bWJlciAqLwkJCVwKIAlzdHJ1Y3QgbG9ja3JlZiBvbGQ7
CQkJCQkJCVwKIAlCVUlMRF9CVUdfT04oc2l6ZW9mKG9sZCkgIT0gOCk7CQkJCQkJXAogCW9sZC5s
b2NrX2NvdW50ID0gUkVBRF9PTkNFKGxvY2tyZWYtPmxvY2tfY291bnQpOwkJCVwKQEAgLTIxLDYg
KzIyLDggQEAKIAkJaWYgKGxpa2VseShvbGQubG9ja19jb3VudCA9PSBwcmV2LmxvY2tfY291bnQp
KSB7CQlcCiAJCQlTVUNDRVNTOwkJCQkJCVwKIAkJfQkJCQkJCQkJXAorCQlpZiAoIS0tcmV0cnkp
CQkJCQkJCVwKKwkJCWJyZWFrOwkJCQkJCQlcCiAJCWNwdV9yZWxheCgpOwkJCQkJCQlcCiAJfQkJ
CQkJCQkJCVwKIH0gd2hpbGUgKDApCg==
--0000000000003645400587d6305c--