* [Qemu-devel] [PATCH] net: delay freeing peer host device
@ 2010-09-27 13:00 Michael S. Tsirkin
2010-09-27 20:18 ` [Qemu-devel] " Alex Williamson
0 siblings, 1 reply; 3+ messages in thread
From: Michael S. Tsirkin @ 2010-09-27 13:00 UTC (permalink / raw)
To: qemu-devel, Anthony Liguori, Alex Williamson
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 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.
At the moment, management can detect that device deletion
is delayed by doing info net. As a next step, we shall add
commands that control hotplug/unplug without
removing the device, and an event to report that
guest has responded to the hotplug event.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net.c | 40 +++++++++++++++++++++++++++++++++-------
net.h | 1 +
2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/net.c b/net.c
index 3d0fde7..edb87af 100644
--- a/net.c
+++ b/net.c
@@ -281,29 +281,55 @@ 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 {
+ QTAILQ_REMOVE(&non_vlan_clients, vc, next);
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);
+ }
+ 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);
+ }
+ }
+
+ 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 {
--
1.7.3-rc1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Qemu-devel] Re: [PATCH] net: delay freeing peer host device
2010-09-27 13:00 [Qemu-devel] [PATCH] net: delay freeing peer host device Michael S. Tsirkin
@ 2010-09-27 20:18 ` Alex Williamson
2010-09-27 21:53 ` Michael S. Tsirkin
0 siblings, 1 reply; 3+ messages in thread
From: Alex Williamson @ 2010-09-27 20:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Mon, 2010-09-27 at 15:00 +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 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.
>
> At the moment, management can detect that device deletion
> is delayed by doing info net. As a next step, we shall add
> commands that control hotplug/unplug without
> removing the device, and an event to report that
> guest has responded to the hotplug event.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> net.c | 40 +++++++++++++++++++++++++++++++++-------
> net.h | 1 +
> 2 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/net.c b/net.c
> index 3d0fde7..edb87af 100644
> --- a/net.c
> +++ b/net.c
> @@ -281,29 +281,55 @@ 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 {
> + QTAILQ_REMOVE(&non_vlan_clients, vc, next);
> 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);
Are we potentially introducing more races here? Now we have the tap
trying to call link change on a peer device that's potentially in the
middle of going away. Thanks,
Alex
> + }
> + if (vc->info->cleanup) {
> + vc->info->cleanup(vc);
> + }
> + 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);
> + }
> + }
> +
> + 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 {
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Qemu-devel] Re: [PATCH] net: delay freeing peer host device
2010-09-27 20:18 ` [Qemu-devel] " Alex Williamson
@ 2010-09-27 21:53 ` Michael S. Tsirkin
0 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2010-09-27 21:53 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel
On Mon, Sep 27, 2010 at 02:18:37PM -0600, Alex Williamson wrote:
> On Mon, 2010-09-27 at 15:00 +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 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.
> >
> > At the moment, management can detect that device deletion
> > is delayed by doing info net. As a next step, we shall add
> > commands that control hotplug/unplug without
> > removing the device, and an event to report that
> > guest has responded to the hotplug event.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > net.c | 40 +++++++++++++++++++++++++++++++++-------
> > net.h | 1 +
> > 2 files changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/net.c b/net.c
> > index 3d0fde7..edb87af 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -281,29 +281,55 @@ 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 {
> > + QTAILQ_REMOVE(&non_vlan_clients, vc, next);
> > 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);
>
> Are we potentially introducing more races here? Now we have the tap
> trying to call link change on a peer device that's potentially in the
> middle of going away. Thanks,
>
> Alex
You mean in guest?
This could always happen anyway: user can call link state change
command. So drivers must be ready for this.
There's no race in qemu because everything is singlethreaded.
> > + }
> > + if (vc->info->cleanup) {
> > + vc->info->cleanup(vc);
> > + }
> > + 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);
> > + }
> > + }
> > +
> > + 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 {
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-09-27 21:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-27 13:00 [Qemu-devel] [PATCH] net: delay freeing peer host device Michael S. Tsirkin
2010-09-27 20:18 ` [Qemu-devel] " Alex Williamson
2010-09-27 21:53 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).