thr3ads.net - Xen devel - [PATCH v2] Don''t destroy the netdev until the vif is shut down [Sep 2013]

If this information is useful, please help other people find it:
Share via:

Paul Durrant

2013-Sep-12 12:14 UTC

[PATCH v2] Don''t destroy the netdev until the vif is shut down

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 <paul.durrant@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
v2:
 - Modify netback_remove() - bug only seemed to manifest with linux guest

 drivers/net/xen-netback/common.h    |    1 +
 drivers/net/xen-netback/interface.c |   13 ++++++++-----
 drivers/net/xen-netback/xenbus.c    |   17 ++++++++++++-----
 3 files changed, 21 insertions(+), 10 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..65e78f9 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -477,14 +477,17 @@ void xenvif_disconnect(struct xenvif *vif)
 	if (vif->task)
 		kthread_stop(vif->task);
 
+	xenvif_unmap_frontend_rings(vif);
+
+	if (need_module_put)
+		module_put(THIS_MODULE);
+}
+
+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);
 }
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

Wei Liu

2013-Sep-12 15:21 UTC

head link

Re: [PATCH v2] Don''t destroy the netdev until the vif is shut down

The title should start with "[PATCH net-next]".

On Thu, Sep 12, 2013 at 01:14:52PM +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 <paul.durrant@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2:
>  - Modify netback_remove() - bug only seemed to manifest with linux guest
> 
>  drivers/net/xen-netback/common.h    |    1 +
>  drivers/net/xen-netback/interface.c |   13 ++++++++-----
>  drivers/net/xen-netback/xenbus.c    |   17 ++++++++++++-----
>  3 files changed, 21 insertions(+), 10 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);
>  
This can be moved a few lines above to group with xenvif_alloc() IMHO.
>  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..65e78f9 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -477,14 +477,17 @@ void xenvif_disconnect(struct xenvif *vif)
>  	if (vif->task)
>  		kthread_stop(vif->task);
>  
> +	xenvif_unmap_frontend_rings(vif);
> +
> +	if (need_module_put)
> +		module_put(THIS_MODULE);
> +}
> +
The original thought for this module_get/put thing is that admin can
just disconnect all frontends then replace netback module. With this
patch that doesn''t work anymore. We leak memory when all frontends are
disconnected and admin runs "modprobe -r xen-netback".  That happened
to
work before ecause everytime a frontend is disconnected the netdev structure
is already freed.

The ability to unload netback is still quite handy for developer / admin
so I would like to keep it. My suggestion would be, move module_get/put
to the point where netdev is allocated / freed. The impact of moving
module_get/put is that we cannot just simply disconnect all frontends
then replace netback, we need to actually shutdown / migrate all VMs
before replacing netback.

Wei.

Paul Durrant

2013-Sep-12 15:30 UTC

head link

Re: [PATCH v2] Don''t destroy the netdev until the vif is shut down

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 12 September 2013 16:21
> To: Paul Durrant
> Cc: netdev@vger.kernel.org; xen-devel@lists.xen.org; David Vrabel; Wei Liu;
> Ian Campbell
> Subject: Re: [PATCH v2] Don''t destroy the netdev until the vif is
shut down
> 
> The title should start with "[PATCH net-next]".
> 
Ok.
> On Thu, Sep 12, 2013 at 01:14:52PM +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 <paul.durrant@citrix.com>
> > Cc: David Vrabel <david.vrabel@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > v2:
> >  - Modify netback_remove() - bug only seemed to manifest with linux
> guest
> >
> >  drivers/net/xen-netback/common.h    |    1 +
> >  drivers/net/xen-netback/interface.c |   13 ++++++++-----
> >  drivers/net/xen-netback/xenbus.c    |   17 ++++++++++++-----
> >  3 files changed, 21 insertions(+), 10 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);
> >
> 
> This can be moved a few lines above to group with xenvif_alloc() IMHO.
> 
A matter of personal taste I think.
> >  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..65e78f9 100644
> > --- a/drivers/net/xen-netback/interface.c
> > +++ b/drivers/net/xen-netback/interface.c
> > @@ -477,14 +477,17 @@ void xenvif_disconnect(struct xenvif *vif)
> >  	if (vif->task)
> >  		kthread_stop(vif->task);
> >
> > +	xenvif_unmap_frontend_rings(vif);
> > +
> > +	if (need_module_put)
> > +		module_put(THIS_MODULE);
> > +}
> > +
> 
> The original thought for this module_get/put thing is that admin can
> just disconnect all frontends then replace netback module. With this
> patch that doesn''t work anymore. We leak memory when all frontends
are
> disconnected and admin runs "modprobe -r xen-netback".  That
happened
> to
> work before ecause everytime a frontend is disconnected the netdev
> structure
> is already freed.
> 
> The ability to unload netback is still quite handy for developer / admin
> so I would like to keep it. My suggestion would be, move module_get/put
> to the point where netdev is allocated / freed. The impact of moving
> module_get/put is that we cannot just simply disconnect all frontends
> then replace netback, we need to actually shutdown / migrate all VMs
> before replacing netback.
> 
Presumably you still had the problem of needing to manually re-run the hotplug
scripts if not using udev though. I''ll move the module_get/put as you
suggest.

  Paul

Xen devel - Sep 2013 - [PATCH v2] Don't destroy the netdev until the vif is shut down

[PATCH v2] Don''t destroy the netdev until the vif is shut down

Re: [PATCH v2] Don''t destroy the netdev until the vif is shut down

Re: [PATCH v2] Don''t destroy the netdev until the vif is shut down