Patchwork [PATCHv3] net: delay freeing peer host device

login
register
mail settings
Submitter Michael S. Tsirkin
Date Oct. 4, 2010, 4:13 p.m.
Message ID <20101004161329.GA5925@redhat.com>
Download mbox | patch
Permalink /patch/66711/
State New
Headers show

Comments

Michael S. Tsirkin - Oct. 4, 2010, 4:13 p.m.
With -netdev, virtio devices present offload
features to guest, depending on the backend used.
Thus, removing host netdev peer while guest is
active leads to guest-visible inconsistency and/or crashes.

As a solution, while guest (NIC) peer device exists,
we prevent the host peer pointer from being deleted.
This patch does this by adding peer_deleted flag in nic state:
if host device is going away while guest device
is around, set this flag and keep a shell of
the host device around for as long as guest device exists.

The link is put down so all packets will get discarded.

Note: this does not fix the fact that hotplug is asynchronous and
depends on the guest.  At the moment, management can detect that pci
device deletion is delayed by doing e.g. info net. As a next step, we
shall add commands that control hotplug/unplug without removing the
device, an event to report that guest has responded to the hotplug
event, and a command to cause instant device removal.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

So the below has survived some stress-testing, I'll put
this on my tree unless there are any objections.

Changes from v2:
  fix crash on repeated netdev_del

 net.c |   50 ++++++++++++++++++++++++++++++++++++++++----------
 net.h |    1 +
 2 files changed, 41 insertions(+), 10 deletions(-)
