From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] [RESEND] KVM:VMX: Add support for Pause-Loop Exiting Date: Tue, 29 Sep 2009 15:34:19 +0200 Message-ID: <4AC20CDB.2070203@redhat.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" To: "Zhai, Edwin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:1301 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752477AbZI2NeT (ORCPT ); Tue, 29 Sep 2009 09:34:19 -0400 In-Reply-To: <4AC082F9.1060502@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On 09/28/2009 11:33 AM, 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. __read_mostly doesn't just mean it's read mostly. It also means it's read often. Otherwise it's just wasting space in hot cachelines. > >> 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. No, global parameters don't work for tuning workloads and guests since they cannot be modified on a per-guest basis. enable_ept is only useful for debugging and testing. > >>> + 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. Right. I thought of the software spinlock detector, but that's another problem. I think you can drop the sleep_time parameter, it can be part of the function. Also kvm_vcpu_sleep() is confusing, we also sleep on halt. Please call it kvm_vcpu_on_spin() or something (since that's what the guest is doing). -- error compiling committee.c: too many arguments to function