[PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Brendan Cully]

# HG changeset patch
# User Brendan Cully <brendan@cs.ubc.ca>
# Date 1238518139 25200
# Node ID ed51e0ec7b8c08b1f191af7f89e53302f855f7d7
# Parent  4c7eb2e71e9d9d94583336c022042ffaa3ba6481
Skip vcpu_hotplug for VCPU 0 in smp_resume.
This function can occasionally take up to 2 seconds to complete,
and smp_suspend also skips VCPU 0.

Signed-off-by: Brendan Cully <brendan@cs.ubc.ca>

diff --git a/drivers/xen/core/cpu_hotplug.c b/drivers/xen/core/cpu_hotplug.c
--- a/drivers/xen/core/cpu_hotplug.c
+++ b/drivers/xen/core/cpu_hotplug.c
@@ -144,8 +144,11 @@
 {
 	unsigned int cpu;
 
-	for_each_possible_cpu(cpu)
+	for_each_possible_cpu(cpu) {
+		if (cpu == 0)
+			continue;
 		vcpu_hotplug(cpu);
+	}
 }
 
 int cpu_up_check(unsigned int cpu)

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Brendan Cully]

On Tuesday, 31 March 2009 at 09:53, Brendan Cully wrote:
> Skip vcpu_hotplug for VCPU 0 in smp_resume.
> This function can occasionally take up to 2 seconds to complete,
> and smp_suspend also skips VCPU 0.

I'm doing a bit of testing ahead of releasing Remus, and I've noticed
that there are a couple of places where resuming from suspension can
get stuck for a long time, which can cause Remus to think that the
domain has died. This is one of them.

The other is in the netfront accelerator. It tears down a xenstore
watch on the accelerator path at every suspend, and adds the watch
back on resume. As with any xenstore interaction, this can
occasionally take a very long time. I'm going from faulty memory here,
but I didn't think it was necessary to tear down and restore watches
across suspend. Would it make sense to move the watch remove and add
into the resume hook (taking it completely out of suspend and
suspend_cancel)?

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Keir Fraser]

On 31/03/2009 18:24, "Brendan Cully" <brendan@cs.ubc.ca> wrote:

> The other is in the netfront accelerator. It tears down a xenstore
> watch on the accelerator path at every suspend, and adds the watch
> back on resume. As with any xenstore interaction, this can
> occasionally take a very long time. I'm going from faulty memory here,
> but I didn't think it was necessary to tear down and restore watches
> across suspend. Would it make sense to move the watch remove and add
> into the resume hook (taking it completely out of suspend and
> suspend_cancel)?

They have to be renewed on full resume, but of course they do not get reset
across a cancelled suspend. Kieran may be able to advise on changing the net
accel code.

 -- Keir

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Brendan Cully]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On Tuesday, 31 March 2009 at 18:35, Keir Fraser wrote:
> On 31/03/2009 18:24, "Brendan Cully" <brendan@cs.ubc.ca> wrote:
> 
> > The other is in the netfront accelerator. It tears down a xenstore
> > watch on the accelerator path at every suspend, and adds the watch
> > back on resume. As with any xenstore interaction, this can
> > occasionally take a very long time. I'm going from faulty memory here,
> > but I didn't think it was necessary to tear down and restore watches
> > across suspend. Would it make sense to move the watch remove and add
> > into the resume hook (taking it completely out of suspend and
> > suspend_cancel)?
> 
> They have to be renewed on full resume, but of course they do not get reset
> across a cancelled suspend. Kieran may be able to advise on changing the net
> accel code.

Could it be as simple as this? I can't remember what happens if
unregister_xenbus_watch is called after the xenbus connection has been
reset. Should we just free the guest structures without interacting
with xenstore at the start of the resume method?

[-- Attachment #2: netfront-xs.patch --]
[-- Type: text/plain, Size: 2611 bytes --]

# HG changeset patch
# User Brendan Cully <brendan@cs.ubc.ca>
# Date 1238529727 25200
# Node ID 9db4499b35f048f4fac6323d8ca73de9779e6447
# Parent  ed51e0ec7b8c08b1f191af7f89e53302f855f7d7
Only rebuild netfront accelerator xenstore watch on resume.
The suspend_cancel function could take 2 seconds doing this.

diff --git a/drivers/xen/netfront/accel.c b/drivers/xen/netfront/accel.c
--- a/drivers/xen/netfront/accel.c
+++ b/drivers/xen/netfront/accel.c
@@ -678,8 +678,6 @@
 	unsigned long flags;
 	int rc = 0;
 	
-	netfront_accelerator_remove_watch(np);
-
 	mutex_lock(&accelerator_mutex);
 
  	/* Check that we've got a device that was accelerated */
@@ -701,20 +699,6 @@
 }
   
   
