vhost: disable on tap link down
PATCH v2 1/1 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.
---
OK, this works for me. Pradeep?
hw/virtio-net.c | 3 ++- net.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletions(-)
hw/virtio-net.c +2 -1
@@ -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) {
net.c +7 -0
@@ -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 ; }
Signature · 1 line
1.7.3.2.91.g446ac
Quoted 25 lines · Michael S. Tsirkin
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 <[email protected]>---
OK, this works for me. Pradeep?
hw/virtio-net.c | 3 ++- net.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletions(-)hw/virtio-net.c +0
-0
hw/virtio-net.c static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) · line 122
Quoted 9 lines
hw/virtio-net.c +2
-1@@ -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?
Quoted 7 lines
net.c +0
-0@@ -1324,6 +1324,13 @@ done:if (vc->info->link_status_changed) { vc->info->link_status_changed (vc);
net.c done: · line 1335
Quoted 10 lines
net.c +7
-0} + +/* 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...
Quoted 40 lines · Markus Armbruster
Quoted 25 lines · Michael S. Tsirkin
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 <[email protected]>---
OK, this works for me. Pradeep?
hw/virtio-net.c | 3 ++- net.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletions(-)hw/virtio-net.c +0
-0hw/virtio-net.c static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) · line 122
Quoted 9 lines
hw/virtio-net.c +2
-1@@ -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.
Quoted 6 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
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).
Quoted 21 lines
Quoted 7 lines
net.c +0
-0@@ -1324,6 +1324,13 @@ done:if (vc->info->link_status_changed) { vc->info->link_status_changed (vc);net.c done: · line 1335
Quoted 10 lines
net.c +7
-0} + +/* 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->peerset), notify the peer of our link status change.
Quoted 6 lines
Could you explain why?
Currently, we call
link_status_changed()only when the netdev's own link status changed. I guess existinglink_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.
Quoted 1 line
VLANs always confuse me. Wish we'd finally Retire them...
Signature · 1 line
MST