From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v7 11/19] x86/VPMU: Interface for setting PMU mode and flags Date: Fri, 06 Jun 2014 16:42:21 -0400 Message-ID: <539227AD.5040400@oracle.com> References: <1402076415-26475-1-git-send-email-boris.ostrovsky@oracle.com> <1402076415-26475-12-git-send-email-boris.ostrovsky@oracle.com> <53921D70.6000001@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53921D70.6000001@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: kevin.tian@intel.com, keir@xen.org, JBeulich@suse.com, jun.nakajima@intel.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, suravee.suthikulpanit@amd.com List-Id: xen-devel@lists.xenproject.org On 06/06/2014 03:58 PM, Andrew Cooper wrote: > On 06/06/14 18:40, Boris Ostrovsky wrote: >> Add runtime interface for setting PMU mode and flags. Three main modes are >> provided: >> * PMU off >> * PMU on: Guests can access PMU MSRs and receive PMU interrupts. dom0 >> profiles itself and the hypervisor. >> * dom0-only PMU: dom0 collects samples for both itself and guests. >> >> For feature flags only Intel's BTS is currently supported. >> >> Mode and flags are set via HYPERVISOR_xenpmu_op hypercall. >> >> Signed-off-by: Boris Ostrovsky >> Reviewed-by: Dietmar Hahn >> Tested-by: Dietmar Hahn >> --- >> >> @@ -275,3 +283,100 @@ void vpmu_dump(struct vcpu *v) >> vpmu->arch_vpmu_ops->arch_vpmu_dump(v); >> } >> >> +/* Unload VPMU contexts */ >> +static void vpmu_unload_all(void) >> +{ >> + struct domain *d; >> + struct vcpu *v; >> + struct vpmu_struct *vpmu; >> + >> + for_each_domain( d ) >> + { >> + for_each_vcpu ( d, v ) >> + { >> + if ( v != current ) >> + vcpu_pause(v); >> + vpmu = vcpu_vpmu(v); >> + >> + if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) ) >> + { >> + if ( v != current ) >> + vcpu_unpause(v); >> + continue; >> + } >> + >> + vpmu_set(vpmu, VPMU_CONTEXT_SAVE); >> + on_selected_cpus(cpumask_of(vpmu->last_pcpu), >> + vpmu_save_force, (void *)v, 1); >> + vpmu_reset(vpmu, VPMU_CONTEXT_LOADED); >> + >> + if ( v != current ) >> + vcpu_unpause(v); >> + } >> + } >> +} > This is a very long function without any preemption. > > To reduce its impact greatly, would it be better to have a global mode > controlling vcpu context switching? You could enable > "VCPU_GLOBAL_DISABLING", then IPI all cpus at the same time to save > their vpmu context. Once the IPI is complete, all vpmu context is clean. > > This avoids a double loop and all the vcpu_pause/unpause()s. I can see the problem but I am not sure what you are suggesting will work (or perhaps I don't quite understand what you are suggesting). If I IPI a processor may be in the middle of working on the VPMU context. Let me think about this. > >> + >> + >> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg) >> +{ >> + int ret = -EINVAL; >> + xen_pmu_params_t pmu_params; > This is probably going to want to be XSM'd up for permissions? I haven't looked at this since earlier versions of the patch but when I did it turned out we were out of bits in the access vector (or whatever that structure was). I in fact mentioned this problem in the cover letter for the series. -boris