[PATCHv2] vhost: disable on tap link down

qemu makes it possible to disable link at tap
which is not communicated to the guest but
causes all packets to be dropped.

Handle this with vhost simply by moving to the userspace emulation.

Note: it might be a good idea to make peer link status match
tap in this case, so the guest gets an event
and updates the carrier state. For now
stay bug for bug compatible with what we used to have.

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

OK, this works for me. Pradeep?

 hw/virtio-net.c |    3 ++-
 net.c           |    7 +++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

"Michael S. Tsirkin" <mst@redhat.com> writes:

> qemu makes it possible to disable link at tap
> which is not communicated to the guest but
> causes all packets to be dropped.
>
> Handle this with vhost simply by moving to the userspace emulation.
>
> Note: it might be a good idea to make peer link status match
> tap in this case, so the guest gets an event
> and updates the carrier state. For now
> stay bug for bug compatible with what we used to have.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> OK, this works for me. Pradeep?
>
>  hw/virtio-net.c |    3 ++-
>  net.c           |    7 +++++++
>  2 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 671d952..3e3d73a 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -115,7 +115,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>      if (!tap_get_vhost_net(n->nic->nc.peer)) {
>          return;
>      }
> -    if (!!n->vhost_started == virtio_net_started(n, status)) {
> +    if (!!n->vhost_started == virtio_net_started(n, status) &&
> +        !n->nic->nc.peer->link_down) {
>          return;
>      }
>      if (!n->vhost_started) {

I'm not familiar with this code, so let me think aloud a bit to see
whether I get it.  Please correct my misunderstandings.

One use of this function is from within NetClientInfo method
link_status_changed().

link_status_changed() communicates a link status change to the device
model, so it can update guest visible state to reflect the status
change.

There's plenty of virtio magic happening, of which I'm pretty ignorant.

Finally, we end up in virtio_net_vhost_status(), which looks like it's
supposed to start/stop the machinery in the host kernel vhost-net
device.

The old condition for returning here is "there's nothing to do": wanted
status == actual status of the kernel device.

You patch adds a new condition to that: the link (the thing
link_status_changed() informs on) is up.

What problem is the patch fixing?

> diff --git a/net.c b/net.c
> index 9ba5be2..57ee997 100644
> --- a/net.c
> +++ b/net.c
> @@ -1324,6 +1324,13 @@ done:
>      if (vc->info->link_status_changed) {
>          vc->info->link_status_changed(vc);
>      }
> +
> +    /* Notify peer. Don't update peer link status: this makes it possible to
> +     * disconnect from host network without notifying the guest.
> +     * FIXME: is this useful? Could just be an artifact of vlan support. */
> +    if (vc->peer && vc->peer->info->link_status_changed) {
> +        vc->peer->info->link_status_changed(vc->peer);
> +    }
>      return 0;
>  }

If this is a peer-to-peer netdev (vc->peer set), notify the peer of our
link status change.

Could you explain why?

Currently, we call link_status_changed() only when the netdev's own link
status changed.  I guess existing link_status_changed() should do
nothing when only their peer's status changed.  And I figure they should
not know whether they're connected to a peer or a VLAN.

VLANs always confuse me.  Wish we'd finally Retire them...

On Wed, Feb 09, 2011 at 09:52:22AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > qemu makes it possible to disable link at tap
> > which is not communicated to the guest but
> > causes all packets to be dropped.
> >
> > Handle this with vhost simply by moving to the userspace emulation.
> >
> > Note: it might be a good idea to make peer link status match
> > tap in this case, so the guest gets an event
> > and updates the carrier state. For now
> > stay bug for bug compatible with what we used to have.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > OK, this works for me. Pradeep?
> >
> >  hw/virtio-net.c |    3 ++-
> >  net.c           |    7 +++++++
> >  2 files changed, 9 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 671d952..3e3d73a 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -115,7 +115,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >      if (!tap_get_vhost_net(n->nic->nc.peer)) {
> >          return;
> >      }
> > -    if (!!n->vhost_started == virtio_net_started(n, status)) {
> > +    if (!!n->vhost_started == virtio_net_started(n, status) &&
> > +        !n->nic->nc.peer->link_down) {
> >          return;
> >      }
> >      if (!n->vhost_started) {
> 
> I'm not familiar with this code, so let me think aloud a bit to see
> whether I get it.  Please correct my misunderstandings.
> 
> One use of this function is from within NetClientInfo method
> link_status_changed().
> 
> link_status_changed() communicates a link status change to the device
> model, so it can update guest visible state to reflect the status
> change.

But also, it has to update vhost to reflect the status change.

> There's plenty of virtio magic happening, of which I'm pretty ignorant.
> 
> Finally, we end up in virtio_net_vhost_status(), which looks like it's
> supposed to start/stop the machinery in the host kernel vhost-net
> device.
> 
> The old condition for returning here is "there's nothing to do": wanted
> status == actual status of the kernel device.
> 
> You patch adds a new condition to that: the link (the thing
> link_status_changed() informs on) is up.
> 
> What problem is the patch fixing?

ATM setting link down on the tap device does not stop vhost,
so packets keep flowing between tap and guest.

The patch will stop vhost when tap link goes down and restarts it when
the link goes back up (assuming other status is still unchanged).

> > diff --git a/net.c b/net.c
> > index 9ba5be2..57ee997 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -1324,6 +1324,13 @@ done:
> >      if (vc->info->link_status_changed) {
> >          vc->info->link_status_changed(vc);
> >      }
> > +
> > +    /* Notify peer. Don't update peer link status: this makes it possible to
> > +     * disconnect from host network without notifying the guest.
> > +     * FIXME: is this useful? Could just be an artifact of vlan support. */
> > +    if (vc->peer && vc->peer->info->link_status_changed) {
> > +        vc->peer->info->link_status_changed(vc->peer);
> > +    }
> >      return 0;
> >  }
> 
> If this is a peer-to-peer netdev (vc->peer set), notify the peer of our
> link status change.
> 
> Could you explain why?
> 
> Currently, we call link_status_changed() only when the netdev's own link
> status changed.  I guess existing link_status_changed() should do
> nothing when only their peer's status changed.  And I figure they should
> not know whether they're connected to a peer or a VLAN.

This is correct. Guest is not notified because link_down of the device
did not change.  However, the reason we do invoke this callback is so
that virtio can note this and stop forwarding packets
from tap to guest by means of the vhost-net module.

> VLANs always confuse me.  Wish we'd finally Retire them...