Messages in this thread | | | From | Waiman Long <> | Subject | Re: [GIT PULL] locking/urgent for v5.12 | Date | Sun, 25 Apr 2021 13:06:52 -0400 |
| |
On 4/25/21 12:39 PM, Linus Torvalds wrote: > 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?
Yes, try_cmpxchg() requires a variable to hold the new value as well as a place to return the actual value before the cmpxchg(). It is just the way try_cmpxchg() works.
>> >> 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? At least for x86, try_cmpxchg() seems to produce a slight better assembly code than the regular cmpxchg(). I guess that may be one of the reason Peter changed it to use try_cmpxchg(). Another reason that I can think of is to make the code fit in one line instead of splitting it up into two lines like the original version from Ali. >> >> 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. As you have said, the spinlock serialization makes sure that only 1 writer is allowed to do that. I agree that using atomic_or() looks better in this case. Both of them are equivalent in this particular case. >> >> I've pulled this, but can we please >> >> - make *both* of the cmpxchg's use "try_cmpxchg()" (and thus that >> "cnts" variable)? Yes, we can certainly change the other cmpxchg() to try_cmpxchg(). >> >> - 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. >> I will post a patch to make the suggested change to qrwlock.c.
Cheers, Longman
| |