Paul Durrant
2013-Sep-17 15:45 UTC
[PATCH v4] 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
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
Paul Durrant
2013-Sep-17 16:45 UTC
Re: [PATCH v4] Don''t destroy the netdev until the vif is shut down
> -----Original Message----- > From: Paul Durrant [mailto:paul.durrant@citrix.com] > Sent: 17 September 2013 16:45 > To: xen-devel@lists.xen.org; netdev@vger.kernel.org > Cc: Paul Durrant; David Vrabel; Wei Liu; Ian Campbell > Subject: [PATCH v4] Don''t destroy the netdev until the vif is shut down >Apologies for the loss of the net-next prefix here. I''ll re-send with the prefix. Paul> 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 > > 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