Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <YIU4005K55S20FU1@zn.tnic>
In-Reply-To: <YIU4005K55S20FU1@zn.tnic>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun, 25 Apr 2021 09:37:20 -0700
Message-ID: <CAHk-=whhCWi_aOvY9AfAz4fRMU53hO7n+w9hU6Wuk4RuHpK31Q@mail.gmail.com>
Subject: Re: [GIT PULL] locking/urgent for v5.12
To:     Borislav Petkov <bp@suse.de>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Ali Saidi <alisaidi@amazon.com>
Cc:     x86-ml <x86@kernel.org>, lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

[ Side note: this is cc'd to x86-ml, even though x86 is the _one_
architecture that was guaranteed to be not at all affected by the
actual locking bug, since a locked op is always ordered on x86. ]

On Sun, Apr 25, 2021 at 2:39 AM Borislav Petkov <bp@suse.de> wrote:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/locking_urgent_for_v5.12
>
> - Fix ordering in the queued writer lock's slowpath.

So I'm looking at that change, because the code is confusing.

Why did it add that "cnts" variable? We know it must have the value
_QW_WAITING, since that's what the atomic_cond_read_relaxed() waits
for.

I'm assuming it's because of the switch to try_cmpxchg by PeterZ?

HOWEVER.

That actually just makes the code even MORE unreadable.

That code was odd and hard to read even before, but now it's
positively confusing.

New confusion:
 - Why is the truly non-critical cmpxchg using "try_cmpxhg()", when
the _first_ cmpxchg - above the loop - is not?

Pre-existing confusion:
 - Why is the code using "atomic_add()" to set a bit?

Yeah, yeah, neither of these are *bugs*, but Christ is that code hard
to read. The "use add to set a bit" is valid because of the spinlock
serialization (ie only one add can ever happen), and the
cmpxchg-vs-try_cmpxchg confusion isn't buggy, it's just really really
confusing that that same function is using two different - but
equivalent - cmpxchg things on the same variable literally a couple of
lines apart.

I've pulled this, but can we please

 - make *both* of the cmpxchg's use "try_cmpxchg()" (and thus that
"cnts" variable)?

 - add a comment about _why_ it's doing "atomic_add()" instead of the
much more logical "atomic_or()", and about how the spinlock serializes
it

I'm assuming the "atomic_add()" is simply because many more
architectures have that as an actual intrinsic atomic. I understand.
But it's really really not obvious from the code.

                Linus