George Dunlap
2009-Sep-21 18:04 UTC
[Xen-devel] [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
The attached patch modifies ept_sync_domain() to make it more efficient wrt flushing ept translations. Specifically: * It synchronously flushes only cpus on which the domain is currently running (d->domain_dirty_cpumask) * It introduces a new vmx-specific mask, "ept_needs_flush", set to the complement of d->domain_dirty_cpumask * In vmx_ctxt_switch_to(), if the cpu is set in ept_needs_flush, it flushes ept before running. Main change I''d like reviewed: in order to avoid a potential race condition described below, I had to re-order the setting of domain_dirty_cpumask and the calling of arch.ctxt_switch_to() in __context_switch(). Potential race without the re-ordering: p1: change ept p2: check ept_needs_flush, finds it false, doesn''t flush p1: sets ept_needs_flush p1: checks domain_dirty, finds p2 not in it p2: sets domain_dirty Thus the vcpu on p2 is scheduled without its ept translations being flushed. I''ve tested this with a 2-vcpu VM doing a parallel compile and ballooning, no problems. Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2009-Sep-21 18:04 UTC
[Xen-devel] Re: [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
Oops, patch... -George On Mon, Sep 21, 2009 at 7:04 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote:> The attached patch modifies ept_sync_domain() to make it more > efficient wrt flushing ept translations. > > Specifically: > * It synchronously flushes only cpus on which the domain is currently > running (d->domain_dirty_cpumask) > * It introduces a new vmx-specific mask, "ept_needs_flush", set to the > complement of d->domain_dirty_cpumask > * In vmx_ctxt_switch_to(), if the cpu is set in ept_needs_flush, it > flushes ept before running. > > Main change I''d like reviewed: in order to avoid a potential race > condition described below, I had to re-order the setting of > domain_dirty_cpumask and the calling of arch.ctxt_switch_to() in > __context_switch(). > > Potential race without the re-ordering: > p1: change ept > p2: check ept_needs_flush, finds it false, doesn''t flush > p1: sets ept_needs_flush > p1: checks domain_dirty, finds p2 not in it > p2: sets domain_dirty > > Thus the vcpu on p2 is scheduled without its ept translations being flushed. > > I''ve tested this with a 2-vcpu VM doing a parallel compile and > ballooning, no problems. > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Sep-22 07:07 UTC
[Xen-devel] Re: [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
>--- a/xen/arch/x86/hvm/vmx/vmx.c Tue Sep 15 10:08:12 2009 +0100 >+++ b/xen/arch/x86/hvm/vmx/vmx.c Mon Sep 21 18:52:26 2009 +0100 >... >@@ -1219,7 +1226,11 @@ > if ( d->arch.hvm_domain.hap_enabled && d->vcpu && d->vcpu[0] ) > { > ASSERT(local_irq_is_enabled()); >- on_each_cpu(__ept_sync_domain, d, 1); >+ /* Mark cpus that may need flushing on next schedule */ >+ cpus_complement(d->arch.hvm_domain.vmx.ept_needs_flush, >+ d->domain_dirty_cpumask); >+ /* And flush on actively-running processors */ >+ on_selected_cpus(&d->domain_dirty_cpumask, __ept_sync_domain, d, 1); > } > } >Passing a pointer to the global cpu mask looks racy here: What if a CPU disappears from domain_dirty_cpumask under your feet? Also, merely using cpus_complement() here seem inefficient: It should be possible to accumulate the flush activity, and avoid re-flushing on a CPU which e.g. got flushed on the second from the last run through this code (and not dirtied afterwards). I.e. I''d think there should be cpus_andnot() here, and setting of the bits as CPUs get dirtied. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Sep-22 08:07 UTC
Re: [Xen-devel] Re: [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
On 22/09/2009 08:07, "Jan Beulich" <JBeulich@novell.com> wrote:> Passing a pointer to the global cpu mask looks racy here: What if a CPU > disappears from domain_dirty_cpumask under your feet?I''m fixing this race before I apply the patch.> Also, merely using cpus_complement() here seem inefficient: It should be > possible to accumulate the flush activity, and avoid re-flushing on a CPU > which e.g. got flushed on the second from the last run through this code > (and not dirtied afterwards). I.e. I''d think there should be cpus_andnot() > here, and setting of the bits as CPUs get dirtied.I don''t see how that is possible, as domain_dirty_cpumask can have changed arbitrarily since the previous invocation of ept_sync_domain(), as can the EPT tables. We have to assume every CPU has potentially stale cached mappings. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Sep-22 08:20 UTC
Re: [Xen-devel] Re: [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
On 22/09/2009 09:07, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:> On 22/09/2009 08:07, "Jan Beulich" <JBeulich@novell.com> wrote: > >> Passing a pointer to the global cpu mask looks racy here: What if a CPU >> disappears from domain_dirty_cpumask under your feet? > > I''m fixing this race before I apply the patch.George, Jan, Please see what you think of xen-unstable:20244. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Sep-22 08:23 UTC
Re: [Xen-devel] Re: [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
>>> Keir Fraser <keir.fraser@eu.citrix.com> 22.09.09 10:07 >>> >On 22/09/2009 08:07, "Jan Beulich" <JBeulich@novell.com> wrote: >> Also, merely using cpus_complement() here seem inefficient: It should be >> possible to accumulate the flush activity, and avoid re-flushing on a CPU >> which e.g. got flushed on the second from the last run through this code >> (and not dirtied afterwards). I.e. I''d think there should be cpus_andnot() >> here, and setting of the bits as CPUs get dirtied. > >I don''t see how that is possible, as domain_dirty_cpumask can have changed >arbitrarily since the previous invocation of ept_sync_domain(), as can the >EPT tables. We have to assume every CPU has potentially stale cached >mappings.Why? It ought to be possible to know which CPUs the guest has run on for any period of time. Any CPU it hasn''t run on wouldn''t need flushing, would it? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Sep-22 08:33 UTC
Re: [Xen-devel] Re: [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
On 22/09/2009 09:23, "Jan Beulich" <JBeulich@novell.com> wrote:>> I don''t see how that is possible, as domain_dirty_cpumask can have changed >> arbitrarily since the previous invocation of ept_sync_domain(), as can the >> EPT tables. We have to assume every CPU has potentially stale cached >> mappings. > > Why? It ought to be possible to know which CPUs the guest has run on > for any period of time. Any CPU it hasn''t run on wouldn''t need flushing, > would it?Probably not, as long as VMCLEAR is sufficient to stop a particular EPTP''s entries being cached? We''d need to maintain an extra cpumask to track this however, and it''s not clear it''s worth it. This is really just about optimising large numbers of almost back-to-back invocations of ept_sync_domain(), where the # flushes of not-currently-active cpus will probably collapse to 1 with the current logic in the patch. The time between batches of calls should be large enough that the extra flushes caused by not tracking VCPU movements between the batches really shouldn''t matter very much. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Sep-22 09:02 UTC
Re: [Xen-devel] Re: [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
>>> Keir Fraser <keir.fraser@eu.citrix.com> 22.09.09 10:20 >>> >On 22/09/2009 09:07, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote: > >> On 22/09/2009 08:07, "Jan Beulich" <JBeulich@novell.com> wrote: >> >>> Passing a pointer to the global cpu mask looks racy here: What if a CPU >>> disappears from domain_dirty_cpumask under your feet? >> >> I''m fixing this race before I apply the patch. > >George, Jan, > >Please see what you think of xen-unstable:20244.With no assertion in ept_sync_domain() on any locks held, is it guaranteed that the function cannot be entered twice at the same time for a given guest? If not, passing a pointer to the new ept_synced member isn''t any better than passing the one to domain_dirty_cpumask. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Sep-22 09:17 UTC
Re: [Xen-devel] Re: [PATCH] EPT: Flush running cpus, add mask to flush when scheduled in
On 22/09/2009 10:02, "Jan Beulich" <JBeulich@novell.com> wrote:>> Please see what you think of xen-unstable:20244. > > With no assertion in ept_sync_domain() on any locks held, is it guaranteed > that the function cannot be entered twice at the same time for a given > guest? If not, passing a pointer to the new ept_synced member isn''t any > better than passing the one to domain_dirty_cpumask.I assume George is knowledgeable on that area. If calls to ept_sync_domain() are not serialised then I think synchronisation around the ept_needs_flush/ept_synced cpumask is indeed pretty suspect. If there isn''t such a serialising lock, we could add one to ept_sync_domain() quite safely. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel