Messages in this thread | | | Date | Thu, 30 Apr 2015 14:56:08 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v16 13/14] pvqspinlock: Improve slowpath performance by avoiding cmpxchg |
| |
On 04/29/2015 02:27 PM, Linus Torvalds wrote: > On Wed, Apr 29, 2015 at 11:11 AM, Peter Zijlstra<peterz@infradead.org> wrote: >> On Fri, Apr 24, 2015 at 02:56:42PM -0400, Waiman Long wrote: >>> In the pv_scan_next() function, the slow cmpxchg atomic operation is >>> performed even if the other CPU is not even close to being halted. This >>> extra cmpxchg can harm slowpath performance. >>> >>> This patch introduces the new mayhalt flag to indicate if the other >>> spinning CPU is close to being halted or not. The current threshold >>> for x86 is 2k cpu_relax() calls. If this flag is not set, the other >>> spinning CPU will have at least 2k more cpu_relax() calls before >>> it can enter the halt state. This should give enough time for the >>> setting of the locked flag in struct mcs_spinlock to propagate to >>> that CPU without using atomic op. >> Yuck! I'm not at all sure you can make assumptions like that. And the >> worst part is, if it goes wrong the borkage is subtle and painful.\ > I have to agree with Peter. > > But it goes beyond this particular patch. Patterns like this: > > xchg(&pn->mayhalt, true); > > are just evil and disgusting. Even befoe this patch, that code had > > (void)xchg(&pn->state, vcpu_halted); > > which is *wrong* and should never be done. > > If you want it to be "set_mb()" (which sets a value and has a memory > barrier), then use set_mb(). Yes, it happens to use a "xchg()" to do > so, but dammit, it documents that whole "this is a memory barrier" in > the name. > Also, anybody who does this should damn well document why the memory > barrier is needed. The xchg(&pn->state, vcpu_halted) at least is > preceded by a comment about the barriers. The new mayhalt has no sane > comment in it, and the reason seems to be that no sane comment is > possible. The xchg() seems to be some black magic thing. > > Let's not introduce magic stuff in our locking primitives. At least > not undocumented magic that makes no sense. > > Linus
Thanks for the comments. I will withdraw this patch and use set_mb() in the code as suggested for better readability.
Cheers, Longman
| |