-int netfront_accelerator_suspend_cancel(struct netfront_info *np,
- 					struct xenbus_device *dev)
-{
-	/* 
-	 * Setting the watch will cause it to fire and probe the
-	 * accelerator, so no need to call accelerator_probe_new_vif()
-	 * directly here
-	 */
-	if (dev->state == XenbusStateConnected)
-		netfront_accelerator_add_watch(np);
-	return 0;
-}
- 
- 
 void netfront_accelerator_resume(struct netfront_info *np,
  				 struct xenbus_device *dev)
 {
@@ -722,6 +706,8 @@
  	spinlock_t *vif_states_lock;
  	unsigned long flags;
  
+	netfront_accelerator_remove_watch(np);
+
  	mutex_lock(&accelerator_mutex);
 
 	/* Check that we've got a device that was accelerated */
diff --git a/drivers/xen/netfront/netfront.c b/drivers/xen/netfront/netfront.c
--- a/drivers/xen/netfront/netfront.c
+++ b/drivers/xen/netfront/netfront.c
@@ -311,13 +311,6 @@
 }
 
 
-static int netfront_suspend_cancel(struct xenbus_device *dev)
-{
-	struct netfront_info *info = dev->dev.driver_data;
-	return netfront_accelerator_suspend_cancel(info, dev);
-}
-
-
 /**
  * We are reconnecting to the backend, due to a suspend/resume, or a backend
  * driver restart.  We tear down our netif structure and recreate it, but
@@ -2191,7 +2184,6 @@
 	.probe = netfront_probe,
 	.remove = __devexit_p(netfront_remove),
 	.suspend = netfront_suspend,
-	.suspend_cancel = netfront_suspend_cancel,
 	.resume = netfront_resume,
 	.otherend_changed = backend_changed,
 };
diff --git a/drivers/xen/netfront/netfront.h b/drivers/xen/netfront/netfront.h
--- a/drivers/xen/netfront/netfront.h
+++ b/drivers/xen/netfront/netfront.h
@@ -249,9 +249,6 @@
 int netfront_accelerator_suspend(struct netfront_info *np,
 				 struct xenbus_device *dev);
 extern
-int netfront_accelerator_suspend_cancel(struct netfront_info *np,
-					struct xenbus_device *dev);
-extern
 void netfront_accelerator_resume(struct netfront_info *np,
 				 struct xenbus_device *dev);
 extern

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Kieran Mansley]

On Tue, 2009-03-31 at 13:04 -0700, Brendan Cully wrote:
> On Tuesday, 31 March 2009 at 18:35, Keir Fraser wrote:
> > On 31/03/2009 18:24, "Brendan Cully" <brendan@cs.ubc.ca> wrote:
> > 
> > > The other is in the netfront accelerator. It tears down a xenstore
> > > watch on the accelerator path at every suspend, and adds the watch
> > > back on resume. As with any xenstore interaction, this can
> > > occasionally take a very long time. I'm going from faulty memory here,
> > > but I didn't think it was necessary to tear down and restore watches
> > > across suspend. Would it make sense to move the watch remove and add
> > > into the resume hook (taking it completely out of suspend and
> > > suspend_cancel)?
> > 
> > They have to be renewed on full resume, but of course they do not get reset
> > across a cancelled suspend. Kieran may be able to advise on changing the net
> > accel code.
> 
> Could it be as simple as this? I can't remember what happens if
> unregister_xenbus_watch is called after the xenbus connection has been
> reset. Should we just free the guest structures without interacting
> with xenstore at the start of the resume method?

I don't think that will work.  We need to remove the watch before the
suspend to ensure that the code that handles the watch will not be
activated during the migration.

The code that handles the watch may attempt to access hardware directly,
and during a migration this hardware might not be there any more.  The
approach the acceleration code takes is therefore to tear down the
accelerator's direct hardware access completely before the suspend, and
only reinstate it after the resume completes (on potentially new
hardware) or after the suspend is cancelled.

It may be possible to synchronise the watch handler with the
suspend/resume/cancel cycle without removing the watch, but that starts
to get complicated.

Kieran

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Keir Fraser]

On 01/04/2009 11:26, "Kieran Mansley" <kmansley@solarflare.com> wrote:

>> Could it be as simple as this? I can't remember what happens if
>> unregister_xenbus_watch is called after the xenbus connection has been
>> reset. Should we just free the guest structures without interacting
>> with xenstore at the start of the resume method?
> 
> It may be possible to synchronise the watch handler with the
> suspend/resume/cancel cycle without removing the watch, but that starts
> to get complicated.

Could we avoid any of this logic executing if there are no net accelerators?

 -- Keir

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Kieran Mansley]

On Wed, 2009-04-01 at 11:31 +0100, Keir Fraser wrote:
> On 01/04/2009 11:26, "Kieran Mansley" <kmansley@solarflare.com> wrote:
> 
> >> Could it be as simple as this? I can't remember what happens if
> >> unregister_xenbus_watch is called after the xenbus connection has been
> >> reset. Should we just free the guest structures without interacting
> >> with xenstore at the start of the resume method?
> > 
> > It may be possible to synchronise the watch handler with the
> > suspend/resume/cancel cycle without removing the watch, but that starts
> > to get complicated.
> 
> Could we avoid any of this logic executing if there are no net accelerators?

The watch handler will try to load an accelerator if the configuration
changes, so even if there were no accelerators before the suspend,
unless you can prevent the watch from firing, you could end up with one
trying to load between the suspend and resume.

If you got rid of the feature to load the requested accelerator
automatically when the configuration changes, then yes, that might be
possible, but I think I'd rather leave that in and use an extra lock and
some state to ignore the watch firing at bad times.  This would mean we
could leave the watch in place during the suspend/resume/cancel cycle
(refreshing on resume).  The suspend_cancel callback would still be
necessary, but it would just be acquiring a lock and modifying some
state rather than doing a xenbus watch operation.

It's not clear to me what the source of the long delay is, and whether
that change would solve it: the extra lock would be contended with the
watch handler's work queue, and so if the watch is the source of the
delay it's possible that we'd just contend in a different way and the
delay would still be there.  Brendan: can you explain the delay for me?

Kieran

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Brendan Cully]

On Wednesday, 01 April 2009 at 12:00, Kieran Mansley wrote:
> On Wed, 2009-04-01 at 11:31 +0100, Keir Fraser wrote:
> > On 01/04/2009 11:26, "Kieran Mansley" <kmansley@solarflare.com> wrote:
> > 
> > >> Could it be as simple as this? I can't remember what happens if
> > >> unregister_xenbus_watch is called after the xenbus connection has been
> > >> reset. Should we just free the guest structures without interacting
> > >> with xenstore at the start of the resume method?
> > > 
> > > It may be possible to synchronise the watch handler with the
> > > suspend/resume/cancel cycle without removing the watch, but that starts
> > > to get complicated.
> > 
> > Could we avoid any of this logic executing if there are no net accelerators?
> 
> The watch handler will try to load an accelerator if the configuration
> changes, so even if there were no accelerators before the suspend,
> unless you can prevent the watch from firing, you could end up with one
> trying to load between the suspend and resume.
> 
> If you got rid of the feature to load the requested accelerator
> automatically when the configuration changes, then yes, that might be
> possible, but I think I'd rather leave that in and use an extra lock and
> some state to ignore the watch firing at bad times.  This would mean we
> could leave the watch in place during the suspend/resume/cancel cycle
> (refreshing on resume).  The suspend_cancel callback would still be
> necessary, but it would just be acquiring a lock and modifying some
> state rather than doing a xenbus watch operation.

I'm afraid my memory of the kernel suspend mechanics has gotten a bit
rusty over the last few months, so I may be off-base here. But I
thought that xs_suspend masked watches until xs_reusume or
xs_suspend_cancel? If that is the case, isn't that on its own enough
to protect netfront?

> It's not clear to me what the source of the long delay is, and whether
> that change would solve it: the extra lock would be contended with the
> watch handler's work queue, and so if the watch is the source of the
> delay it's possible that we'd just contend in a different way and the
> delay would still be there.  Brendan: can you explain the delay for me?

It's the round trips to xenstored to manage the watches, which always
have a chance to be very slow.

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Ian Campbell]

On Wed, 2009-04-01 at 06:26 -0400, Kieran Mansley wrote:
> On Tue, 2009-03-31 at 13:04 -0700, Brendan Cully wrote:
> > On Tuesday, 31 March 2009 at 18:35, Keir Fraser wrote:
> > > On 31/03/2009 18:24, "Brendan Cully" <brendan@cs.ubc.ca> wrote:
> > > 
> > > > The other is in the netfront accelerator. It tears down a xenstore
> > > > watch on the accelerator path at every suspend, and adds the watch
> > > > back on resume. As with any xenstore interaction, this can
> > > > occasionally take a very long time. I'm going from faulty memory here,
> > > > but I didn't think it was necessary to tear down and restore watches
> > > > across suspend. Would it make sense to move the watch remove and add
> > > > into the resume hook (taking it completely out of suspend and
> > > > suspend_cancel)?
> > > 
> > > They have to be renewed on full resume, but of course they do not get reset
> > > across a cancelled suspend. Kieran may be able to advise on changing the net
> > > accel code.
> > 
> > Could it be as simple as this? I can't remember what happens if
> > unregister_xenbus_watch is called after the xenbus connection has been
> > reset. Should we just free the guest structures without interacting
> > with xenstore at the start of the resume method?
> 
> I don't think that will work.  We need to remove the watch before the
> suspend to ensure that the code that handles the watch will not be
> activated during the migration.

It's worth bearing in mind for if/when you port this stuff to a newer
pvops kernel that it tries to make much greater use of the Linux device
model callbacks rather than using xenbus specific suspend/resume
methods. The Linux device model doesn't have a suspend cancelled
callback in it, AFAIK.

Ian.

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Kieran Mansley]

On Wed, 2009-04-01 at 11:39 -0700, Brendan Cully wrote:
> I'm afraid my memory of the kernel suspend mechanics has gotten a bit
> rusty over the last few months, so I may be off-base here. But I
> thought that xs_suspend masked watches until xs_reusume or
> xs_suspend_cancel? If that is the case, isn't that on its own enough
> to protect netfront?

If that's the case, it should be enough, yes.

Do watches that would have fired during that time fire at the end of
masked period?  If not, we'd still have to access xenstore on suspend
cancel to check if we've missed a change.

Kieran

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Keir Fraser]

On 02/04/2009 09:13, "Kieran Mansley" <kmansley@solarflare.com> wrote:

> On Wed, 2009-04-01 at 11:39 -0700, Brendan Cully wrote:
>> I'm afraid my memory of the kernel suspend mechanics has gotten a bit
>> rusty over the last few months, so I may be off-base here. But I
>> thought that xs_suspend masked watches until xs_reusume or
>> xs_suspend_cancel? If that is the case, isn't that on its own enough
>> to protect netfront?
> 
> If that's the case, it should be enough, yes.
> 
> Do watches that would have fired during that time fire at the end of
> masked period?  If not, we'd still have to access xenstore on suspend
> cancel to check if we've missed a change.

I don't think watches get lost across suspend-cancel, just delayed.

 -- Keir

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Kieran Mansley]

On Thu, 2009-04-02 at 09:27 +0100, Keir Fraser wrote:
> On 02/04/2009 09:13, "Kieran Mansley" <kmansley@solarflare.com> wrote:
> 
> > On Wed, 2009-04-01 at 11:39 -0700, Brendan Cully wrote:
> >> I'm afraid my memory of the kernel suspend mechanics has gotten a bit
> >> rusty over the last few months, so I may be off-base here. But I
> >> thought that xs_suspend masked watches until xs_reusume or
> >> xs_suspend_cancel? If that is the case, isn't that on its own enough
> >> to protect netfront?
> > 
> > If that's the case, it should be enough, yes.
> > 
> > Do watches that would have fired during that time fire at the end of
> > masked period?  If not, we'd still have to access xenstore on suspend
> > cancel to check if we've missed a change.
> 
> I don't think watches get lost across suspend-cancel, just delayed.

OK, I'll try and produce a patch based on the assumptions that (i)
watches won't fire between suspend and either resume or suspend-cancel;
(ii) any that would have fired are triggered after a suspend-cancel; and
(iii) the watch must be refreshed on a resume (with no guarantees about
lost watches).

Kieran

Re: [PATCH] Skip vcpu_hotplug for VCPU 0 in smp_resume

[Kieran Mansley]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On Thu, 2009-04-02 at 10:13 +0100, Kieran Mansley wrote:
> OK, I'll try and produce a patch based on the assumptions that (i)
> watches won't fire between suspend and either resume or suspend-cancel;
> (ii) any that would have fired are triggered after a suspend-cancel; and
> (iii) the watch must be refreshed on a resume (with no guarantees about
> lost watches).

OK, here's the patch.

Signed-off-by: Kieran Mansley <kmansley@solarflare.com>

Keir: I'll leave it up to you as to whether this is a bug fix (and so
should be included in 3.4) or just queued for unstable after 3.4 is
released.  Either is fine with us.

Thanks

Kieran 

[-- Attachment #2: suspend_watch --]
[-- Type: text/plain, Size: 2698 bytes --]

Avoid removing watch across suspend/suspend_cancel cycle

diff -r b358ebf1c416 drivers/xen/netfront/accel.c
--- a/drivers/xen/netfront/accel.c
+++ b/drivers/xen/netfront/accel.c
@@ -50,6 +50,8 @@ static int netfront_load_accelerator(str
 static int netfront_load_accelerator(struct netfront_info *np, 
 				     struct xenbus_device *dev, 
 				     const char *frontend);
+
+static void netfront_accelerator_remove_watch(struct netfront_info *np);
 
 /*
  * List of all netfront accelerator plugin modules available.  Each
@@ -164,8 +166,11 @@ void netfront_accelerator_add_watch(stru
 {
 	int err;
 	
-	/* Check we're not trying to overwrite an existing watch */
-	BUG_ON(np->accel_vif_state.accel_watch.node != NULL);
+	/* 
+	 * If old watch exists, e.g. from before suspend/resume,
+	 * remove it now 
+	 */
+	netfront_accelerator_remove_watch(np);
 
 	/* Get a watch on the accelerator plugin */
 	err = xenbus_watch_path2(np->xbdev, np->xbdev->otherend, 
@@ -180,6 +185,19 @@ void netfront_accelerator_add_watch(stru
 }
 
 
+static void 
+netfront_accelerator_purge_watch(struct netfront_accel_vif_state *vif_state)
+{
+	flush_workqueue(accel_watch_workqueue);
+
+	/* Clean up any state left from watch */
+	if (vif_state->accel_frontend != NULL) {
+		kfree(vif_state->accel_frontend);
+		vif_state->accel_frontend = NULL;
+	}
+}
+
+
 static
 void netfront_accelerator_remove_watch(struct netfront_info *np)
 {
@@ -191,13 +209,7 @@ void netfront_accelerator_remove_watch(s
 		kfree(vif_state->accel_watch.node);
 		vif_state->accel_watch.node = NULL;
 
-		flush_workqueue(accel_watch_workqueue);
-
-		/* Clean up any state left from watch */
-		if (vif_state->accel_frontend != NULL) {
-			kfree(vif_state->accel_frontend);
-			vif_state->accel_frontend = NULL;
-		}
+		netfront_accelerator_purge_watch(vif_state);
 	}	
 }
 
@@ -670,8 +682,6 @@ int netfront_accelerator_suspend(struct 
 {
 	int rc = 0;
 	
-	netfront_accelerator_remove_watch(np);
-
 	mutex_lock(&accelerator_mutex);
 
  	/* Check that we've got a device that was accelerated */
@@ -692,13 +702,16 @@ int netfront_accelerator_suspend_cancel(
 int netfront_accelerator_suspend_cancel(struct netfront_info *np,
  					struct xenbus_device *dev)
 {
+	netfront_accelerator_purge_watch(&np->accel_vif_state);
+
 	/* 
-	 * Setting the watch will cause it to fire and probe the
-	 * accelerator, so no need to call accelerator_probe_new_vif()
-	 * directly here
+	 * Gratuitously fire the watch handler to reinstate the
+	 * configured accelerator
 	 */
 	if (dev->state == XenbusStateConnected)
-		netfront_accelerator_add_watch(np);
+		queue_work(accel_watch_workqueue, 
+			   &np->accel_vif_state.accel_work);
+
 	return 0;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel