On Wed, 7 Apr 2010, Darren Hart wrote:Thomas Gleixner wrote:On Mon, 5 Apr 2010, Darren Hart wrote:Eeek. Having the owner in the loop is a good way to negate the benefits
Hmm. The order is weird. Why don't you do that simpler ?
Get the uval, the tid and the thread_info pointer outside of the
loop. Also task_pid_vnr(current) just needs a one time lookup.
of adaptive spinning by spinning forever (unlikely, but it could
certainly spin across multiple owners). Nice catch.
As for the uval.... I'm not sure what you mean. You get curval below
inside the loop, and there is no "uval" in the my version of the code.
Well, you need a first time lookup of owner and ownertid for which you
need the user space value (uval),
but thinking more about it it's not
even necessary. Just initialize ownertid to 0 so it will drop into the
lookup code when we did not acquire the futex in the cmpxchg.
As for the order, I had put the initial spin prior to the cmpxchg to
avoid doing too many cmpxchg's in a row as they are rather expensive.
However, since this is (now) the first opportunity to do try and acquire
the lock atomically after entering the futex syscall, I think you're
right, it should be the first thing in the loop.
change the loop to do:Single return point makes instrumentation so much easier. Unless folks
for (;;) {
curval = cmpxchg_futex_value_locked(uaddr, 0, curtid);
if (!curval)
return 1;
_really_ object, I'll leave it as is until we're closer to merging.
I don't care either way. That was just example code.
if ((curval & FUTEX_TID_MASK) != ownertid) {
ownertid = curval & FUTEX_TID_MASK;
owner = update_owner(ownertid);
}
Hrm... at this point the owner has changed... so we should break and go
to sleep, not update the owner and start spinning again. The
futex_spin_on_owner() will detect this and abort, so I'm not seeing the
purpose of the above if() block.
Why ? If the owner has changed and the new owner is running on another
cpu then why not spin further ?
+ hrtimer_init_sleeper(to, current);Why setup all this _before_ trying the adaptive spin ?
+ hrtimer_set_expires(&to->timer, *time);
+ }
I placed the retry: label above the adaptive spin loop. This way if we wake a
task and the lock is "stolen" it doesn't just go right back to sleep. This
should aid in fairness and also performance in less contended cases. I didn't
think it was worth a "if (first_time_through && time)" sort of block to be
able to setup the timer after the spin loop.
Hmm, ok.
Do we really need all this code ? A simple owner->on_cpu (owner needsAs Peter pointed out in IRC, p->oncpu isn't generic. I'll go trolling through
to be the task_struct then) would be sufficient to figure that out,
wouldn't it?
the mutex_spin_on_owner() discussions to see if I can determine why that's the
case.
AFAICT p->oncpu is the correct thing to use when CONFIG_SMP=y. All it
needs is a simple accessor function and you can keep all the futex
cruft in futex.c where it belongs.