Messages in this thread | | | From | Linus Torvalds <> | Date | Sun, 25 Apr 2021 09:39:49 -0700 | Subject | Re: [GIT PULL] locking/urgent for v5.12 |
| |
Oh, and replying to myself only because I spazzed out and pressed "send" before I had filled out the full participants line.
Sorry for the duplicate message quoted in full below.
Linus
On Sun, Apr 25, 2021 at 9:37 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > [ 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
| |