From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753345AbbIGAGA (ORCPT <rfc822;w@1wt.eu>);
	Sun, 6 Sep 2015 20:06:00 -0400
Received: from mx2.suse.de ([195.135.220.15]:56316 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752722AbbIGAF5 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Sun, 6 Sep 2015 20:05:57 -0400
Date: Sun, 6 Sep 2015 17:05:46 -0700
From: Davidlohr Bueso <dave@stgolabs.net>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
        Dave Chinner <david@fromorbit.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Waiman Long <Waiman.Long@hp.com>, Ingo Molnar <mingo@kernel.org>
Subject: Re: [4.2, Regression] Queued spinlocks cause major XFS performance
 regression
Message-ID: <20150907000546.GA27993@linux-q0g1.site>
References: <20150904054820.GY3902@dastard>
 <CA+55aFyuob5iOOptzdD1W7gsxcrUGkgU50UoLA+Aq29-jO0KSw@mail.gmail.com>
 <20150904071143.GZ3902@dastard>
 <20150904082954.GB3902@dastard>
 <CA+55aFwyUmyOavLjh99qvfv7zn4wJJ9M9sP7baQt_7304g5bbg@mail.gmail.com>
 <20150904151427.GG18489@twins.programming.kicks-ass.net>
 <20150904152523.GR18673@twins.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20150904152523.GR18673@twins.programming.kicks-ass.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 04 Sep 2015, Peter Zijlstra wrote:

>-static inline bool virt_queued_spin_lock(struct qspinlock *lock)
>+static inline bool virt_spin_lock(struct qspinlock *lock)

Given that we fall back to the cmpxchg loop even when PARAVIRT is not in the
picture, I believe this function is horribly misnamed.

> {
> 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> 		return false;
>
>-	while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0)
>-		cpu_relax();
>+	/*
>+	 * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>+	 * back to a Test-and-Set spinlock, because fair locks have
>+	 * horrible lock 'holder' preemption issues.
>+	 */
>+

This comment is also misleading... but if you tuck the whole function
under some PARAVIRT option, it obviously makes sense to just leave as is.
And let native actually _use_ qspinlocks.

>+	do {
>+		while (atomic_read(&lock->val) != 0)
>+			cpu_relax();
>+	} while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);

CCAS to the rescue again.

Thanks,
Davidlohr