Re: [PATCH v2 6/6] locking/pvqspinlock: Queue node adaptive spinning

[Waiman Long <waiman.long@hp.com>]

On 07/15/2015 06:01 AM, Peter Zijlstra wrote:
> On Tue, Jul 14, 2015 at 10:13:37PM -0400, Waiman Long wrote:
>> +static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
>>   {
>>   	struct pv_node *pn = (struct pv_node *)node;
>> +	struct pv_node *pp = (struct pv_node *)prev;
>> +	bool wait_early, can_wait_early;
>>   	int loop;
>>
>>   	for (;;) {
>> -		for (loop = SPIN_THRESHOLD; loop; loop--) {
>> +		/*
>> +		 * Spin less if the previous vCPU was in the halted state
>> +		 * and it is not the queue head.
>> +		 */
>> +		can_wait_early = (pn->waithist>  PV_WAITHIST_THRESHOLD);
>> +		wait_early = can_wait_early&&  !READ_ONCE(prev->locked)&&
>> +			     (READ_ONCE(pp->state) == vcpu_halted);
>> +		loop = wait_early ? QNODE_SPIN_THRESHOLD_SHORT
>> +				  : QNODE_SPIN_THRESHOLD;
>> +		for (; loop; loop--, cpu_relax()) {
>> +			bool halted;
>> +
>>   			if (READ_ONCE(node->locked))
>>   				return;
>> -			cpu_relax();
>> +
>> +			if (!can_wait_early || (loop&  QNODE_SPIN_CHECK_MASK))
>> +				continue;
>> +
>> +			/*
>> +			 * Look for state transition at previous node.
>> +			 *
>> +			 * running =>  halted:
>> +			 *	call pv_wait() now if kick-ahead is enabled
>> +			 *	or reduce spin threshold to
>> +			 *	QNODE_SPIN_THRESHOLD_SHORT or less.
>> +			 * halted =>  running:
>> +			 *	reset spin threshold to QNODE_SPIN_THRESHOLD
>> +			 */
>> +			halted = (READ_ONCE(pp->state) == vcpu_halted)&&
>> +				 !READ_ONCE(prev->locked);
>> +			if (wait_early == halted)
>> +				continue;
>> +			wait_early = halted;
>> +
>> +			if (!wait_early)
>> +				loop = QNODE_SPIN_THRESHOLD;
>> +			else if (pv_kick_ahead)
>> +				break;
>> +			else if (loop>  QNODE_SPIN_THRESHOLD_SHORT)
>> +				loop = QNODE_SPIN_THRESHOLD_SHORT;
>>   		}
>> +		if (wait_early)
>> +			pvstat_inc(pvstat_wait_early);
>> +
>> +		/*
>> +		 * A pv_wait while !wait_early has higher weight than when
>> +		 * wait_early is true.
>> +		 */
>> +		if (pn->waithist<  PV_WAITHIST_MAX)
>> +			pn->waithist += wait_early ? 1 : PV_WAIT_INC;
> So when you looked at this patch, you didn't go like, OMFG!?
>
> So what was wrong with something like:
>
> static inline int pv_spin_threshold(struct pv_node *prev)
> {
> 	if (READ_ONCE(prev->locked)) /* it can run, wait for it */
> 		return SPIN_THRESHOLD;
>
> 	if (READ_ONCE(prev->state) == vcpu_halted) /* its not running, do not wait */
> 		return 1;
>
> 	return SPIN_THRESHOLD;
> }
>
> static void pv_wait_head(...)
> {
> 	for (;;) {
> 		for (loop = pv_spin_threshold(pp); loop; loop--) {
> 			if (READ_ONCE(node->locked))
> 				return;
> 			cpu_relax();
> 		}
>
> 		if (!lp) {
> 			...
> 		}
> 		pv_wait(&l->locked, _Q_SLOW_VAL);
> 	}
> }
>
> What part of: "keep it simple" and "gradual complexity" have you still
> not grasped?

I confess that I was a bit sloppy in that part of the code. I want to 
get it out for review ASAP without doing too much fine tuning as I 
expect at least a few iterations for this patchset. I will certainly 
change it in the new patch. Anyway, thanks for the great suggestion.

Cheers,
Longman