lkml.org 
[lkml]   [2015]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH tip/locking/core v10 6/7] locking/pvqspinlock: Allow limited lock stealing
On 11/10/2015 11:03 AM, Peter Zijlstra wrote:
> On Mon, Nov 09, 2015 at 07:09:26PM -0500, Waiman Long wrote:
>> @@ -291,7 +292,7 @@ static __always_inline void __pv_wait_head(struct qspinlock *lock,
>> void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>> {
>> struct mcs_spinlock *prev, *next, *node;
>> - u32 new, old, tail;
>> + u32 new, old, tail, locked;
>> int idx;
>>
>> BUILD_BUG_ON(CONFIG_NR_CPUS>= (1U<< _Q_TAIL_CPU_BITS));
>> @@ -431,11 +432,25 @@ queue:
>> * sequentiality; this is because the set_locked() function below
>> * does not imply a full barrier.
>> *
>> + * The PV pv_wait_head_or_lock function, if active, will acquire
>> + * the lock and return a non-zero value. So we have to skip the
>> + * smp_load_acquire() call. As the next PV queue head hasn't been
>> + * designated yet, there is no way for the locked value to become
>> + * _Q_SLOW_VAL. So both the set_locked() and the
>> + * atomic_cmpxchg_relaxed() calls will be safe.
>> + *
>> + * If PV isn't active, 0 will be returned instead.
>> + *
>> */
>> - pv_wait_head(lock, node);
>> - while ((val = smp_load_acquire(&lock->val.counter))& _Q_LOCKED_PENDING_MASK)
>> + locked = val = pv_wait_head_or_lock(lock, node);
>> + if (locked)
>> + goto reset_tail_or_wait_next;
>> +
>> + while ((val = smp_load_acquire(&lock->val.counter))
>> + & _Q_LOCKED_PENDING_MASK)
>> cpu_relax();
>>
>> +reset_tail_or_wait_next:
>> /*
>> * claim the lock:
>> *
>> @@ -447,8 +462,12 @@ queue:
>> * to grab the lock.
>> */
>> for (;;) {
>> - if (val != tail) {
>> - set_locked(lock);
>> + /*
>> + * The lock value may or may not have the _Q_LOCKED_VAL bit set.
>> + */
>> + if ((val& _Q_TAIL_MASK) != tail) {
>> + if (!locked)
>> + set_locked(lock);
>> break;
>> }
>> /*
> How about this instead? If we've already got _Q_LOCKED_VAL set, issuing
> that store again isn't much of a problem, the cacheline is already hot
> and we own it and its a regular store not an atomic.
>
> @@ -432,10 +433,13 @@ void queued_spin_lock_slowpath(struct qs
> * does not imply a full barrier.
> *
> */
> - pv_wait_head(lock, node);
> + if ((val = pv_wait_head_or_lock(lock, node)))
> + goto locked;
> +
> while ((val = smp_load_acquire(&lock->val.counter))& _Q_LOCKED_PENDING_MASK)
> cpu_relax();
>
> +locked:
> /*
> * claim the lock:
> *
> @@ -447,7 +451,8 @@ void queued_spin_lock_slowpath(struct qs
> * to grab the lock.
> */
> for (;;) {
> - if (val != tail) {
> + /* In the PV case we might already have _Q_LOCKED_VAL set */
> + if ((val& _Q_TAIL_MASK) != tail) {
> set_locked(lock);
> break;
> }
>

That is certainly fine. I was doing that originally, but then change it
to add an additional if.

BTW, I have a process question. Should I just resend the patch 6 or
should I resend the whole series? I do have a couple of bugs in the
(_Q_PENDING_BITS != 8) part of the patch that I need to fix too.

Cheers,
Longman



\
 
 \ /
  Last update: 2015-11-10 21:21    [W:0.055 / U:0.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site