From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Zhai, Edwin" Subject: Re: [PATCH] [RESEND] KVM:VMX: Add support for Pause-Loop Exiting Date: Tue, 29 Sep 2009 20:05:48 +0800 Message-ID: <4AC1F81C.9050001@intel.com> References: <4ABA2AD7.6080008@intel.com> <4ABA2C22.7020000@redhat.com> <4ABC18C6.9070202@intel.com> <4ABF2221.4000505@redhat.com> <4AC082F9.1060502@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "kvm@vger.kernel.org" , "Zhai, Edwin" To: Avi Kivity Return-path: Received: from mga01.intel.com ([192.55.52.88]:32035 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752235AbZI2MFp (ORCPT ); Tue, 29 Sep 2009 08:05:45 -0400 In-Reply-To: <4AC082F9.1060502@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: Avi, Any comments for this new patch? Thanks, Zhai, Edwin wrote: > Avi Kivity wrote: > >> +#define KVM_VMX_DEFAULT_PLE_GAP 41 >> +#define KVM_VMX_DEFAULT_PLE_WINDOW 4096 >> +static int __read_mostly ple_gap = KVM_VMX_DEFAULT_PLE_GAP; >> +module_param(ple_gap, int, S_IRUGO); >> + >> +static int __read_mostly ple_window = KVM_VMX_DEFAULT_PLE_WINDOW; >> +module_param(ple_window, int, S_IRUGO); >> >> >> >> Shouldn't be __read_mostly since they're read very rarely (__read_mostly >> should be for variables that are very often read, and rarely written). >> >> > > In general, they are read only except that experienced user may try > different parameter for perf tuning. > > >> I'm not even sure they should be parameters. >> >> > > For different spinlock in different OS, and for different workloads, we > need different parameter for tuning. It's similar as the enable_ept. > > >> >> >>> /* >>> + * Indicate a busy-waiting vcpu in spinlock. We do not enable the PAUSE >>> + * exiting, so only get here on cpu with PAUSE-Loop-Exiting. >>> + */ >>> +static int handle_pause(struct kvm_vcpu *vcpu, >>> + struct kvm_run *kvm_run) >>> +{ >>> + ktime_t expires; >>> + skip_emulated_instruction(vcpu); >>> + >>> + /* Sleep for 1 msec, and hope lock-holder got scheduled */ >>> + expires = ktime_add_ns(ktime_get(), 1000000UL); >>> >>> >>> >> I think this should be much lower, 50-100us. Maybe this should be a >> parameter. With 1ms we losing significant cpu time if the congestion >> clears. >> >> > > I have made it a parameter with default value of 100 us. > > >> >> >>> + set_current_state(TASK_INTERRUPTIBLE); >>> + schedule_hrtimeout(&expires, HRTIMER_MODE_ABS); >>> + >>> >>> >>> >> Please add a tracepoint for this (since it can cause significant change >> in behaviour), >> > > Isn't trace_kvm_exit(exit_reason, ...) enough? We can tell the PLE > vmexit from other vmexits. > > >> and move the logic to kvm_main.c. It will be reused by >> the AMD implementation, possibly my software spinlock detector, >> paravirtualized spinlocks, and hopefully other architectures. >> >> > > Done. > >> >> >>> + return 1; >>> +} >>> + >>> +/* >>> >>> >>> >> >>