Date: Wed, 15 Jul 2015 12:01:41 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Waiman Long <Waiman.Long@hp.com>
Cc: Ingo Molnar <mingo@redhat.com>, Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        linux-kernel@vger.kernel.org, Scott J Norton <scott.norton@hp.com>,
        Douglas Hatch <doug.hatch@hp.com>, Davidlohr Bueso <dave@stgolabs.net>
Subject: Re: [PATCH v2 6/6] locking/pvqspinlock: Queue node adaptive spinning
Message-ID: <20150715100141.GI2859@worktop.programming.kicks-ass.net>
References: <1436926417-20256-1-git-send-email-Waiman.Long@hp.com>
 <1436926417-20256-7-git-send-email-Waiman.Long@hp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1436926417-20256-7-git-send-email-Waiman.Long@hp.com>
User-Agent: Mutt/1.5.22.1 (2013-10-16)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2850
Lines: 100

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?
--
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/