[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCHv3 2/2] x86/ept: defer the invalidation until the p2m lock is released



On 03/12/15 16:42, David Vrabel wrote:
> From: David Vrabel <dvrabel@xxxxxxxxxx>
> 
> Holding the p2m lock while calling ept_sync_domain() is very expensive
> since it does a on_selected_cpus() call.  IPIs on many socket machines
> can be very slows and on_selected_cpus() is serialized.
> 
> Defer the invalidate until the p2m lock is released.  Since the processor
> may cache partial translations, we also need to make sure any page table
> pages to be freed are not freed until the invalidate is complete.  Such
> pages are temporarily stored in a list.
> 
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>

Looks good.

One thing is that now (actually since the previous patch) the flush in
__ept_sync_domain() itself is actually entirely redundant, as the check
will happen on the vmentry path again anyway.  But I think you do want
to wait until at least all vcpus have been interrupted, and
on_selected_cpus() looks like the simplest way to do that.  (Otherwise
you could just call smp_send_event_check_mask() and get rid of
__ept_sync_domain() entirely.)

Jan / Andy, any thoughts?

 -George

> ---
> v2:
> - use per-p2m list for deferred pages.
> - update synced_mask while holding write lock.
> ---
>  xen/arch/x86/mm/mm-locks.h | 23 ++++++++++++++--------
>  xen/arch/x86/mm/p2m-ept.c  | 48 
> +++++++++++++++++++++++++++++++++++++---------
>  xen/arch/x86/mm/p2m.c      | 18 +++++++++++++++++
>  xen/include/asm-x86/p2m.h  |  7 +++++++
>  4 files changed, 79 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
> index 76c7217..b5eb560 100644
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -263,14 +263,21 @@ declare_mm_lock(altp2mlist)
>   */
>  
>  declare_mm_rwlock(altp2m);
> -#define p2m_lock(p)                         \
> -{                                           \
> -    if ( p2m_is_altp2m(p) )                 \
> -        mm_write_lock(altp2m, &(p)->lock);  \
> -    else                                    \
> -        mm_write_lock(p2m, &(p)->lock);     \
> -}
> -#define p2m_unlock(p)         mm_write_unlock(&(p)->lock);
> +#define p2m_lock(p)                             \
> +    do {                                        \
> +        if ( p2m_is_altp2m(p) )                 \
> +            mm_write_lock(altp2m, &(p)->lock);  \
> +        else                                    \
> +            mm_write_lock(p2m, &(p)->lock);     \
> +        (p)->defer_flush++;                     \
> +    } while (0)
> +#define p2m_unlock(p)                                                   \
> +    do {                                                                \
> +        if ( --(p)->defer_flush == 0 && (p)->need_flush )               \
> +            (p)->flush_and_unlock(p);                                   \
> +        else                                                            \
> +            mm_write_unlock(&(p)->lock);                                \
> +    } while (0)
>  #define gfn_lock(p,g,o)       p2m_lock(p)
>  #define gfn_unlock(p,g,o)     p2m_unlock(p)
>  #define p2m_read_lock(p)      mm_read_lock(p2m, &(p)->lock)
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 014e2b2..bec27d7 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -263,7 +263,7 @@ static void ept_free_entry(struct p2m_domain *p2m, 
> ept_entry_t *ept_entry, int l
>          unmap_domain_page(epte);
>      }
>      
> -    p2m_free_ptp(p2m, mfn_to_page(ept_entry->mfn));
> +    p2m_free_ptp_defer(p2m, mfn_to_page(ept_entry->mfn));
>  }
>  
>  static bool_t ept_split_super_page(struct p2m_domain *p2m,
> @@ -1096,24 +1096,53 @@ static void __ept_sync_domain(void *info)
>          __invept(INVEPT_SINGLE_CONTEXT, ept_get_eptp(ept), 0);
>  }
>  
> -void ept_sync_domain(struct p2m_domain *p2m)
> +static void ept_sync_domain_prepare(struct p2m_domain *p2m)
>  {
>      struct domain *d = p2m->domain;
>      struct ept_data *ept = &p2m->ept;
> -    /* Only if using EPT and this domain has some VCPUs to dirty. */
> -    if ( !paging_mode_hap(d) || !d->vcpu || !d->vcpu[0] )
> -        return;
> -
> -    ASSERT(local_irq_is_enabled());
>  
>      if ( nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) )
>          p2m_flush_nestedp2m(d);
>  
>      /* May need to invalidate on all PCPUs. */
>      cpumask_setall(ept->invalidate);
> +}
> +
> +static void ept_sync_domain_mask(struct p2m_domain *p2m, const cpumask_t 
> *mask)
> +{
> +    on_selected_cpus(mask, __ept_sync_domain, p2m, 1);
> +}
> +
> +void ept_sync_domain(struct p2m_domain *p2m)
> +{
> +    struct domain *d = p2m->domain;
> +
> +    /* Only if using EPT and this domain has some VCPUs to dirty. */
> +    if ( !paging_mode_hap(d) || !d->vcpu || !d->vcpu[0] )
> +        return;
> +
> +    ept_sync_domain_prepare(p2m);
> +
> +    if ( p2m->defer_flush )
> +    {
> +        p2m->need_flush = 1;
> +        return;
> +    }
> +    p2m->need_flush = 0;
> +
> +    ept_sync_domain_mask(p2m, d->domain_dirty_cpumask);
> +}
> +
> +static void ept_flush_and_unlock(struct p2m_domain *p2m)
> +{
> +    PAGE_LIST_HEAD(deferred_pages);
> +
> +    page_list_move(&deferred_pages, &p2m->deferred_pages);
> +
> +    mm_write_unlock(&p2m->lock);
>  
> -    on_selected_cpus(d->domain_dirty_cpumask,
> -                     __ept_sync_domain, p2m, 1);
> +    ept_sync_domain_mask(p2m, p2m->domain->domain_dirty_cpumask);
> +    p2m_free_ptp_list(p2m, &deferred_pages);
>  }
>  
>  static void ept_enable_pml(struct p2m_domain *p2m)
> @@ -1164,6 +1193,7 @@ int ept_p2m_init(struct p2m_domain *p2m)
>      p2m->change_entry_type_range = ept_change_entry_type_range;
>      p2m->memory_type_changed = ept_memory_type_changed;
>      p2m->audit_p2m = NULL;
> +    p2m->flush_and_unlock = ept_flush_and_unlock;
>  
>      /* Set the memory type used when accessing EPT paging structures. */
>      ept->ept_mt = EPT_DEFAULT_MT;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index ed0bbd7..502bdad 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -65,6 +65,7 @@ static int p2m_initialise(struct domain *d, struct 
> p2m_domain *p2m)
>      mm_lock_init(&p2m->pod.lock);
>      INIT_LIST_HEAD(&p2m->np2m_list);
>      INIT_PAGE_LIST_HEAD(&p2m->pages);
> +    INIT_PAGE_LIST_HEAD(&p2m->deferred_pages);
>      INIT_PAGE_LIST_HEAD(&p2m->pod.super);
>      INIT_PAGE_LIST_HEAD(&p2m->pod.single);
>  
> @@ -504,6 +505,23 @@ void p2m_free_ptp(struct p2m_domain *p2m, struct 
> page_info *pg)
>      return;
>  }
>  
> +void p2m_free_ptp_defer(struct p2m_domain *p2m, struct page_info *pg)
> +{
> +    page_list_del(pg, &p2m->pages);
> +    page_list_add(pg, &p2m->deferred_pages);
> +}
> +
> +void p2m_free_ptp_list(struct p2m_domain *p2m, struct page_list_head *list)
> +{
> +    struct page_info *pg, *tmp;
> +
> +    page_list_for_each_safe(pg, tmp, list)
> +    {
> +        page_list_del(pg, list);
> +        p2m->domain->arch.paging.free_page(p2m->domain, pg);
> +    }
> +}
> +
>  /*
>   * Allocate a new p2m table for a domain.
>   *
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index fa46dd9..ee03997 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -261,6 +261,11 @@ struct p2m_domain {
>                                            unsigned long gfn, l1_pgentry_t *p,
>                                            l1_pgentry_t new, unsigned int 
> level);
>      long               (*audit_p2m)(struct p2m_domain *p2m);
> +    void               (*flush_and_unlock)(struct p2m_domain *p2m);
> +
> +    struct page_list_head deferred_pages;
> +    unsigned int defer_flush;
> +    bool_t need_flush;
>  
>      /* Default P2M access type for each page in the the domain: new pages,
>       * swapped in pages, cleared pages, and pages that are ambiguously
> @@ -688,6 +693,8 @@ static inline bool_t p2m_mem_access_sanity_check(struct 
> domain *d)
>  
>  struct page_info *p2m_alloc_ptp(struct p2m_domain *p2m, unsigned long type);
>  void p2m_free_ptp(struct p2m_domain *p2m, struct page_info *pg);
> +void p2m_free_ptp_defer(struct p2m_domain *p2m, struct page_info *pg);
> +void p2m_free_ptp_list(struct p2m_domain *p2m, struct page_list_head *list);
>  
>  /* Directly set a p2m entry: only for use by p2m code. Does not need
>   * a call to put_gfn afterwards/ */
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.