lkml.org 
[lkml]   [2010]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/6] futex: Add FUTEX_LOCK with optional adaptive spinning
    On Wed, 7 Apr 2010, Darren Hart wrote:
    > Thomas Gleixner wrote:
    > > On Mon, 5 Apr 2010, Darren Hart wrote:
    > > 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.
    >
    > Eeek. Having the owner in the loop is a good way to negate the benefits
    > 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:
    > >
    > > for (;;) {
    > > curval = cmpxchg_futex_value_locked(uaddr, 0, curtid);
    > > if (!curval)
    > > return 1;
    >
    > Single return point makes instrumentation so much easier. Unless folks
    > _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);
    > > > + hrtimer_set_expires(&to->timer, *time);
    > > > + }
    > >
    > > Why setup all this _before_ trying the adaptive spin ?
    >
    >
    > 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 needs
    > > to be the task_struct then) would be sufficient to figure that out,
    > > wouldn't it?
    >
    > As Peter pointed out in IRC, p->oncpu isn't generic. I'll go trolling through
    > 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.

    Thanks,

    tglx


    \
     
     \ /
      Last update: 2010-04-07 22:03    [W:0.025 / U:0.468 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site