Message-ID: <55A7105E.5020400@hp.com>
Date: Wed, 15 Jul 2015 22:01:02 -0400
From: Waiman Long <waiman.long@hp.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130109 Thunderbird/10.0.12
MIME-Version: 1.0
To: Peter Zijlstra <peterz@infradead.org>
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 4/6] locking/pvqspinlock: Allow vCPUs kick-ahead
References: <1436926417-20256-1-git-send-email-Waiman.Long@hp.com> <1436926417-20256-5-git-send-email-Waiman.Long@hp.com> <20150715093924.GH2859@worktop.programming.kicks-ass.net>
In-Reply-To: <20150715093924.GH2859@worktop.programming.kicks-ass.net>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3988
Lines: 101

On 07/15/2015 05:39 AM, Peter Zijlstra wrote:
> On Tue, Jul 14, 2015 at 10:13:35PM -0400, Waiman Long wrote:
>> Frequent CPU halting (vmexit) and CPU kicking (vmenter) lengthens
>> critical section and block forward progress.  This patch implements
>> a kick-ahead mechanism where the unlocker will kick the queue head
>> vCPUs as well as up to four additional vCPUs next to the queue head
>> if they were halted.  The kickings are done after exiting the critical
>> section to improve parallelism.
>>
>> The amount of kick-ahead allowed depends on the number of vCPUs
>> in the VM guest.  This patch, by itself, won't do much as most of
>> the kickings are currently done at lock time. Coupled with the next
>> patch that defers lock time kicking to unlock time, it should improve
>> overall system performance in a busy overcommitted guest.
>>
>> Linux kernel builds were run in KVM guest on an 8-socket, 4
>> cores/socket Westmere-EX system and a 4-socket, 8 cores/socket
>> Haswell-EX system. Both systems are configured to have 32 physical
>> CPUs. The kernel build times before and after the patch were:
>>
>> 		    Westmere			Haswell
>>    Patch		32 vCPUs    48 vCPUs	32 vCPUs    48 vCPUs
>>    -----		--------    --------    --------    --------
>>    Before patch	 3m25.0s    10m34.1s	 2m02.0s    15m35.9s
>>    After patch    3m27.4s    10m32.0s	 2m00.8s    14m52.5s
>>
>> There wasn't too much difference before and after the patch.
> That means either the patch isn't worth it, or as you seem to imply its
> in the wrong place in this series.

It needs to be coupled with the next patch to be effective as most of 
the kicking are happening at the lock side, instead of at the unlock 
side. If you look at the sample pvqspinlock stats in patch 3:

lock_kick_count=755354
unlock_kick_count=87

The number of unlock kicks is negligible compared with the lock kicks. 
Patch 5 does have a dependency on patch 4 unless we make it 
unconditionally defers kicking to the unlock call which was what I had 
done in the v1 patch. The reason why I change this in v2 is because I 
found a very slight performance degradation in doing so.

>> @@ -224,7 +233,16 @@ static unsigned int pv_lock_hash_bits __read_mostly;
>>    */
>>   void __init __pv_init_lock_hash(void)
>>   {
>> -	int pv_hash_size = ALIGN(4 * num_possible_cpus(), PV_HE_PER_LINE);
>> +	int ncpus = num_possible_cpus();
>> +	int pv_hash_size = ALIGN(4 * ncpus, PV_HE_PER_LINE);
>> +	int i;
>> +
>> +	/*
>> +	 * The minimum number of vCPUs required in each kick-ahead level
>> +	 */
>> +	static const u8 kick_ahead_threshold[PV_KICK_AHEAD_MAX] = {
>> +		4, 8, 16, 32
>> +	};
> You are aware we have ilog2(), right?

Right, I am not aware of ilog2(). I am going to use that instead.

>> +	/*
>> +	 * Enable the unlock kick ahead mode according to the number of
>> +	 * vCPUs available.
>> +	 */
>> +	for (i = PV_KICK_AHEAD_MAX; i>  0; i--)
>> +		if (ncpus>= kick_ahead_threshold[i - 1]) {
>> +			pv_kick_ahead = i;
>> +			break;
>> +		}
> That's missing { }.
>
>> +	if (pv_kick_ahead)
>> +		pr_info("PV unlock kick ahead level %d enabled\n",
>> +			pv_kick_ahead);
> Idem.

Will fix the {} problems.

> That said, I still really dislike this patch, it again seems a random
> bunch of hacks.

Any suggestions to make it better suit your taste?

> You also do not offer any support for any of the magic numbers..

I chose 4 for PV_KICK_AHEAD_MAX as I didn't see much performance 
difference when I did a kick-ahead of 5. Also, it may be too unfair to 
the vCPU that was doing the kicking if the number is too big. Another 
magic number is  pv_kick_ahead number. This one is kind of arbitrary. 
Right now I do a log2, but it can be divided by 4 (rshift 2) as well.

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/