On Wed, Sep 30, 2009 at 09:01:51AM +0800, Zhai, Edwin wrote: > Avi, > I modify it according your comments. The only thing I want to keep is > the module param ple_gap/window. Although they are not per-guest, they > can be used to find the right value, and disable PLE for debug purpose. > > Thanks, > > > Avi Kivity wrote: >> 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). kvm_vcpu_on_spin() should add the vcpu to vcpu->wq (so a new pending interrupt wakes it up immediately). Do you (and/or Mark) have any numbers for non-vcpu overcommited guests? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