Alex Williamson - Oct. 6, 2010, 3:48 p.m.
On Mon, 2010-10-04 at 18:13 +0200, Michael S. Tsirkin wrote:
> With -netdev, virtio devices present offload
> features to guest, depending on the backend used.
> Thus, removing host netdev peer while guest is
> active leads to guest-visible inconsistency and/or crashes.
> 
> As a solution, while guest (NIC) peer device exists,
> we prevent the host peer pointer from being deleted.
> This patch does this by adding peer_deleted flag in nic state:
> if host device is going away while guest device
> is around, set this flag and keep a shell of
> the host device around for as long as guest device exists.
> 
> The link is put down so all packets will get discarded.
> 
> Note: this does not fix the fact that hotplug is asynchronous and
> depends on the guest.  At the moment, management can detect that pci
> device deletion is delayed by doing e.g. info net. As a next step, we
> shall add commands that control hotplug/unplug without removing the
> device, an event to report that guest has responded to the hotplug
> event, and a command to cause instant device removal.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> So the below has survived some stress-testing, I'll put
> this on my tree unless there are any objections.
> 
> Changes from v2:
>   fix crash on repeated netdev_del
> 
>  net.c |   50 ++++++++++++++++++++++++++++++++++++++++----------
>  net.h |    1 +
>  2 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/net.c b/net.c
> index 3d0fde7..032f09e 100644
> --- a/net.c
> +++ b/net.c
> @@ -281,29 +281,59 @@ NICState *qemu_new_nic(NetClientInfo *info,
>      return nic;
>  }
>  
> -void qemu_del_vlan_client(VLANClientState *vc)
> +static void qemu_free_vlan_client(VLANClientState *vc)
>  {
> -    if (vc->vlan) {
> -        QTAILQ_REMOVE(&vc->vlan->clients, vc, next);
> -    } else {
> +    if (!vc->vlan) {
>          if (vc->send_queue) {
>              qemu_del_net_queue(vc->send_queue);
>          }
> -        QTAILQ_REMOVE(&non_vlan_clients, vc, next);
>          if (vc->peer) {
>              vc->peer->peer = NULL;
>          }
>      }
> -
> -    if (vc->info->cleanup) {
> -        vc->info->cleanup(vc);
> -    }
> -
>      qemu_free(vc->name);
>      qemu_free(vc->model);
>      qemu_free(vc);
>  }
>  
> +void qemu_del_vlan_client(VLANClientState *vc)
> +{
> +    /* If there is a peer NIC, delete and cleanup client, but do not free. */
> +    if (!vc->vlan && vc->peer && vc->peer->info->type == NET_CLIENT_TYPE_NIC) {
> +        NICState *nic = DO_UPCAST(NICState, nc, vc->peer);
> +        if (nic->peer_deleted) {
> +            return;
> +        }
> +        nic->peer_deleted = true;
> +        /* Let NIC know peer is gone. */
> +        vc->peer->link_down = true;
> +        if (vc->peer->info->link_status_changed) {
> +            vc->peer->info->link_status_changed(vc->peer);
> +        }
> +        if (vc->info->cleanup) {
> +            vc->info->cleanup(vc);
> +        }

This is now the only case that calls cleanup, is that intentional?
Seems like we won't end up calling nic cleanup routines.

Alex

> +        QTAILQ_REMOVE(&non_vlan_clients, vc, next);
> +        return;
> +    }
> +
> +    /* If this is a peer NIC and peer has already been deleted, free it now. */
> +    if (!vc->vlan && vc->peer && vc->info->type == NET_CLIENT_TYPE_NIC) {
> +        NICState *nic = DO_UPCAST(NICState, nc, vc);
> +        if (nic->peer_deleted) {
> +            qemu_free_vlan_client(vc->peer);
> +        }
> +    }
> +
> +    if (vc->vlan) {
> +        QTAILQ_REMOVE(&vc->vlan->clients, vc, next);
> +    } else {
> +        QTAILQ_REMOVE(&non_vlan_clients, vc, next);
> +    }
> +
> +    qemu_free_vlan_client(vc);
> +}
> +
>  VLANClientState *
>  qemu_find_vlan_client_by_name(Monitor *mon, int vlan_id,
>                                const char *client_str)
> diff --git a/net.h b/net.h
> index 518cf9c..44c31a9 100644
> --- a/net.h
> +++ b/net.h
> @@ -72,6 +72,7 @@ typedef struct NICState {
>      VLANClientState nc;
>      NICConf *conf;
>      void *opaque;
> +    bool peer_deleted;
>  } NICState;
>  
>  struct VLANState {
Michael S. Tsirkin - Oct. 6, 2010, 4:01 p.m.
On Wed, Oct 06, 2010 at 09:48:06AM -0600, Alex Williamson wrote:
> > +void qemu_del_vlan_client(VLANClientState *vc)
> > +{
> > +    /* If there is a peer NIC, delete and cleanup client, but do not free. */
> > +    if (!vc->vlan && vc->peer && vc->peer->info->type == NET_CLIENT_TYPE_NIC) {
> > +        NICState *nic = DO_UPCAST(NICState, nc, vc->peer);
> > +        if (nic->peer_deleted) {
> > +            return;
> > +        }
> > +        nic->peer_deleted = true;
> > +        /* Let NIC know peer is gone. */
> > +        vc->peer->link_down = true;
> > +        if (vc->peer->info->link_status_changed) {
> > +            vc->peer->info->link_status_changed(vc->peer);
> > +        }
> > +        if (vc->info->cleanup) {
> > +            vc->info->cleanup(vc);
> > +        }
> 
> This is now the only case that calls cleanup, is that intentional?
> Seems like we won't end up calling nic cleanup routines.
> 
> Alex


Good catch.
I'll fix it up, thanks!

Patch

diff --git a/net.c b/net.c
index 3d0fde7..032f09e 100644
--- a/net.c
+++ b/net.c
@@ -281,29 +281,59 @@  NICState *qemu_new_nic(NetClientInfo *info,
     return nic;
 }
 
-void qemu_del_vlan_client(VLANClientState *vc)
+static void qemu_free_vlan_client(VLANClientState *vc)
 {
-    if (vc->vlan) {
-        QTAILQ_REMOVE(&vc->vlan->clients, vc, next);
-    } else {
+    if (!vc->vlan) {
         if (vc->send_queue) {
             qemu_del_net_queue(vc->send_queue);
         }
-        QTAILQ_REMOVE(&non_vlan_clients, vc, next);
         if (vc->peer) {
             vc->peer->peer = NULL;
         }
     }
-
-    if (vc->info->cleanup) {
-        vc->info->cleanup(vc);
-    }
-
     qemu_free(vc->name);
     qemu_free(vc->model);
     qemu_free(vc);
 }
 
+void qemu_del_vlan_client(VLANClientState *vc)
+{
+    /* If there is a peer NIC, delete and cleanup client, but do not free. */
+    if (!vc->vlan && vc->peer && vc->peer->info->type == NET_CLIENT_TYPE_NIC) {
+        NICState *nic = DO_UPCAST(NICState, nc, vc->peer);
+        if (nic->peer_deleted) {
+            return;
+        }
+        nic->peer_deleted = true;
+        /* Let NIC know peer is gone. */
+        vc->peer->link_down = true;
+        if (vc->peer->info->link_status_changed) {
+            vc->peer->info->link_status_changed(vc->peer);
+        }
+        if (vc->info->cleanup) {
+            vc->info->cleanup(vc);
+        }
+        QTAILQ_REMOVE(&non_vlan_clients, vc, next);
+        return;
+    }
+
+    /* If this is a peer NIC and peer has already been deleted, free it now. */
+    if (!vc->vlan && vc->peer && vc->info->type == NET_CLIENT_TYPE_NIC) {
+        NICState *nic = DO_UPCAST(NICState, nc, vc);
+        if (nic->peer_deleted) {
+            qemu_free_vlan_client(vc->peer);
+        }
+    }
+
+    if (vc->vlan) {
+        QTAILQ_REMOVE(&vc->vlan->clients, vc, next);
+    } else {
+        QTAILQ_REMOVE(&non_vlan_clients, vc, next);
+    }
+
+    qemu_free_vlan_client(vc);
+}
+
 VLANClientState *
 qemu_find_vlan_client_by_name(Monitor *mon, int vlan_id,
                               const char *client_str)
diff --git a/net.h b/net.h
index 518cf9c..44c31a9 100644
--- a/net.h
+++ b/net.h
@@ -72,6 +72,7 @@  typedef struct NICState {
     VLANClientState nc;
     NICConf *conf;
     void *opaque;
+    bool peer_deleted;
 } NICState;
 
 struct VLANState {