[PATCH net-next v4] Don't destroy the netdev until the vif is shut down

Subsystems: networking drivers, the rest, xen network backend driver

STALE4700d

9 messages, 4 authors, 2013-09-20 · open the first message on its own page

[PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: Paul Durrant <hidden>
Date: 2013-09-17 16:46:16

Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
Cc: David Vrabel <redacted>
Cc: Wei Liu <redacted>
Cc: Ian Campbell <redacted>
---
v2:
 - Modify netback_remove() - bug only seemed to manifest with linux guest

v3:
 - Move __module_get() and module_put() calls

v4:
 - Clear tx_irq in xenvif_disconnect() to make sure that a subsequent
   call to xenvif_connect() doesn't get nop-ed.

 drivers/net/xen-netback/common.h    |    1 +
 drivers/net/xen-netback/interface.c |   26 ++++++++++----------------
 drivers/net/xen-netback/xenbus.c    |   17 ++++++++++++-----
 3 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index a197743..5715318 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -184,6 +184,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 		   unsigned long rx_ring_ref, unsigned int tx_evtchn,
 		   unsigned int rx_evtchn);
 void xenvif_disconnect(struct xenvif *vif);
+void xenvif_free(struct xenvif *vif);
 
 int xenvif_xenbus_init(void);
 void xenvif_xenbus_fini(void);
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 625c6f4..0465e0f 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -353,6 +353,9 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	}
 
 	netdev_dbg(dev, "Successfully created xenvif\n");
+
+	__module_get(THIS_MODULE);
+
 	return vif;
 }
 
@@ -366,8 +369,6 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 	if (vif->tx_irq)
 		return 0;
 
-	__module_get(THIS_MODULE);
-
 	err = xenvif_map_frontend_rings(vif, tx_ring_ref, rx_ring_ref);
 	if (err < 0)
 		goto err;
@@ -452,12 +453,6 @@ void xenvif_carrier_off(struct xenvif *vif)
 
 void xenvif_disconnect(struct xenvif *vif)
 {
-	/* Disconnect funtion might get called by generic framework
-	 * even before vif connects, so we need to check if we really
-	 * need to do a module_put.
-	 */
-	int need_module_put = 0;
-
 	if (netif_carrier_ok(vif->dev))
 		xenvif_carrier_off(vif);
 
@@ -468,23 +463,22 @@ void xenvif_disconnect(struct xenvif *vif)
 			unbind_from_irqhandler(vif->tx_irq, vif);
 			unbind_from_irqhandler(vif->rx_irq, vif);
 		}
-		/* vif->irq is valid, we had a module_get in
-		 * xenvif_connect.
-		 */
-		need_module_put = 1;
+		vif->tx_irq = 0;
 	}
 
 	if (vif->task)
 		kthread_stop(vif->task);
 
+	xenvif_unmap_frontend_rings(vif);
+}
+
+void xenvif_free(struct xenvif *vif)
+{
 	netif_napi_del(&vif->napi);
 
 	unregister_netdev(vif->dev);
 
-	xenvif_unmap_frontend_rings(vif);
-
 	free_netdev(vif->dev);
 
-	if (need_module_put)
-		module_put(THIS_MODULE);
+	module_put(THIS_MODULE);
 }
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 1fe48fe3..a53782e 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -42,7 +42,7 @@ static int netback_remove(struct xenbus_device *dev)
 	if (be->vif) {
 		kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
 		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
-		xenvif_disconnect(be->vif);
+		xenvif_free(be->vif);
 		be->vif = NULL;
 	}
 	kfree(be);
@@ -213,9 +213,18 @@ static void disconnect_backend(struct xenbus_device *dev)
 {
 	struct backend_info *be = dev_get_drvdata(&dev->dev);
 
+	if (be->vif)
+		xenvif_disconnect(be->vif);
+}
+
+static void destroy_backend(struct xenbus_device *dev)
+{
+	struct backend_info *be = dev_get_drvdata(&dev->dev);
+
 	if (be->vif) {
+		kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
 		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
-		xenvif_disconnect(be->vif);
+		xenvif_free(be->vif);
 		be->vif = NULL;
 	}
 }
