* [PATCH for-4.6 v2] x86/mm: Make {hap, shadow}_teardown() preemptible
@ 2015-08-05 15:47 Andrew Cooper
2015-08-05 16:17 ` Wei Liu
2015-08-05 17:00 ` George Dunlap
0 siblings, 2 replies; 4+ messages in thread
From: Andrew Cooper @ 2015-08-05 15:47 UTC (permalink / raw)
To: Xen-devel
Cc: George Dunlap, Andrew Cooper, Anshul Makkar, Wei Liu, Jan Beulich
From: Anshul Makkar <anshul.makkar@citrix.com>
A domain with sufficient shadow allocation can cause a watchdog timeout
during domain destruction. Expand the existing -ERESTART logic in
paging_teardown() to allow {hap/sh}_set_allocation() to become
restartable during the DOMCTL_destroydomain hypercall.
Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
v2:
* Drop {HAP,SHADOW}_PRINTK(). They are not very useful, and will get
very noisy if enabled.
Wei: Currently, Xen can't actually run 1TB guests without suffering a
watchdog timeout when destroying the domain. Therefore this is fairly
important for 4.6 (and backport), or we will have to retroactively drop
the currently-stated supported limits on the wiki.
The patch has had extensive testing in XenServer, although has been
forward ported from 4.5.
---
xen/arch/x86/mm/hap/hap.c | 22 ++++++++--------------
xen/arch/x86/mm/paging.c | 9 ++++++---
xen/arch/x86/mm/shadow/common.c | 24 +++++++++---------------
xen/include/asm-x86/hap.h | 2 +-
xen/include/asm-x86/shadow.h | 4 ++--
5 files changed, 26 insertions(+), 35 deletions(-)
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index d375c4d..e9c0080 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -551,7 +551,7 @@ void hap_final_teardown(struct domain *d)
}
if ( d->arch.paging.hap.total_pages != 0 )
- hap_teardown(d);
+ hap_teardown(d, NULL);
p2m_teardown(p2m_get_hostp2m(d));
/* Free any memory that the p2m teardown released */
@@ -561,7 +561,7 @@ void hap_final_teardown(struct domain *d)
paging_unlock(d);
}
-void hap_teardown(struct domain *d)
+void hap_teardown(struct domain *d, int *preempted)
{
struct vcpu *v;
mfn_t mfn;
@@ -589,18 +589,11 @@ void hap_teardown(struct domain *d)
if ( d->arch.paging.hap.total_pages != 0 )
{
- HAP_PRINTK("teardown of domain %u starts."
- " pages total = %u, free = %u, p2m=%u\n",
- d->domain_id,
- d->arch.paging.hap.total_pages,
- d->arch.paging.hap.free_pages,
- d->arch.paging.hap.p2m_pages);
- hap_set_allocation(d, 0, NULL);
- HAP_PRINTK("teardown done."
- " pages total = %u, free = %u, p2m=%u\n",
- d->arch.paging.hap.total_pages,
- d->arch.paging.hap.free_pages,
- d->arch.paging.hap.p2m_pages);
+ hap_set_allocation(d, 0, preempted);
+
+ if ( preempted && *preempted )
+ goto out;
+
ASSERT(d->arch.paging.hap.total_pages == 0);
}
@@ -609,6 +602,7 @@ void hap_teardown(struct domain *d)
xfree(d->arch.hvm_domain.dirty_vram);
d->arch.hvm_domain.dirty_vram = NULL;
+out:
paging_unlock(d);
}
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 618f475..5becee8 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -799,12 +799,15 @@ long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
/* Call when destroying a domain */
int paging_teardown(struct domain *d)
{
- int rc;
+ int rc, preempted = 0;
if ( hap_enabled(d) )
- hap_teardown(d);
+ hap_teardown(d, &preempted);
else
- shadow_teardown(d);
+ shadow_teardown(d, &preempted);
+
+ if ( preempted )
+ return -ERESTART;
/* clean up log dirty resources. */
rc = paging_free_log_dirty_bitmap(d, 0);
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index abce8e2..0264b91 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3071,7 +3071,7 @@ int shadow_enable(struct domain *d, u32 mode)
return rv;
}
-void shadow_teardown(struct domain *d)
+void shadow_teardown(struct domain *d, int *preempted)
/* Destroy the shadow pagetables of this domain and free its shadow memory.
* Should only be called for dying domains. */
{
@@ -3132,23 +3132,16 @@ void shadow_teardown(struct domain *d)
if ( d->arch.paging.shadow.total_pages != 0 )
{
- SHADOW_PRINTK("teardown of domain %u starts."
- " Shadow pages total = %u, free = %u, p2m=%u\n",
- d->domain_id,
- d->arch.paging.shadow.total_pages,
- d->arch.paging.shadow.free_pages,
- d->arch.paging.shadow.p2m_pages);
/* Destroy all the shadows and release memory to domheap */
- sh_set_allocation(d, 0, NULL);
+ sh_set_allocation(d, 0, preempted);
+
+ if ( preempted && *preempted )
+ goto out;
+
/* Release the hash table back to xenheap */
if (d->arch.paging.shadow.hash_table)
shadow_hash_teardown(d);
- /* Should not have any more memory held */
- SHADOW_PRINTK("teardown done."
- " Shadow pages total = %u, free = %u, p2m=%u\n",
- d->arch.paging.shadow.total_pages,
- d->arch.paging.shadow.free_pages,
- d->arch.paging.shadow.p2m_pages);
+
ASSERT(d->arch.paging.shadow.total_pages == 0);
}
@@ -3177,6 +3170,7 @@ void shadow_teardown(struct domain *d)
d->arch.hvm_domain.dirty_vram = NULL;
}
+out:
paging_unlock(d);
/* Must be called outside the lock */
@@ -3198,7 +3192,7 @@ void shadow_final_teardown(struct domain *d)
* It is possible for a domain that never got domain_kill()ed
* to get here with its shadow allocation intact. */
if ( d->arch.paging.shadow.total_pages != 0 )
- shadow_teardown(d);
+ shadow_teardown(d, NULL);
/* It is now safe to pull down the p2m map. */
p2m_teardown(p2m_get_hostp2m(d));
diff --git a/xen/include/asm-x86/hap.h b/xen/include/asm-x86/hap.h
index bd87481..c613836 100644
--- a/xen/include/asm-x86/hap.h
+++ b/xen/include/asm-x86/hap.h
@@ -38,7 +38,7 @@ int hap_domctl(struct domain *d, xen_domctl_shadow_op_t *sc,
XEN_GUEST_HANDLE_PARAM(void) u_domctl);
int hap_enable(struct domain *d, u32 mode);
void hap_final_teardown(struct domain *d);
-void hap_teardown(struct domain *d);
+void hap_teardown(struct domain *d, int *preempted);
void hap_vcpu_init(struct vcpu *v);
int hap_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
index 9cd653e..6d0aefb 100644
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -73,7 +73,7 @@ int shadow_domctl(struct domain *d,
XEN_GUEST_HANDLE_PARAM(void) u_domctl);
/* Call when destroying a domain */
-void shadow_teardown(struct domain *d);
+void shadow_teardown(struct domain *d, int *preempted);
/* Call once all of the references to the domain have gone away */
void shadow_final_teardown(struct domain *d);
@@ -85,7 +85,7 @@ int shadow_domctl(struct domain *d,
#else /* !CONFIG_SHADOW_PAGING */
-#define shadow_teardown(d) ASSERT(is_pv_domain(d))
+#define shadow_teardown(d, p) ASSERT(is_pv_domain(d))
#define shadow_final_teardown(d) ASSERT(is_pv_domain(d))
#define shadow_enable(d, mode) \
({ ASSERT(is_pv_domain(d)); -EOPNOTSUPP; })
--
1.7.10.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH for-4.6 v2] x86/mm: Make {hap, shadow}_teardown() preemptible
2015-08-05 15:47 [PATCH for-4.6 v2] x86/mm: Make {hap, shadow}_teardown() preemptible Andrew Cooper
@ 2015-08-05 16:17 ` Wei Liu
2015-08-05 17:00 ` George Dunlap
1 sibling, 0 replies; 4+ messages in thread
From: Wei Liu @ 2015-08-05 16:17 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Anshul Makkar, Wei Liu, Jan Beulich, Xen-devel
On Wed, Aug 05, 2015 at 04:47:59PM +0100, Andrew Cooper wrote:
> From: Anshul Makkar <anshul.makkar@citrix.com>
>
> A domain with sufficient shadow allocation can cause a watchdog timeout
> during domain destruction. Expand the existing -ERESTART logic in
> paging_teardown() to allow {hap/sh}_set_allocation() to become
> restartable during the DOMCTL_destroydomain hypercall.
>
> Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Tim Deegan <tim@xen.org>
> ---
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
>
> v2:
> * Drop {HAP,SHADOW}_PRINTK(). They are not very useful, and will get
> very noisy if enabled.
>
> Wei: Currently, Xen can't actually run 1TB guests without suffering a
> watchdog timeout when destroying the domain. Therefore this is fairly
> important for 4.6 (and backport), or we will have to retroactively drop
> the currently-stated supported limits on the wiki.
>
Right. This is a good reason to have it for 4.6.
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH for-4.6 v2] x86/mm: Make {hap, shadow}_teardown() preemptible
2015-08-05 15:47 [PATCH for-4.6 v2] x86/mm: Make {hap, shadow}_teardown() preemptible Andrew Cooper
2015-08-05 16:17 ` Wei Liu
@ 2015-08-05 17:00 ` George Dunlap
2015-08-06 9:28 ` Ian Campbell
1 sibling, 1 reply; 4+ messages in thread
From: George Dunlap @ 2015-08-05 17:00 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Anshul Makkar, Wei Liu, Jan Beulich, Xen-devel
On Wed, Aug 5, 2015 at 4:47 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> From: Anshul Makkar <anshul.makkar@citrix.com>
>
> A domain with sufficient shadow allocation can cause a watchdog timeout
> during domain destruction. Expand the existing -ERESTART logic in
> paging_teardown() to allow {hap/sh}_set_allocation() to become
> restartable during the DOMCTL_destroydomain hypercall.
>
> Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Tim Deegan <tim@xen.org>
Looks good, thanks:
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
>
> v2:
> * Drop {HAP,SHADOW}_PRINTK(). They are not very useful, and will get
> very noisy if enabled.
>
> Wei: Currently, Xen can't actually run 1TB guests without suffering a
> watchdog timeout when destroying the domain. Therefore this is fairly
> important for 4.6 (and backport), or we will have to retroactively drop
> the currently-stated supported limits on the wiki.
>
> The patch has had extensive testing in XenServer, although has been
> forward ported from 4.5.
> ---
> xen/arch/x86/mm/hap/hap.c | 22 ++++++++--------------
> xen/arch/x86/mm/paging.c | 9 ++++++---
> xen/arch/x86/mm/shadow/common.c | 24 +++++++++---------------
> xen/include/asm-x86/hap.h | 2 +-
> xen/include/asm-x86/shadow.h | 4 ++--
> 5 files changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index d375c4d..e9c0080 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -551,7 +551,7 @@ void hap_final_teardown(struct domain *d)
> }
>
> if ( d->arch.paging.hap.total_pages != 0 )
> - hap_teardown(d);
> + hap_teardown(d, NULL);
>
> p2m_teardown(p2m_get_hostp2m(d));
> /* Free any memory that the p2m teardown released */
> @@ -561,7 +561,7 @@ void hap_final_teardown(struct domain *d)
> paging_unlock(d);
> }
>
> -void hap_teardown(struct domain *d)
> +void hap_teardown(struct domain *d, int *preempted)
> {
> struct vcpu *v;
> mfn_t mfn;
> @@ -589,18 +589,11 @@ void hap_teardown(struct domain *d)
>
> if ( d->arch.paging.hap.total_pages != 0 )
> {
> - HAP_PRINTK("teardown of domain %u starts."
> - " pages total = %u, free = %u, p2m=%u\n",
> - d->domain_id,
> - d->arch.paging.hap.total_pages,
> - d->arch.paging.hap.free_pages,
> - d->arch.paging.hap.p2m_pages);
> - hap_set_allocation(d, 0, NULL);
> - HAP_PRINTK("teardown done."
> - " pages total = %u, free = %u, p2m=%u\n",
> - d->arch.paging.hap.total_pages,
> - d->arch.paging.hap.free_pages,
> - d->arch.paging.hap.p2m_pages);
> + hap_set_allocation(d, 0, preempted);
> +
> + if ( preempted && *preempted )
> + goto out;
> +
> ASSERT(d->arch.paging.hap.total_pages == 0);
> }
>
> @@ -609,6 +602,7 @@ void hap_teardown(struct domain *d)
> xfree(d->arch.hvm_domain.dirty_vram);
> d->arch.hvm_domain.dirty_vram = NULL;
>
> +out:
> paging_unlock(d);
> }
>
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index 618f475..5becee8 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -799,12 +799,15 @@ long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> /* Call when destroying a domain */
> int paging_teardown(struct domain *d)
> {
> - int rc;
> + int rc, preempted = 0;
>
> if ( hap_enabled(d) )
> - hap_teardown(d);
> + hap_teardown(d, &preempted);
> else
> - shadow_teardown(d);
> + shadow_teardown(d, &preempted);
> +
> + if ( preempted )
> + return -ERESTART;
>
> /* clean up log dirty resources. */
> rc = paging_free_log_dirty_bitmap(d, 0);
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index abce8e2..0264b91 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -3071,7 +3071,7 @@ int shadow_enable(struct domain *d, u32 mode)
> return rv;
> }
>
> -void shadow_teardown(struct domain *d)
> +void shadow_teardown(struct domain *d, int *preempted)
> /* Destroy the shadow pagetables of this domain and free its shadow memory.
> * Should only be called for dying domains. */
> {
> @@ -3132,23 +3132,16 @@ void shadow_teardown(struct domain *d)
>
> if ( d->arch.paging.shadow.total_pages != 0 )
> {
> - SHADOW_PRINTK("teardown of domain %u starts."
> - " Shadow pages total = %u, free = %u, p2m=%u\n",
> - d->domain_id,
> - d->arch.paging.shadow.total_pages,
> - d->arch.paging.shadow.free_pages,
> - d->arch.paging.shadow.p2m_pages);
> /* Destroy all the shadows and release memory to domheap */
> - sh_set_allocation(d, 0, NULL);
> + sh_set_allocation(d, 0, preempted);
> +
> + if ( preempted && *preempted )
> + goto out;
> +
> /* Release the hash table back to xenheap */
> if (d->arch.paging.shadow.hash_table)
> shadow_hash_teardown(d);
> - /* Should not have any more memory held */
> - SHADOW_PRINTK("teardown done."
> - " Shadow pages total = %u, free = %u, p2m=%u\n",
> - d->arch.paging.shadow.total_pages,
> - d->arch.paging.shadow.free_pages,
> - d->arch.paging.shadow.p2m_pages);
> +
> ASSERT(d->arch.paging.shadow.total_pages == 0);
> }
>
> @@ -3177,6 +3170,7 @@ void shadow_teardown(struct domain *d)
> d->arch.hvm_domain.dirty_vram = NULL;
> }
>
> +out:
> paging_unlock(d);
>
> /* Must be called outside the lock */
> @@ -3198,7 +3192,7 @@ void shadow_final_teardown(struct domain *d)
> * It is possible for a domain that never got domain_kill()ed
> * to get here with its shadow allocation intact. */
> if ( d->arch.paging.shadow.total_pages != 0 )
> - shadow_teardown(d);
> + shadow_teardown(d, NULL);
>
> /* It is now safe to pull down the p2m map. */
> p2m_teardown(p2m_get_hostp2m(d));
> diff --git a/xen/include/asm-x86/hap.h b/xen/include/asm-x86/hap.h
> index bd87481..c613836 100644
> --- a/xen/include/asm-x86/hap.h
> +++ b/xen/include/asm-x86/hap.h
> @@ -38,7 +38,7 @@ int hap_domctl(struct domain *d, xen_domctl_shadow_op_t *sc,
> XEN_GUEST_HANDLE_PARAM(void) u_domctl);
> int hap_enable(struct domain *d, u32 mode);
> void hap_final_teardown(struct domain *d);
> -void hap_teardown(struct domain *d);
> +void hap_teardown(struct domain *d, int *preempted);
> void hap_vcpu_init(struct vcpu *v);
> int hap_track_dirty_vram(struct domain *d,
> unsigned long begin_pfn,
> diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
> index 9cd653e..6d0aefb 100644
> --- a/xen/include/asm-x86/shadow.h
> +++ b/xen/include/asm-x86/shadow.h
> @@ -73,7 +73,7 @@ int shadow_domctl(struct domain *d,
> XEN_GUEST_HANDLE_PARAM(void) u_domctl);
>
> /* Call when destroying a domain */
> -void shadow_teardown(struct domain *d);
> +void shadow_teardown(struct domain *d, int *preempted);
>
> /* Call once all of the references to the domain have gone away */
> void shadow_final_teardown(struct domain *d);
> @@ -85,7 +85,7 @@ int shadow_domctl(struct domain *d,
>
> #else /* !CONFIG_SHADOW_PAGING */
>
> -#define shadow_teardown(d) ASSERT(is_pv_domain(d))
> +#define shadow_teardown(d, p) ASSERT(is_pv_domain(d))
> #define shadow_final_teardown(d) ASSERT(is_pv_domain(d))
> #define shadow_enable(d, mode) \
> ({ ASSERT(is_pv_domain(d)); -EOPNOTSUPP; })
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH for-4.6 v2] x86/mm: Make {hap, shadow}_teardown() preemptible
2015-08-05 17:00 ` George Dunlap
@ 2015-08-06 9:28 ` Ian Campbell
0 siblings, 0 replies; 4+ messages in thread
From: Ian Campbell @ 2015-08-06 9:28 UTC (permalink / raw)
To: George Dunlap, Andrew Cooper
Cc: Anshul Makkar, Wei Liu, Jan Beulich, Xen-devel
On Wed, 2015-08-05 at 18:00 +0100, George Dunlap wrote:
> On Wed, Aug 5, 2015 at 4:47 PM, Andrew Cooper <andrew.cooper3@citrix.com>
> wrote:
> > From: Anshul Makkar <anshul.makkar@citrix.com>
> >
> > A domain with sufficient shadow allocation can cause a watchdog timeout
> > during domain destruction. Expand the existing -ERESTART logic in
> > paging_teardown() to allow {hap/sh}_set_allocation() to become
> > restartable during the DOMCTL_destroydomain hypercall.
> >
> > Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com>
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Reviewed-by: Tim Deegan <tim@xen.org>
>
> Looks good, thanks:
>
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-08-06 9:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-05 15:47 [PATCH for-4.6 v2] x86/mm: Make {hap, shadow}_teardown() preemptible Andrew Cooper
2015-08-05 16:17 ` Wei Liu
2015-08-05 17:00 ` George Dunlap
2015-08-06 9:28 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.