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

Groups > linux.kernel > #1390183 > unrolled thread

Re: [RESEND PATCH v3] locking/pvqspinlock: Add lock holder CPU argument to pv_wait()

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: [RESEND PATCH v3] locking/pvqspinlock: Add lock holder CPU  argument to pv_wait() Boqun Feng <boqun.feng@gmail.com> - 2016-04-28 15:30 +0200
    Re: [RESEND PATCH v3] locking/pvqspinlock: Add lock holder CPU argument  to pv_wait() Waiman Long <waiman.long@hpe.com> - 2016-04-28 23:00 +0200

#1390183 — Re: [RESEND PATCH v3] locking/pvqspinlock: Add lock holder CPU argument to pv_wait()

[Multipart message — attachments visible in raw view] — view raw

Hi Waiman,

On Thu, Apr 21, 2016 at 11:02:05AM -0400, Waiman Long wrote:
> Pan Xinhui was asking for a lock holder cpu argument in pv_wait()
> to help the porting of pvqspinlock to PPC. The new argument will can
> help hypervisor expediate the execution of the critical section by
> the lock holder, if its vCPU isn't running, so that it can release
> the lock sooner.
> 
> The pv_wait() function is now extended to have a third argument
> that holds the vCPU number of the lock holder, if this is
> known. Architecture specific hypervisor code can then make use of
> that information to make better decision of which vCPU should be
> running next.
> 
> This patch also adds a new field in the pv_node structure to hold
> the vCPU number of the previous queue entry. For the queue head,
> the prev_cpu entry is likely to be the that of the lock holder,
> though lock stealing may render this information inaccurate.
> 
> In pv_wait_head_or_lock(), pv_wait() will now be called with that
> vCPU number as it is likely to be the lock holder.
> 
> In pv_wait_node(), the newly added pv_lookup_hash() function will
> be called to look up the queue head and pass in the lock holder vCPU
> number stored there, if found.
> 
> Suggested-by:  Pan Xinhui <xinhui@linux.vnet.ibm.com>
> Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
> ---
> 
>  v2->v3:
>   - Rephrase the changelog and some of the comments to make the
>     intention of this patch more clear.
>   - Make pv_lookup_hash() architecture dependent to eliminate its cost
>     if an architecture doesn't need it. Also make the number of
>     cachelines searched in pv_lookup_hash() to between 1-4.
>   - [RESEND] Fix typo error.
> 
>  v1->v2:
>   - In pv_wait_node(), lookup the hash table for the queue head and pass
>     the lock holder cpu number to pv_wait().
> 

[snip]

