Re: [Qemu-devel] [PATCHv2] vhost: disable on tap link down

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> 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 <address@hidden>
> ---
>
> 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...