Re: [GIT PULL] locking/urgent for v5.12

[Waiman Long]

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@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxx> 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