Re: [V6 PATCH] virtio-net: send gratuitous packets when needed
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2012-04-04 07:49:31
Also in:
lkml, qemu-devel, virtualization
On Wed, Mar 28, 2012 at 01:44:28PM +0800, Jason Wang wrote:
As hypervior does not have the knowledge of guest network configuration, it's better to ask guest to send gratuitous packets when needed. Guest tests VIRTIO_NET_S_ANNOUNCE bit during config change interrupt and when it is set, a workqueue is scheduled to send gratuitous packet through NETDEV_NOTIFY_PEERS. This feature is negotiated through bit VIRTIO_NET_F_GUEST_ANNOUNCE. Changes from v5: - notify the chain before acking the link annoucement - ack the link announcement notification through control vq Changes from v4: - typos - handle workqueue unconditionally - move VIRTIO_NET_S_ANNOUNCE to bit 8 to separate rw bits from ro bits Changes from v3: - cancel the workqueue during freeze Changes from v2: - fix the race between unregister_dev() and workqueue Signed-off-by: Jason Wang <jasowang@redhat.com>
I think this needs some fixes. See below. Thanks.
quoted hunk ↗ jump to hunk
--- drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++++++++- include/linux/virtio_net.h | 13 +++++++++++++ 2 files changed, 44 insertions(+), 1 deletions(-)diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 4880aa8..0f60da7 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c@@ -72,6 +72,9 @@ struct virtnet_info { /* Work struct for refilling if we run low on memory. */ struct delayed_work refill; + /* Work struct for sending gratuitous packets. */
A bit confusing: it does not send anything itself. A better comment 'for announcing the existence of device on the network'.
quoted hunk ↗ jump to hunk
+ struct work_struct announce; + /* Chain pages by the private ptr. */ struct page *pages;@@ -781,12 +784,30 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, return status == VIRTIO_NET_OK; } +static void virtnet_ack_link_announce(struct virtnet_info *vi) +{ + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE, + VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, + 0, 0)) {
This can run in parallel with other commands. That's pretty bad - will corrupt the cvq. Take rtnl lock around calls to this function?
+ dev_warn(&vi->dev->dev, "Failed to ack link nnounce.\n");
announce
+ }
Better to drop {} around a single statement.
+}
+
+static void announce_work(struct work_struct *work)
+{
+ struct virtnet_info *vi = container_of(work, struct virtnet_info,
+ announce);
+ netif_notify_peers(vi->dev);
+ virtnet_ack_link_announce(vi);
+}
+
static int virtnet_close(struct net_device *dev)
{
struct virtnet_info *vi = netdev_priv(dev);
/* Make sure refill_work doesn't re-enable napi! */
cancel_delayed_work_sync(&vi->refill);
+ cancel_work_sync(&vi->announce);I think that a config change event can trigger after this point, so cancel here won't be effective. But why do it here? virtnet_remove not a better place? We can do it after remove_vq_common.
quoted hunk ↗ jump to hunk
napi_disable(&vi->napi); return 0;@@ -962,11 +983,17 @@ static void virtnet_update_status(struct virtnet_info *vi) return; /* Ignore unknown (future) status bits */ - v &= VIRTIO_NET_S_LINK_UP; + v &= VIRTIO_NET_S_LINK_UP | VIRTIO_NET_S_ANNOUNCE;
The announce bit in vi->status is always clear,
so this is IMO confusing. I would do:
if (v & VIRTIO_NET_S_ANNOUNCE) {
schedule
}
v &= VIRTIO_NET_S_LINK_UP;
and the rest of the logic is not necessary then.
Also, this might run extra ack announce commands after
VIRTIO_NET_S_ANNOUNCE bit is clear. The spec
isn't clear on whether this is legal.
It would be very hard to fix this, so let's add a comment
stating that it's legal, and clarify the spec
in any case.
if (vi->status == v)
return;
+ if (v & VIRTIO_NET_S_ANNOUNCE) {
+ v &= ~VIRTIO_NET_S_ANNOUNCE;
+ if (v & VIRTIO_NET_S_LINK_UP)
+ schedule_work(&vi->announce);I think we really want an nrt wq here - if this triggers multiple times there's no good reason to try and run the ack command many times in parallel.
+ }
+
vi->status = v;
if (vi->status & VIRTIO_NET_S_LINK_UP) {quoted hunk ↗ jump to hunk
@@ -1076,6 +1103,7 @@ static int virtnet_probe(struct virtio_device *vdev) goto free; INIT_DELAYED_WORK(&vi->refill, refill_work); + INIT_WORK(&vi->announce, announce_work); sg_init_table(vi->rx_sg, ARRAY_SIZE(vi->rx_sg)); sg_init_table(vi->tx_sg, ARRAY_SIZE(vi->tx_sg));@@ -1187,6 +1215,7 @@ static int virtnet_freeze(struct virtio_device *vdev) virtqueue_disable_cb(vi->svq); if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) virtqueue_disable_cb(vi->cvq); + cancel_work_sync(&vi->announce);
A config change event can trigger after this point, so cancel here won't be effective. Possibly, we need state like config_enable in the block device. Also, what exactly will happen on suspend? As we reset, ANNOUCE bit will be clear so - do we forget to announce? Probably not good ...
quoted hunk ↗ jump to hunk
netif_device_detach(vi->dev); cancel_delayed_work_sync(&vi->refill);@@ -1233,6 +1262,7 @@ static unsigned int features[] = { VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ, VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, + VIRTIO_NET_F_GUEST_ANNOUNCE, }; static struct virtio_driver virtio_net_driver = {diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index 970d5a2..383e8a0 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h@@ -49,8 +49,10 @@ #define VIRTIO_NET_F_CTRL_RX 18 /* Control channel RX mode support */ #define VIRTIO_NET_F_CTRL_VLAN 19 /* Control channel VLAN filtering */ #define VIRTIO_NET_F_CTRL_RX_EXTRA 20 /* Extra RX mode control support */ +#define VIRTIO_NET_F_GUEST_ANNOUNCE 21 /* Guest can send gratituous packet */
Rusty aligned the comments using tabs so let's do it here too? A better comment 'can announce device on the network'.
#define VIRTIO_NET_S_LINK_UP 1 /* Link is up */ +#define VIRTIO_NET_S_ANNOUNCE 2 /* Announcement is needed */
why 3 spaces before the value here?
quoted hunk ↗ jump to hunk
struct virtio_net_config { /* The config defining mac address (if VIRTIO_NET_F_MAC) */@@ -152,4 +154,15 @@ struct virtio_net_ctrl_mac { #define VIRTIO_NET_CTRL_VLAN_ADD 0 #define VIRTIO_NET_CTRL_VLAN_DEL 1 +/* + * Control link announce acknowledgement + * + * The command VIRTIO_NET_CTRL_ANNOUNCE_ACK is used to indicate that + * driver has recevied the notification and device would clear the
s/recevied/received/ A bit clearer to replace 'and' with ; here.
+ * VIRTIO_NET_S_ANNOUNCE bit in the status filed after it received
s/filed/field/ s/received/receives/
+ * this command. + */ +#define VIRTIO_NET_CTRL_ANNOUNCE 3 + #define VIRTIO_NET_CTRL_ANNOUNCE_ACK 0 + #endif /* _LINUX_VIRTIO_NET_H */ _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization