On Tue, Oct 04, 2011 at 11:28:51AM +0200, Avi Kivity wrote: > On 10/03/2011 05:36 PM, Gleb Natapov wrote: > >On Mon, Oct 03, 2011 at 05:00:25PM +0200, Avi Kivity wrote: > >> On 10/03/2011 03:49 PM, Gleb Natapov wrote: > >> >Support guest/host-only profiling by switch perf msrs on > >> >a guest entry if needed. > >> > > >> >@@ -6052,6 +6056,26 @@ static void vmx_cancel_injection(struct kvm_vcpu *vcpu) > >> > vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0); > >> > } > >> > > >> >+static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx) > >> >+{ > >> >+#ifdef CONFIG_PERF_EVENTS > >> > >> No need for #ifdef (if you also define perf_guest_get_msrs() when > >> !CONFIG_PERF_EVENTS). > >> > >Yes, but will compiler be smart enough to remove the code of the > >function completely? It will have to figure that vmx->perf_msrs_cnt is > >always 0 somehow. > > It won't, but do we care? > Dead code, that likely to be inlined, on a hot path. > >> > > >> >+ > >> >+ perf_guest_get_msrs(vmx->perf_msrs_cnt, vmx->perf_msrs); > >> >+ for (i = 0; i< vmx->perf_msrs_cnt; i++) { > >> >+ struct perf_guest_switch_msr *msr =&vmx->perf_msrs[i]; > >> >+ if (msr->host == msr->guest) > >> >+ clear_atomic_switch_msr(vmx, msr->msr); > >> >+ else > >> >+ add_atomic_switch_msr(vmx, msr->msr, msr->guest, > >> >+ msr->host); > >> > >> This generates a lot of VMWRITEs even if nothing changes, just to > >> re-set bits in the VMCS to their existing values. Need to add > >> something like this: > >> > >> if (loaded_vmcs->msr[i].host == msr->host > >> && loaded_vmcs->msr[i].guest == msr->guest) > >> continue; > >VMWRITE happens only when number of autoloaded MSRs changes (which is > >rare), not on each call to add_atomic_switch_msr(). I thought about > >optimizing this write too by doing > >vmcs_write32(VM_(ENTRY|EXIT)_MSR_LOAD_COUNT, m->nr) only once by > >checking that m->nr changed during vmentry. Can be done later. > > For EFER and PERF_CTRL, it's done unconditionally, no? For those yes. We do not cache their value currently. Can be added, but this is independent optimization. > > >> > >> btw, shouldn't the msr autoload list be part of loaded_vmcs as well? > >> > >Why? > > Any caching is only relative to the vmcs (unless we invalidate the > cache on vmcs switch). Ah, you are talking about not yet existent cache. Then I see why it should be in loaded_vmcs for EFER and PERF_CTRL. > > >> > >> Do we really need a private buffer? Perhaps perf_guest_get_msrs() > >> can return a perf-internal buffer (but then, we will need to copy it > >> for the optimization above, but that's a separate issue). > >> > >The buffer will be small, so IMHO private one is not an issue. We can > >make it perf internal per cpu buffer I think. > > > > I think the API is nicer with perf returning a read-only internal > buffer; this way there is no kmalloc involved since perf knows its > internal limits. > Yeah, I am trying it now it it looks nicer. -- Gleb. -- 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