> @@ -282,7 +328,8 @@ static void pv_init_node(struct mcs_spinlock *node)
>   * pv_kick_node() is used to set _Q_SLOW_VAL and fill in hash table on its
>   * behalf.
>   */
> -static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
> +static void pv_wait_node(struct qspinlock *lock, struct mcs_spinlock *node,
> +			 struct mcs_spinlock *prev)
>  {
>  	struct pv_node *pn = (struct pv_node *)node;
>  	struct pv_node *pp = (struct pv_node *)prev;
> @@ -290,6 +337,8 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
>  	int loop;
>  	bool wait_early;
>  
> +	pn->prev_cpu = pp->cpu;	/* Save vCPU # of previous queue entry */
> +
>  	/* waitcnt processing will be compiled out if !QUEUED_LOCK_STAT */
>  	for (;; waitcnt++) {
>  		for (wait_early = false, loop = SPIN_THRESHOLD; loop; loop--) {
> @@ -314,10 +363,23 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
>  		smp_store_mb(pn->state, vcpu_halted);
>  
>  		if (!READ_ONCE(node->locked)) {
> +			struct pv_node *ph;
> +
>  			qstat_inc(qstat_pv_wait_node, true);
>  			qstat_inc(qstat_pv_wait_again, waitcnt);
>  			qstat_inc(qstat_pv_wait_early, wait_early);
> -			pv_wait(&pn->state, vcpu_halted);
> +
> +			/*
> +			 * If the current queue head is in the hash table,
> +			 * the prev_cpu field of its pv_node may contain the
> +			 * vCPU # of the lock holder. However, lock stealing
> +			 * may make that information inaccurate. Anyway, we
> +			 * look up the hash table to try to get the lock
> +			 * holder vCPU number.
> +			 */
> +			ph = pv_lookup_hash(lock);

I am thinking, could we save the hash lookup here if wait_early == true?
Because in that case, the previous node is likely to get the lock soon,
it makes sense we yield to that node rather than the holder, so may we
can do something like:

			if (wait_early)
				pv_wait(&pn->state, vcpu_halted, pn->prev_cpu);
			else {
				ph = pv_lookup_hash(lock);
				pv_wait(&pn->state, vcpu_halted,
					ph ? ph->prev_cpu : -1);
			}

Thoughts?

Regards,
Boqun

> +			pv_wait(&pn->state, vcpu_halted,
> +				ph ? ph->prev_cpu : -1);
>  		}
>  
>  		/*

[snip]

[toc] | [next] | [standalone]

#1390526 — Re: [RESEND PATCH v3] locking/pvqspinlock: Add lock holder CPU argument to pv_wait()

On 04/28/2016 09:32 AM, Boqun Feng wrote:
> Hi Waiman,
>
> On Thu, Apr 21, 2016 at 11:02:05AM -0400, Waiman Long wrote:
>> Pan Xinhui was asking for a lock holder cpu argument in pv_wait()
>> to help the porting of pvqspinlock to PPC. The new argument will can
>> help hypervisor expediate the execution of the critical section by
>> the lock holder, if its vCPU isn't running, so that it can release
>> the lock sooner.
>>
>> The pv_wait() function is now extended to have a third argument
>> that holds the vCPU number of the lock holder, if this is
>> known. Architecture specific hypervisor code can then make use of
>> that information to make better decision of which vCPU should be
>> running next.
>>
>> This patch also adds a new field in the pv_node structure to hold
>> the vCPU number of the previous queue entry. For the queue head,
>> the prev_cpu entry is likely to be the that of the lock holder,
>> though lock stealing may render this information inaccurate.
>>
>> In pv_wait_head_or_lock(), pv_wait() will now be called with that
>> vCPU number as it is likely to be the lock holder.
>>
>> In pv_wait_node(), the newly added pv_lookup_hash() function will
>> be called to look up the queue head and pass in the lock holder vCPU
>> number stored there, if found.
>>
>> Suggested-by:  Pan Xinhui<xinhui@linux.vnet.ibm.com>
>> Signed-off-by: Waiman Long<Waiman.Long@hpe.com>
>> ---
>>
>>   v2->v3:
>>    - Rephrase the changelog and some of the comments to make the
>>      intention of this patch more clear.
>>    - Make pv_lookup_hash() architecture dependent to eliminate its cost
>>      if an architecture doesn't need it. Also make the number of
>>      cachelines searched in pv_lookup_hash() to between 1-4.
>>    - [RESEND] Fix typo error.
>>
>>   v1->v2:
>>    - In pv_wait_node(), lookup the hash table for the queue head and pass
>>      the lock holder cpu number to pv_wait().
>>
> [snip]
>
>> @@ -282,7 +328,8 @@ static void pv_init_node(struct mcs_spinlock *node)
>>    * pv_kick_node() is used to set _Q_SLOW_VAL and fill in hash table on its
>>    * behalf.
>>    */
>> -static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
>> +static void pv_wait_node(struct qspinlock *lock, struct mcs_spinlock *node,
>> +			 struct mcs_spinlock *prev)
>>   {
>>   	struct pv_node *pn = (struct pv_node *)node;
>>   	struct pv_node *pp = (struct pv_node *)prev;
>> @@ -290,6 +337,8 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
>>   	int loop;
>>   	bool wait_early;
>>
>> +	pn->prev_cpu = pp->cpu;	/* Save vCPU # of previous queue entry */
>> +
>>   	/* waitcnt processing will be compiled out if !QUEUED_LOCK_STAT */
>>   	for (;; waitcnt++) {
>>   		for (wait_early = false, loop = SPIN_THRESHOLD; loop; loop--) {
>> @@ -314,10 +363,23 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
>>   		smp_store_mb(pn->state, vcpu_halted);
>>
>>   		if (!READ_ONCE(node->locked)) {
>> +			struct pv_node *ph;
>> +
>>   			qstat_inc(qstat_pv_wait_node, true);
>>   			qstat_inc(qstat_pv_wait_again, waitcnt);
>>   			qstat_inc(qstat_pv_wait_early, wait_early);
>> -			pv_wait(&pn->state, vcpu_halted);
>> +
>> +			/*
>> +			 * If the current queue head is in the hash table,
>> +			 * the prev_cpu field of its pv_node may contain the
>> +			 * vCPU # of the lock holder. However, lock stealing
>> +			 * may make that information inaccurate. Anyway, we
>> +			 * look up the hash table to try to get the lock
>> +			 * holder vCPU number.
>> +			 */
>> +			ph = pv_lookup_hash(lock);
> I am thinking, could we save the hash lookup here if wait_early == true?
> Because in that case, the previous node is likely to get the lock soon,
> it makes sense we yield to that node rather than the holder, so may we
> can do something like:
>
> 			if (wait_early)
> 				pv_wait(&pn->state, vcpu_halted, pn->prev_cpu);
> 			else {
> 				ph = pv_lookup_hash(lock);
> 				pv_wait(&pn->state, vcpu_halted,
> 					ph ? ph->prev_cpu : -1);
> 			}
>
> Thoughts?
>
> Regards,
> Boqun
>

One reason why I don't want to just wake up the previous node vCPU is 
because I am concerned that a vCPU may go into a suspend-wakeup loop. So 
I will wait until we get experimental evidence that this really help 
performance before we commit to this kind of change.

Cheers,
Longman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web