lkml.org 
[lkml]   [2014]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v9 03/19] qspinlock: Add pending bit
On 04/17/2014 11:42 AM, Peter Zijlstra wrote:
> On Thu, Apr 17, 2014 at 11:03:55AM -0400, Waiman Long wrote:
>> +/**
>> + * trylock_pending - try to acquire queue spinlock using the pending bit
>> + * @lock : Pointer to queue spinlock structure
>> + * @pval : Pointer to value of the queue spinlock 32-bit word
>> + * Return: 1 if lock acquired, 0 otherwise
>> + */
>> +static inline int trylock_pending(struct qspinlock *lock, u32 *pval)
>> +{
>> + u32 old, new, val = *pval;
> I'm not thrilled about you breaking this into a separate function; the
> compiler will put it right back and now you get to have that ugly
> pointer stuff.
>
> It also makes the function control flow not match the state diagram
> anymore.

I separate it out primarily to break the pending bit logic away from the
core MCS queuing logic to make each of them easier to understand as they
are kind of independent. I fully understand that the compiler will put
them back together. As I pile on more code, the slowpath function will
grow bigger making it harder to comprehend and find out where are the
boundary between them.

I will take a look at the state diagram to see what adjustment will be
needed.

>> +
>> + /*
>> + * trylock || pending
>> + *
>> + * 0,0,0 -> 0,0,1 ; trylock
>> + * 0,0,1 -> 0,1,1 ; pending
>> + */
>> + for (;;) {
>> + /*
>> + * If we observe any contention; queue.
>> + */
>> + if (val& ~_Q_LOCKED_MASK)
>> + return 0;
>> +
>> + new = _Q_LOCKED_VAL;
>> + if (val == new)
>> + new |= _Q_PENDING_VAL;
>> +
>> + old = atomic_cmpxchg(&lock->val, val, new);
>> + if (old == val)
>> + break;
>> +
>> + *pval = val = old;
>> + }
>> +
>> + /*
>> + * we won the trylock
>> + */
>> + if (new == _Q_LOCKED_VAL)
>> + return 1;
>> +
>> + /*
>> + * we're pending, wait for the owner to go away.
>> + *
>> + * *,1,1 -> *,1,0
>> + */
>> + while ((val = atomic_read(&lock->val))& _Q_LOCKED_MASK)
>> + arch_mutex_cpu_relax();
> That was a cpu_relax().

Yes, but arch_mutex_cpu_relax() is the same as cpu_relax() for x86.

-Longman


\
 
 \ /
  Last update: 2014-04-18 00:01    [W:0.195 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site