* [Qemu-devel] [PATCHv2] virtio-net: unify vhost-net start/stop
@ 2010-09-27 16:41 Michael S. Tsirkin
2010-09-27 19:43 ` [Qemu-devel] " Alex Williamson
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2010-09-27 16:41 UTC (permalink / raw)
To: qemu-devel, Anthony Liguori, Alex Williamson
Move all of vhost-net start/stop logic to a single routine,
and call it from everywhere.
Additionally, start/stop vhost-net on link up/down:
we should not transmit anything if user asked us to
put the link down.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Sent a wrong version, sorry about the noise.
hw/virtio-net.c | 89 ++++++++++++++++++++++++++----------------------------
1 files changed, 43 insertions(+), 46 deletions(-)
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 075f72d..f87c219 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -51,6 +51,7 @@ typedef struct VirtIONet
uint8_t nouni;
uint8_t nobcast;
uint8_t vhost_started;
+ bool vm_running;
VMChangeStateEntry *vmstate;
struct {
int in_use;
@@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
}
}
+static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
+{
+ VirtIONet *n = to_virtio_net(vdev);
+ if (!n->nic->nc.peer) {
+ return;
+ }
+ if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+ return;
+ }
+
+ if (!tap_get_vhost_net(n->nic->nc.peer)) {
+ return;
+ }
+ if (!!n->vhost_started == (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ (n->status & VIRTIO_NET_S_LINK_UP) &&
+ n->vm_running) {
+ return;
+ }
+ if (!n->vhost_started) {
+ int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
+ if (r < 0) {
+ fprintf(stderr, "unable to start vhost net: %d: "
+ "falling back on userspace virtio\n", -r);
+ } else {
+ n->vhost_started = 1;
+ }
+ } else {
+ vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
+ n->vhost_started = 0;
+ }
+}
+
static void virtio_net_set_link_status(VLANClientState *nc)
{
VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
@@ -107,6 +140,8 @@ static void virtio_net_set_link_status(VLANClientState *nc)
if (n->status != old_status)
virtio_notify_config(&n->vdev);
+
+ virtio_net_set_status(&n->vdev, n->vdev.status);
}
static void virtio_net_reset(VirtIODevice *vdev)
@@ -120,10 +155,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
n->nomulti = 0;
n->nouni = 0;
n->nobcast = 0;
- if (n->vhost_started) {
- vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
- n->vhost_started = 0;
- }
/* Flush any MAC and VLAN filter table state */
n->mac_table.in_use = 0;
@@ -726,12 +757,9 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
{
VirtIONet *n = opaque;
- if (n->vhost_started) {
- /* TODO: should we really stop the backend?
- * If we don't, it might keep writing to memory. */
- vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
- n->vhost_started = 0;
- }
+ /* At this point, backend must be stopped, otherwise
+ * it might keep writing to memory. */
+ assert(!n->vhost_started);
virtio_save(&n->vdev, f);
qemu_put_buffer(f, n->mac, ETH_ALEN);
@@ -863,44 +891,14 @@ static NetClientInfo net_virtio_info = {
.link_status_changed = virtio_net_set_link_status,
};
-static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
-{
- VirtIONet *n = to_virtio_net(vdev);
- if (!n->nic->nc.peer) {
- return;
- }
- if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
- return;
- }
-
- if (!tap_get_vhost_net(n->nic->nc.peer)) {
- return;
- }
- if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
- return;
- }
- if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
- int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
- if (r < 0) {
- fprintf(stderr, "unable to start vhost net: %d: "
- "falling back on userspace virtio\n", -r);
- } else {
- n->vhost_started = 1;
- }
- } else {
- vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
- n->vhost_started = 0;
- }
-}
-
static void virtio_net_vmstate_change(void *opaque, int running, int reason)
{
VirtIONet *n = opaque;
- uint8_t status = running ? VIRTIO_CONFIG_S_DRIVER_OK : 0;
+ n->vm_running = running;
/* This is called when vm is started/stopped,
- * it will start/stop vhost backend if * appropriate
+ * it will start/stop vhost backend if appropriate
* e.g. after migration. */
- virtio_net_set_status(&n->vdev, n->vdev.status & status);
+ virtio_net_set_status(&n->vdev, n->vdev.status);
}
VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
@@ -951,9 +949,8 @@ void virtio_net_exit(VirtIODevice *vdev)
VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
qemu_del_vm_change_state_handler(n->vmstate);
- if (n->vhost_started) {
- vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
- }
+ /* This will stop vhost backend if appropriate. */
+ virtio_net_set_status(vdev, 0);
qemu_purge_queued_packets(&n->nic->nc);
--
1.7.3-rc1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] Re: [PATCHv2] virtio-net: unify vhost-net start/stop
2010-09-27 16:41 [Qemu-devel] [PATCHv2] virtio-net: unify vhost-net start/stop Michael S. Tsirkin
@ 2010-09-27 19:43 ` Alex Williamson
2010-09-27 21:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2010-09-27 19:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Mon, 2010-09-27 at 18:41 +0200, Michael S. Tsirkin wrote:
> Move all of vhost-net start/stop logic to a single routine,
> and call it from everywhere.
>
> Additionally, start/stop vhost-net on link up/down:
> we should not transmit anything if user asked us to
> put the link down.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Sent a wrong version, sorry about the noise.
>
> hw/virtio-net.c | 89 ++++++++++++++++++++++++++----------------------------
> 1 files changed, 43 insertions(+), 46 deletions(-)
>
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 075f72d..f87c219 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -51,6 +51,7 @@ typedef struct VirtIONet
> uint8_t nouni;
> uint8_t nobcast;
> uint8_t vhost_started;
> + bool vm_running;
> VMChangeStateEntry *vmstate;
> struct {
> int in_use;
> @@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> }
> }
>
> +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> +{
> + VirtIONet *n = to_virtio_net(vdev);
> + if (!n->nic->nc.peer) {
> + return;
> + }
> + if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> + return;
> + }
> +
> + if (!tap_get_vhost_net(n->nic->nc.peer)) {
> + return;
> + }
> + if (!!n->vhost_started == (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> + (n->status & VIRTIO_NET_S_LINK_UP) &&
> + n->vm_running) {
I don't think this works...
#define VIRTIO_CONFIG_S_DRIVER_OK 4
!!(1) == (status & 4)?
> + return;
> + }
> + if (!n->vhost_started) {
> + int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> + if (r < 0) {
> + fprintf(stderr, "unable to start vhost net: %d: "
> + "falling back on userspace virtio\n", -r);
Definitely worth noting if not using vhost, but aren't we potentially
going to see this in the log a lot in that case? Thanks,
Alex
> + } else {
> + n->vhost_started = 1;
> + }
> + } else {
> + vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> + n->vhost_started = 0;
> + }
> +}
> +
> static void virtio_net_set_link_status(VLANClientState *nc)
> {
> VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> @@ -107,6 +140,8 @@ static void virtio_net_set_link_status(VLANClientState *nc)
>
> if (n->status != old_status)
> virtio_notify_config(&n->vdev);
> +
> + virtio_net_set_status(&n->vdev, n->vdev.status);
> }
>
> static void virtio_net_reset(VirtIODevice *vdev)
> @@ -120,10 +155,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
> n->nomulti = 0;
> n->nouni = 0;
> n->nobcast = 0;
> - if (n->vhost_started) {
> - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> - n->vhost_started = 0;
> - }
>
> /* Flush any MAC and VLAN filter table state */
> n->mac_table.in_use = 0;
> @@ -726,12 +757,9 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
> {
> VirtIONet *n = opaque;
>
> - if (n->vhost_started) {
> - /* TODO: should we really stop the backend?
> - * If we don't, it might keep writing to memory. */
> - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> - n->vhost_started = 0;
> - }
> + /* At this point, backend must be stopped, otherwise
> + * it might keep writing to memory. */
> + assert(!n->vhost_started);
> virtio_save(&n->vdev, f);
>
> qemu_put_buffer(f, n->mac, ETH_ALEN);
> @@ -863,44 +891,14 @@ static NetClientInfo net_virtio_info = {
> .link_status_changed = virtio_net_set_link_status,
> };
>
> -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> -{
> - VirtIONet *n = to_virtio_net(vdev);
> - if (!n->nic->nc.peer) {
> - return;
> - }
> - if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> - return;
> - }
> -
> - if (!tap_get_vhost_net(n->nic->nc.peer)) {
> - return;
> - }
> - if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> - return;
> - }
> - if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> - int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
> - if (r < 0) {
> - fprintf(stderr, "unable to start vhost net: %d: "
> - "falling back on userspace virtio\n", -r);
> - } else {
> - n->vhost_started = 1;
> - }
> - } else {
> - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> - n->vhost_started = 0;
> - }
> -}
> -
> static void virtio_net_vmstate_change(void *opaque, int running, int reason)
> {
> VirtIONet *n = opaque;
> - uint8_t status = running ? VIRTIO_CONFIG_S_DRIVER_OK : 0;
> + n->vm_running = running;
> /* This is called when vm is started/stopped,
> - * it will start/stop vhost backend if * appropriate
> + * it will start/stop vhost backend if appropriate
> * e.g. after migration. */
> - virtio_net_set_status(&n->vdev, n->vdev.status & status);
> + virtio_net_set_status(&n->vdev, n->vdev.status);
> }
>
> VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
> @@ -951,9 +949,8 @@ void virtio_net_exit(VirtIODevice *vdev)
> VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> qemu_del_vm_change_state_handler(n->vmstate);
>
> - if (n->vhost_started) {
> - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> - }
> + /* This will stop vhost backend if appropriate. */
> + virtio_net_set_status(vdev, 0);
>
> qemu_purge_queued_packets(&n->nic->nc);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] Re: [PATCHv2] virtio-net: unify vhost-net start/stop
2010-09-27 19:43 ` [Qemu-devel] " Alex Williamson
@ 2010-09-27 21:52 ` Michael S. Tsirkin
2010-09-28 15:16 ` Alex Williamson
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2010-09-27 21:52 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel
On Mon, Sep 27, 2010 at 01:43:11PM -0600, Alex Williamson wrote:
> On Mon, 2010-09-27 at 18:41 +0200, Michael S. Tsirkin wrote:
> > Move all of vhost-net start/stop logic to a single routine,
> > and call it from everywhere.
> >
> > Additionally, start/stop vhost-net on link up/down:
> > we should not transmit anything if user asked us to
> > put the link down.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Sent a wrong version, sorry about the noise.
> >
> > hw/virtio-net.c | 89 ++++++++++++++++++++++++++----------------------------
> > 1 files changed, 43 insertions(+), 46 deletions(-)
> >
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 075f72d..f87c219 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -51,6 +51,7 @@ typedef struct VirtIONet
> > uint8_t nouni;
> > uint8_t nobcast;
> > uint8_t vhost_started;
> > + bool vm_running;
> > VMChangeStateEntry *vmstate;
> > struct {
> > int in_use;
> > @@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > }
> > }
> >
> > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > +{
> > + VirtIONet *n = to_virtio_net(vdev);
> > + if (!n->nic->nc.peer) {
> > + return;
> > + }
> > + if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > + return;
> > + }
> > +
> > + if (!tap_get_vhost_net(n->nic->nc.peer)) {
> > + return;
> > + }
> > + if (!!n->vhost_started == (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > + (n->status & VIRTIO_NET_S_LINK_UP) &&
> > + n->vm_running) {
>
> I don't think this works...
>
> #define VIRTIO_CONFIG_S_DRIVER_OK 4
>
> !!(1) == (status & 4)?
The && converts it to a boolean.
> > + return;
> > + }
> > + if (!n->vhost_started) {
> > + int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > + if (r < 0) {
> > + fprintf(stderr, "unable to start vhost net: %d: "
> > + "falling back on userspace virtio\n", -r);
>
> Definitely worth noting if not using vhost, but aren't we potentially
> going to see this in the log a lot in that case? Thanks,
>
> Alex
Not really, we detect failure at start normally, and do
not proceed.
This means that somehow the device went dead: should not
happen really, but since it's easy to recover we do.
> > + } else {
> > + n->vhost_started = 1;
> > + }
> > + } else {
> > + vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > + n->vhost_started = 0;
> > + }
> > +}
> > +
> > static void virtio_net_set_link_status(VLANClientState *nc)
> > {
> > VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> > @@ -107,6 +140,8 @@ static void virtio_net_set_link_status(VLANClientState *nc)
> >
> > if (n->status != old_status)
> > virtio_notify_config(&n->vdev);
> > +
> > + virtio_net_set_status(&n->vdev, n->vdev.status);
> > }
> >
> > static void virtio_net_reset(VirtIODevice *vdev)
> > @@ -120,10 +155,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
> > n->nomulti = 0;
> > n->nouni = 0;
> > n->nobcast = 0;
> > - if (n->vhost_started) {
> > - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > - n->vhost_started = 0;
> > - }
> >
> > /* Flush any MAC and VLAN filter table state */
> > n->mac_table.in_use = 0;
> > @@ -726,12 +757,9 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
> > {
> > VirtIONet *n = opaque;
> >
> > - if (n->vhost_started) {
> > - /* TODO: should we really stop the backend?
> > - * If we don't, it might keep writing to memory. */
> > - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > - n->vhost_started = 0;
> > - }
> > + /* At this point, backend must be stopped, otherwise
> > + * it might keep writing to memory. */
> > + assert(!n->vhost_started);
> > virtio_save(&n->vdev, f);
> >
> > qemu_put_buffer(f, n->mac, ETH_ALEN);
> > @@ -863,44 +891,14 @@ static NetClientInfo net_virtio_info = {
> > .link_status_changed = virtio_net_set_link_status,
> > };
> >
> > -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > -{
> > - VirtIONet *n = to_virtio_net(vdev);
> > - if (!n->nic->nc.peer) {
> > - return;
> > - }
> > - if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > - return;
> > - }
> > -
> > - if (!tap_get_vhost_net(n->nic->nc.peer)) {
> > - return;
> > - }
> > - if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > - return;
> > - }
> > - if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> > - int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > - if (r < 0) {
> > - fprintf(stderr, "unable to start vhost net: %d: "
> > - "falling back on userspace virtio\n", -r);
> > - } else {
> > - n->vhost_started = 1;
> > - }
> > - } else {
> > - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > - n->vhost_started = 0;
> > - }
> > -}
> > -
> > static void virtio_net_vmstate_change(void *opaque, int running, int reason)
> > {
> > VirtIONet *n = opaque;
> > - uint8_t status = running ? VIRTIO_CONFIG_S_DRIVER_OK : 0;
> > + n->vm_running = running;
> > /* This is called when vm is started/stopped,
> > - * it will start/stop vhost backend if * appropriate
> > + * it will start/stop vhost backend if appropriate
> > * e.g. after migration. */
> > - virtio_net_set_status(&n->vdev, n->vdev.status & status);
> > + virtio_net_set_status(&n->vdev, n->vdev.status);
> > }
> >
> > VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
> > @@ -951,9 +949,8 @@ void virtio_net_exit(VirtIODevice *vdev)
> > VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> > qemu_del_vm_change_state_handler(n->vmstate);
> >
> > - if (n->vhost_started) {
> > - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > - }
> > + /* This will stop vhost backend if appropriate. */
> > + virtio_net_set_status(vdev, 0);
> >
> > qemu_purge_queued_packets(&n->nic->nc);
> >
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] Re: [PATCHv2] virtio-net: unify vhost-net start/stop
2010-09-27 21:52 ` Michael S. Tsirkin
@ 2010-09-28 15:16 ` Alex Williamson
2010-10-03 20:17 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2010-09-28 15:16 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Mon, 2010-09-27 at 23:52 +0200, Michael S. Tsirkin wrote:
> On Mon, Sep 27, 2010 at 01:43:11PM -0600, Alex Williamson wrote:
> > On Mon, 2010-09-27 at 18:41 +0200, Michael S. Tsirkin wrote:
> > > Move all of vhost-net start/stop logic to a single routine,
> > > and call it from everywhere.
> > >
> > > Additionally, start/stop vhost-net on link up/down:
> > > we should not transmit anything if user asked us to
> > > put the link down.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >
> > > Sent a wrong version, sorry about the noise.
> > >
> > > hw/virtio-net.c | 89 ++++++++++++++++++++++++++----------------------------
> > > 1 files changed, 43 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > index 075f72d..f87c219 100644
> > > --- a/hw/virtio-net.c
> > > +++ b/hw/virtio-net.c
> > > @@ -51,6 +51,7 @@ typedef struct VirtIONet
> > > uint8_t nouni;
> > > uint8_t nobcast;
> > > uint8_t vhost_started;
> > > + bool vm_running;
> > > VMChangeStateEntry *vmstate;
> > > struct {
> > > int in_use;
> > > @@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > > }
> > > }
> > >
> > > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > > +{
> > > + VirtIONet *n = to_virtio_net(vdev);
> > > + if (!n->nic->nc.peer) {
> > > + return;
> > > + }
> > > + if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > > + return;
> > > + }
> > > +
> > > + if (!tap_get_vhost_net(n->nic->nc.peer)) {
> > > + return;
> > > + }
> > > + if (!!n->vhost_started == (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > + (n->status & VIRTIO_NET_S_LINK_UP) &&
> > > + n->vm_running) {
> >
> > I don't think this works...
> >
> > #define VIRTIO_CONFIG_S_DRIVER_OK 4
> >
> > !!(1) == (status & 4)?
>
> The && converts it to a boolean.
Are you missing a set of parens around everything right of the ==? ie:
if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
(n->status & VIRTIO_NET_S_LINK_UP) &&
n->vm_running)) {...
Has this actually been tested?
Alex
> > > + return;
> > > + }
> > > + if (!n->vhost_started) {
> > > + int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > > + if (r < 0) {
> > > + fprintf(stderr, "unable to start vhost net: %d: "
> > > + "falling back on userspace virtio\n", -r);
> >
> > Definitely worth noting if not using vhost, but aren't we potentially
> > going to see this in the log a lot in that case? Thanks,
> >
> > Alex
>
> Not really, we detect failure at start normally, and do
> not proceed.
> This means that somehow the device went dead: should not
> happen really, but since it's easy to recover we do.
>
> > > + } else {
> > > + n->vhost_started = 1;
> > > + }
> > > + } else {
> > > + vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > > + n->vhost_started = 0;
> > > + }
> > > +}
> > > +
> > > static void virtio_net_set_link_status(VLANClientState *nc)
> > > {
> > > VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> > > @@ -107,6 +140,8 @@ static void virtio_net_set_link_status(VLANClientState *nc)
> > >
> > > if (n->status != old_status)
> > > virtio_notify_config(&n->vdev);
> > > +
> > > + virtio_net_set_status(&n->vdev, n->vdev.status);
> > > }
> > >
> > > static void virtio_net_reset(VirtIODevice *vdev)
> > > @@ -120,10 +155,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
> > > n->nomulti = 0;
> > > n->nouni = 0;
> > > n->nobcast = 0;
> > > - if (n->vhost_started) {
> > > - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > > - n->vhost_started = 0;
> > > - }
> > >
> > > /* Flush any MAC and VLAN filter table state */
> > > n->mac_table.in_use = 0;
> > > @@ -726,12 +757,9 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
> > > {
> > > VirtIONet *n = opaque;
> > >
> > > - if (n->vhost_started) {
> > > - /* TODO: should we really stop the backend?
> > > - * If we don't, it might keep writing to memory. */
> > > - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > > - n->vhost_started = 0;
> > > - }
> > > + /* At this point, backend must be stopped, otherwise
> > > + * it might keep writing to memory. */
> > > + assert(!n->vhost_started);
> > > virtio_save(&n->vdev, f);
> > >
> > > qemu_put_buffer(f, n->mac, ETH_ALEN);
> > > @@ -863,44 +891,14 @@ static NetClientInfo net_virtio_info = {
> > > .link_status_changed = virtio_net_set_link_status,
> > > };
> > >
> > > -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > > -{
> > > - VirtIONet *n = to_virtio_net(vdev);
> > > - if (!n->nic->nc.peer) {
> > > - return;
> > > - }
> > > - if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > > - return;
> > > - }
> > > -
> > > - if (!tap_get_vhost_net(n->nic->nc.peer)) {
> > > - return;
> > > - }
> > > - if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > - return;
> > > - }
> > > - if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> > > - int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > > - if (r < 0) {
> > > - fprintf(stderr, "unable to start vhost net: %d: "
> > > - "falling back on userspace virtio\n", -r);
> > > - } else {
> > > - n->vhost_started = 1;
> > > - }
> > > - } else {
> > > - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > > - n->vhost_started = 0;
> > > - }
> > > -}
> > > -
> > > static void virtio_net_vmstate_change(void *opaque, int running, int reason)
> > > {
> > > VirtIONet *n = opaque;
> > > - uint8_t status = running ? VIRTIO_CONFIG_S_DRIVER_OK : 0;
> > > + n->vm_running = running;
> > > /* This is called when vm is started/stopped,
> > > - * it will start/stop vhost backend if * appropriate
> > > + * it will start/stop vhost backend if appropriate
> > > * e.g. after migration. */
> > > - virtio_net_set_status(&n->vdev, n->vdev.status & status);
> > > + virtio_net_set_status(&n->vdev, n->vdev.status);
> > > }
> > >
> > > VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
> > > @@ -951,9 +949,8 @@ void virtio_net_exit(VirtIODevice *vdev)
> > > VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> > > qemu_del_vm_change_state_handler(n->vmstate);
> > >
> > > - if (n->vhost_started) {
> > > - vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > > - }
> > > + /* This will stop vhost backend if appropriate. */
> > > + virtio_net_set_status(vdev, 0);
> > >
> > > qemu_purge_queued_packets(&n->nic->nc);
> > >
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] Re: [PATCHv2] virtio-net: unify vhost-net start/stop
2010-09-28 15:16 ` Alex Williamson
@ 2010-10-03 20:17 ` Michael S. Tsirkin
0 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2010-10-03 20:17 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel
On Tue, Sep 28, 2010 at 09:16:34AM -0600, Alex Williamson wrote:
> On Mon, 2010-09-27 at 23:52 +0200, Michael S. Tsirkin wrote:
> > On Mon, Sep 27, 2010 at 01:43:11PM -0600, Alex Williamson wrote:
> > > On Mon, 2010-09-27 at 18:41 +0200, Michael S. Tsirkin wrote:
> > > > Move all of vhost-net start/stop logic to a single routine,
> > > > and call it from everywhere.
> > > >
> > > > Additionally, start/stop vhost-net on link up/down:
> > > > we should not transmit anything if user asked us to
> > > > put the link down.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >
> > > > Sent a wrong version, sorry about the noise.
> > > >
> > > > hw/virtio-net.c | 89 ++++++++++++++++++++++++++----------------------------
> > > > 1 files changed, 43 insertions(+), 46 deletions(-)
> > > >
> > > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > > index 075f72d..f87c219 100644
> > > > --- a/hw/virtio-net.c
> > > > +++ b/hw/virtio-net.c
> > > > @@ -51,6 +51,7 @@ typedef struct VirtIONet
> > > > uint8_t nouni;
> > > > uint8_t nobcast;
> > > > uint8_t vhost_started;
> > > > + bool vm_running;
> > > > VMChangeStateEntry *vmstate;
> > > > struct {
> > > > int in_use;
> > > > @@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > > > }
> > > > }
> > > >
> > > > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > > > +{
> > > > + VirtIONet *n = to_virtio_net(vdev);
> > > > + if (!n->nic->nc.peer) {
> > > > + return;
> > > > + }
> > > > + if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > > > + return;
> > > > + }
> > > > +
> > > > + if (!tap_get_vhost_net(n->nic->nc.peer)) {
> > > > + return;
> > > > + }
> > > > + if (!!n->vhost_started == (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > + (n->status & VIRTIO_NET_S_LINK_UP) &&
> > > > + n->vm_running) {
> > >
> > > I don't think this works...
> > >
> > > #define VIRTIO_CONFIG_S_DRIVER_OK 4
> > >
> > > !!(1) == (status & 4)?
> >
> > The && converts it to a boolean.
>
> Are you missing a set of parens around everything right of the ==? ie:
>
> if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> (n->status & VIRTIO_NET_S_LINK_UP) &&
> n->vm_running)) {...
Yes, that's it. Thanks!
--
MST
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-10-03 20:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-27 16:41 [Qemu-devel] [PATCHv2] virtio-net: unify vhost-net start/stop Michael S. Tsirkin
2010-09-27 19:43 ` [Qemu-devel] " Alex Williamson
2010-09-27 21:52 ` Michael S. Tsirkin
2010-09-28 15:16 ` Alex Williamson
2010-10-03 20:17 ` 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).