@@ -246,14 +255,11 @@ static void frontend_changed(struct xenbus_device *dev,
 	case XenbusStateConnected:
 		if (dev->state == XenbusStateConnected)
 			break;
-		backend_create_xenvif(be);
 		if (be->vif)
 			connect(be);
 		break;
 
 	case XenbusStateClosing:
-		if (be->vif)
-			kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
 		disconnect_backend(dev);
 		xenbus_switch_state(dev, XenbusStateClosing);
 		break;
@@ -262,6 +268,7 @@ static void frontend_changed(struct xenbus_device *dev,
 		xenbus_switch_state(dev, XenbusStateClosed);
 		if (xenbus_dev_is_online(dev))
 			break;
+		destroy_backend(dev);
 		/* fall through if not online */
 	case XenbusStateUnknown:
 		device_unregister(&dev->dev);
-- 
1.7.10.4

Re: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: David Miller <davem@davemloft.net>
Date: 2013-09-18 00:31:04

From: Paul Durrant <redacted>
Date: Tue, 17 Sep 2013 17:46:08 +0100
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
Can a xen driver expert please review and give an Acked-by:?

Thanks.

Re: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: Wei Liu <hidden>
Date: 2013-09-18 10:37:07

On Tue, Sep 17, 2013 at 05:46:08PM +0100, Paul Durrant wrote:
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
Cc: David Vrabel <redacted>
Cc: Wei Liu <redacted>
Cc: Ian Campbell <redacted>
Acked-by: Wei Liu <redacted>

Thanks!
Wei.

Re: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: Ian Campbell <hidden>
Date: 2013-09-18 15:58:20

On Wed, 2013-09-18 at 11:37 +0100, Wei Liu wrote:
On Tue, Sep 17, 2013 at 05:46:08PM +0100, Paul Durrant wrote:
quoted
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
Cc: David Vrabel <redacted>
Cc: Wei Liu <redacted>
Cc: Ian Campbell <redacted>
Acked-by: Wei Liu <redacted>
yeah, looks good, thanks.

Paul, did you test this with non-Windows frontends too? and do things
like vif hot(un)plug still work?

Ian.

RE: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: Paul Durrant <hidden>
Date: 2013-09-18 16:06:18

-----Original Message-----
From: Ian Campbell [mailto:ian.campbell@citrix.com]
Sent: 18 September 2013 16:58
To: Wei Liu
Cc: Paul Durrant; xen-devel@lists.xen.org; netdev@vger.kernel.org; David
Vrabel
Subject: Re: [PATCH net-next v4] Don't destroy the netdev until the vif is
shut down

On Wed, 2013-09-18 at 11:37 +0100, Wei Liu wrote:
quoted
On Tue, Sep 17, 2013 at 05:46:08PM +0100, Paul Durrant wrote:
quoted
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
Cc: David Vrabel <redacted>
Cc: Wei Liu <redacted>
Cc: Ian Campbell <redacted>
Acked-by: Wei Liu <redacted>
yeah, looks good, thanks.

Paul, did you test this with non-Windows frontends too? and do things
like vif hot(un)plug still work?
Ian,

I tested with a debian (wheezy) PV guest but I didn't test unplug. I cycled a windows frontend several times (which is how I spotted the tx_irq thing), and shutdown and brought up both the debian and windows guests several times. I can test unplug too if you'd like.

  Paul

Re: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: Ian Campbell <hidden>
Date: 2013-09-18 16:25:20

On Wed, 2013-09-18 at 17:04 +0100, Paul Durrant wrote:
quoted
-----Original Message-----
From: Ian Campbell [mailto:ian.campbell@citrix.com]
Sent: 18 September 2013 16:58
To: Wei Liu
Cc: Paul Durrant; xen-devel@lists.xen.org; netdev@vger.kernel.org; David
Vrabel
Subject: Re: [PATCH net-next v4] Don't destroy the netdev until the vif is
shut down

On Wed, 2013-09-18 at 11:37 +0100, Wei Liu wrote:
quoted
On Tue, Sep 17, 2013 at 05:46:08PM +0100, Paul Durrant wrote:
quoted
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
Cc: David Vrabel <redacted>
Cc: Wei Liu <redacted>
Cc: Ian Campbell <redacted>
Acked-by: Wei Liu <redacted>
yeah, looks good, thanks.

Paul, did you test this with non-Windows frontends too? and do things
like vif hot(un)plug still work?
Ian,

I tested with a debian (wheezy) PV guest but I didn't test unplug. I
cycled a windows frontend several times (which is how I spotted the
tx_irq thing), and shutdown and brought up both the debian and windows
guests several times. I can test unplug too if you'd like.
I don't think it needs to be a blocker for accepting this patch but it
would be good to try it, it's the sort of area which historically gets
broken by this sort of change.

Ian.
  Paul

RE: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: Paul Durrant <hidden>
Date: 2013-09-19 11:55:40

-----Original Message-----
From: Ian Campbell [mailto:ian.campbell@citrix.com]
Sent: 18 September 2013 17:25
To: Paul Durrant
Cc: Wei Liu; xen-devel@lists.xen.org; netdev@vger.kernel.org; David Vrabel
Subject: Re: [PATCH net-next v4] Don't destroy the netdev until the vif is
shut down

On Wed, 2013-09-18 at 17:04 +0100, Paul Durrant wrote:
quoted
quoted
-----Original Message-----
From: Ian Campbell [mailto:ian.campbell@citrix.com]
Sent: 18 September 2013 16:58
To: Wei Liu
Cc: Paul Durrant; xen-devel@lists.xen.org; netdev@vger.kernel.org;
David
quoted
quoted
Vrabel
Subject: Re: [PATCH net-next v4] Don't destroy the netdev until the vif is
shut down

On Wed, 2013-09-18 at 11:37 +0100, Wei Liu wrote:
quoted
On Tue, Sep 17, 2013 at 05:46:08PM +0100, Paul Durrant wrote:
quoted
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
Cc: David Vrabel <redacted>
Cc: Wei Liu <redacted>
Cc: Ian Campbell <redacted>
Acked-by: Wei Liu <redacted>
yeah, looks good, thanks.

Paul, did you test this with non-Windows frontends too? and do things
like vif hot(un)plug still work?
Ian,

I tested with a debian (wheezy) PV guest but I didn't test unplug. I
cycled a windows frontend several times (which is how I spotted the
tx_irq thing), and shutdown and brought up both the debian and windows
guests several times. I can test unplug too if you'd like.
I don't think it needs to be a blocker for accepting this patch but it
would be good to try it, it's the sort of area which historically gets
broken by this sort of change.
FWIW I just checked network-detach with a windows frontend and it worked ok.

  Paul

Re: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: David Miller <davem@davemloft.net>
Date: 2013-09-19 18:05:15

From: Paul Durrant <redacted>
Date: Tue, 17 Sep 2013 17:46:08 +0100
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
I think all of the testing requests have been satisfied so I have
applied this.

But _please_ write a more proper subject line in future patch
submissions.  You have to provide a subsystem prefix in the
subject line, here I added "xen-netback: ", otherwise someone
skimming the shortlog has no idea what area of code this change
touches.

Thanks.

RE: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down

From: Paul Durrant <hidden>
Date: 2013-09-20 08:10:27

-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: 19 September 2013 19:05
To: Paul Durrant
Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; David Vrabel; Wei Liu;
Ian Campbell
Subject: Re: [PATCH net-next v4] Don't destroy the netdev until the vif is
shut down

From: Paul Durrant <redacted>
Date: Tue, 17 Sep 2013 17:46:08 +0100
quoted
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.

With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.

Signed-off-by: Paul Durrant <redacted>
I think all of the testing requests have been satisfied so I have
applied this.

But _please_ write a more proper subject line in future patch
submissions.  You have to provide a subsystem prefix in the
subject line, here I added "xen-netback: ", otherwise someone
skimming the shortlog has no idea what area of code this change
touches.
Ok. Will do. Thanks,

  Paul
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help