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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@xxxxxxxxxx]
> Sent: 17 September 2013 16:45
> To: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel