lkml.org 
[lkml]   [2021]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [GIT PULL] locking/urgent for v5.12
Date
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

\
 
 \ /
  Last update: 2021-04-25 19:08    [W:0.046 / U:0.816 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site