Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1192595 > unrolled thread

Re: [PATCH v3 2/7] locking/pvqspinlock: Add pending bit support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/7] locking/pvqspinlock: Add pending bit support Davidlohr Bueso <dave@stgolabs.net> - 2015-07-27 03:00 +0200
    Re: [PATCH v3 2/7] locking/pvqspinlock: Add pending bit support Waiman Long <waiman.long@hp.com> - 2015-07-29 23:00 +0200

#1192595 — Re: [PATCH v3 2/7] locking/pvqspinlock: Add pending bit support

On Wed, 2015-07-22 at 16:12 -0400, Waiman Long wrote:
> Like the native qspinlock, using the pending bit when it is lightly
> loaded to acquire the lock is faster than going through the PV queuing
> process which is even slower than the native queuing process. It also
> avoids loading two additional cachelines (the MCS and PV nodes).
> 
> This patch adds the pending bit support for PV qspinlock. The pending
> bit code has a smaller spin threshold (1<<10). It will default back
> to the queuing method if it cannot acquired the lock within a certain
> time limit.

Can we infer that this new spin threshold is the metric to detect these
"light loads"? If so, I cannot help but wonder if there is some more
straightforward/ad-hoc way of detecting this, ie some pv_<> function.
That would also save a lot of time as it would not be time based.
Although it might be a more costly call altogether, I dunno.

Some comments about this 'loop' threshold.

> +static int pv_pending_lock(struct qspinlock *lock, u32 val)
> +{
> +	int loop = PENDING_SPIN_THRESHOLD;
> +	u32 new, old;
> +
> +	/*
> +	 * wait for in-progress pending->locked hand-overs
> +	 */
> +	if (val == _Q_PENDING_VAL) {
> +		while (((val = atomic_read(&lock->val)) == _Q_PENDING_VAL) &&
> +			loop--)
> +			cpu_relax();
> +	}
> +
> +	/*
> +	 * trylock || pending
> +	 */
> +	for (;;) {
> +		if (val & ~_Q_LOCKED_MASK)
> +			goto queue;
> +		new = _Q_LOCKED_VAL;
> +		if (val == new)
> +			new |= _Q_PENDING_VAL;
> +		old = atomic_cmpxchg(&lock->val, val, new);
> +		if (old == val)
> +			break;
> +		if (loop-- <= 0)
> +			goto queue;
> +	}

So I'm not clear about the semantics of what (should) occurs when the
threshold is exhausted. In the trylock/pending loop above, you
immediately return 0, indicating we want to queue. Ok, but below:

> +
> +	if (new == _Q_LOCKED_VAL)
> +		goto gotlock;
> +	/*
> +	 * We are pending, wait for the owner to go away.
> +	 */
> +	while (((val = smp_load_acquire(&lock->val.counter)) & _Q_LOCKED_MASK)
> +		&& (loop-- > 0))
> +		cpu_relax();
> +
> +	if (!(val & _Q_LOCKED_MASK)) {
> +		clear_pending_set_locked(lock);
> +		goto gotlock;
> +	}
> +	/*
> +	 * Clear the pending bit and fall back to queuing
> +	 */
> +	clear_pending(lock);

... you call clear_pending before returning. Is this intentional? Smells
fishy.

And basically afaict all this chunk of code does is spin until loop is
exhausted, and breakout when we got the lock. Ie, something like this is
a lot cleaner:

                while (loop--) {
                	/*
                         * We are pending, wait for the owner to go away.
                         */
                	val = smp_load_acquire(&lock->val.counter);
                	if (!(val & _Q_LOCKED_MASK)) {
                		clear_pending_set_locked(lock);
                		goto gotlock;
                	}
                
                	cpu_relax();		
                }
                
                /*
                 * Clear the pending bit and fall back to queuing
                 */
                clear_pending(lock);
                

> +queue:
> +	return 0;
> +
> +gotlock:
> +	return 1;
> +}
> +

Thanks,
Davidlohr

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1195496

On 07/27/2015 03:39 PM, Davidlohr Bueso wrote:
> On Mon, 2015-07-27 at 13:30 -0400, Waiman Long wrote:
>> The pending bit acts as a 1-slot waiting queue. So if the vCPU needs to
>> fall back to regular queuing, it needs to clear the bit.
> Right, that's what I thought. So you would also need to call
> clear_pending() as soon as the trylock/pending loop hits zero. Or am I
> missing something?
>
> /*
>   * trylock || pending
>   */
> for (;;) {
> 	...
> 	if (loop--<= 0) {
> 		clear_pending(lock);
> 		goto queue;
> 	}
> }
>
> Also, no need to check for negative loop, zero should be enough to
> breakout.

The while loop below can potentially make the loop variable become negative:


         /*
          * wait for in-progress pending->locked hand-overs
          */
         if (val == _Q_PENDING_VAL) {
                 while (((val = atomic_read(&lock->val)) == 
_Q_PENDING_VAL) &&
                         loop--)
                         cpu_relax();
         }

Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web