[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 3/8] qspinlock, x86: Add x86 specific optimization for 2 contending tasks

On 02/28/2014 04:29 AM, Peter Zijlstra wrote:
On Thu, Feb 27, 2014 at 03:42:19PM -0500, Waiman Long wrote:
+       old = xchg(&qlock->lock_wait, _QSPINLOCK_WAITING|_QSPINLOCK_LOCKED);
+       if (old == 0) {
+               /*
+                * Got the lock, can clear the waiting bit now
+                */
+               smp_u8_store_release(&qlock->wait, 0);
So we just did an atomic op, and now you're trying to optimize this
write. Why do you need a whole byte for that?

Surely a cmpxchg loop with the right atomic op can't be _that_ much
slower? Its far more readable and likely avoids that steal fail below as
At low contention level, atomic operations that requires a lock prefix are
the major contributor to the total execution times. I saw estimate online
that the time to execute a lock prefix instruction can easily be 50X longer
than a regular instruction that can be pipelined. That is why I try to do it
with as few lock prefix instructions as possible. If I have to do an atomic
cmpxchg, it probably won't be faster than the regular qspinlock slowpath.
At low contention the cmpxchg won't have to be retried (much) so using
it won't be a problem and you get to have arbitrary atomic ops.

Given that speed at low contention level which is the common case is
important to get this patch accepted, I have to do what I can to make it run
as far as possible for this 2 contending task case.
What I'm saying is that you can do the whole thing with a single
cmpxchg. No extra ops needed. And at that point you don't need a whole
byte, you can use a single bit.

that removes the whole NR_CPUS dependent logic.

After modifying it to do a deterministic cmpxchg, the test run time of 2 contending tasks jumps up from 600ms (best case) to about 1700ms which was worse than the original qspinlock's 1300-1500ms. It is the opportunistic nature of the xchg() code that can potentially combine multiple steps in the deterministic atomic sequence which can saves time. Without that, I would rather prefer going back to the basic qspinlock queuing sequence for 2 contending tasks.

Please take a look at the performance data in my patch 3 to see if the slowdown at 2 and 3 contending tasks are acceptable or not.

The reason why I need a whole byte for the lock bit is because of the simple unlock code of assigning 0 to the lock byte by the lock holder. Utilizing other bits in the low byte for other purpose will complicate the unlock path and slow down the no-contention case.

+               /*
+                * Someone has steal the lock, so wait again
+                */
+               goto try_again;
That's just a fail.. steals should not ever be allowed. It's a fair lock
after all.
The code is unfair, but this unfairness help it to run faster than ticket
spinlock in this particular case. And the regular qspinlock slowpath is
fair. A little bit of unfairness in this particular case helps its speed.
*groan*, no, unfairness not cool. ticket lock is absolutely fair; we
should preserve this.

We can preserve that by removing patch 3.

BTW; can you share your benchmark thingy?

I have attached the test program that I used to generate the timing data for patch 3.


Attachment: locktest.tar.gz
Description: GNU Zip compressed data

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.