Re: [Qemu-devel] [RFC v2 PATCH 5/4 PATCH] virtio-net: send gratuitous packet when needed
On Tue, Oct 25, 2011 at 10:50:41AM +0800, Jason Wang wrote: On 10/24/2011 01:25 PM, Michael S. Tsirkin wrote: On Mon, Oct 24, 2011 at 02:54:59PM +1030, Rusty Russell wrote: On Sat, 22 Oct 2011 13:43:11 +0800, Jason Wang jasow...@redhat.com wrote: This make let virtio-net driver can send gratituous packet by a new config bit - VIRTIO_NET_S_ANNOUNCE in each config update interrupt. When this bit is set by backend, the driver would schedule a workqueue to send gratituous packet through NETDEV_NOTIFY_PEERS. This feature is negotiated through bit VIRTIO_NET_F_GUEST_ANNOUNCE. Signed-off-by: Jason Wang jasow...@redhat.com This seems like a huge layering violation. Imagine this in real hardware, for example. commits 06c4648d46d1b757d6b9591a86810be79818b60c and 99606477a5888b0ead0284fecb13417b1da8e3af document the need for this: NETDEV_NOTIFY_PEERS notifier indicates that a device moved to a different physical link. and In real hardware such notifications are only generated when the device comes up or the address changes. So hypervisor could get the same behaviour by sending link up/down events, this is just an optimization so guest won't do unecessary stuff like try to reconfigure an IP address. Maybe LOCATION_CHANGE would be a better name? ANNOUNCE_SELF? It would be nice to formulate what kind of event are we notifying the guest about. The announce part of it is really up to the guest, isn't it? There may be a good reason why virtual devices might want this kind of reconfiguration cheat, which is unnecessary for normal machines, I think yes, the difference with real hardware is guest can change location without link getting dropped. FWIW, Xen seems to use this capability too. So does ms netvsc. but it'd have to be spelled out clearly in the spec to justify it... Cheers, Rusty. Agree, and I'd like to see the spec too. The interface seems to involve the guest clearing the status bit when it detects an event? I would describe this in spec. The interface need guest to clear the status bit, this would let the back-end know it has finished the work as we may need to send the gratuitous packets many times. Also - how does it interact with the link up event? We probably don't want to schedule this when we detect a link status change or during initialization, as this patch seems to do? What if link goes down while the work is running? Is that OK? Looks like there's are duplications if guest enable arp_notify vm is started, How hard would it be to avoid these duplicates? but we need to handle the situation that resuming a stopped virtual machine. For the link down race, I don't see any real issue, either dropping or queued. For example, you do unregister_netdev(vi-dev); cancel_work_sync(vi-announce); which looks scary as announce seems to use the netdev. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [RFC v2 PATCH 5/4 PATCH] virtio-net: send gratuitous packet when needed
On 10/25/2011 11:41 PM, Michael S. Tsirkin wrote: On Tue, Oct 25, 2011 at 10:50:41AM +0800, Jason Wang wrote: On 10/24/2011 01:25 PM, Michael S. Tsirkin wrote: On Mon, Oct 24, 2011 at 02:54:59PM +1030, Rusty Russell wrote: On Sat, 22 Oct 2011 13:43:11 +0800, Jason Wang jasow...@redhat.com wrote: This make let virtio-net driver can send gratituous packet by a new config bit - VIRTIO_NET_S_ANNOUNCE in each config update interrupt. When this bit is set by backend, the driver would schedule a workqueue to send gratituous packet through NETDEV_NOTIFY_PEERS. This feature is negotiated through bit VIRTIO_NET_F_GUEST_ANNOUNCE. Signed-off-by: Jason Wang jasow...@redhat.com This seems like a huge layering violation. Imagine this in real hardware, for example. commits 06c4648d46d1b757d6b9591a86810be79818b60c and 99606477a5888b0ead0284fecb13417b1da8e3af document the need for this: NETDEV_NOTIFY_PEERS notifier indicates that a device moved to a different physical link. and In real hardware such notifications are only generated when the device comes up or the address changes. So hypervisor could get the same behaviour by sending link up/down events, this is just an optimization so guest won't do unecessary stuff like try to reconfigure an IP address. Maybe LOCATION_CHANGE would be a better name? ANNOUNCE_SELF? It would be nice to formulate what kind of event are we notifying the guest about. The announce part of it is really up to the guest, isn't it? Right. There may be a good reason why virtual devices might want this kind of reconfiguration cheat, which is unnecessary for normal machines, I think yes, the difference with real hardware is guest can change location without link getting dropped. FWIW, Xen seems to use this capability too. So does ms netvsc. but it'd have to be spelled out clearly in the spec to justify it... Cheers, Rusty. Agree, and I'd like to see the spec too. The interface seems to involve the guest clearing the status bit when it detects an event? I would describe this in spec. The interface need guest to clear the status bit, this would let the back-end know it has finished the work as we may need to send the gratuitous packets many times. Also - how does it interact with the link up event? We probably don't want to schedule this when we detect a link status change or during initialization, as this patch seems to do? What if link goes down while the work is running? Is that OK? Looks like there's are duplications if guest enable arp_notify vm is started, How hard would it be to avoid these duplicates? Not hard, it could be done in backend by distinguishing the reason : fresh start or cont after migration or stop. but we need to handle the situation that resuming a stopped virtual machine. For the link down race, I don't see any real issue, either dropping or queued. For example, you do unregister_netdev(vi-dev); cancel_work_sync(vi-announce); which looks scary as announce seems to use the netdev. oops, it's a bug, I would fix it. Thanks -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [RFC v2 PATCH 5/4 PATCH] virtio-net: send gratuitous packet when needed
On Mon, 2011-10-24 at 07:25 +0200, Michael S. Tsirkin wrote: On Mon, Oct 24, 2011 at 02:54:59PM +1030, Rusty Russell wrote: On Sat, 22 Oct 2011 13:43:11 +0800, Jason Wang jasow...@redhat.com wrote: This make let virtio-net driver can send gratituous packet by a new config bit - VIRTIO_NET_S_ANNOUNCE in each config update interrupt. When this bit is set by backend, the driver would schedule a workqueue to send gratituous packet through NETDEV_NOTIFY_PEERS. This feature is negotiated through bit VIRTIO_NET_F_GUEST_ANNOUNCE. Signed-off-by: Jason Wang jasow...@redhat.com This seems like a huge layering violation. Imagine this in real hardware, for example. commits 06c4648d46d1b757d6b9591a86810be79818b60c and 99606477a5888b0ead0284fecb13417b1da8e3af document the need for this: NETDEV_NOTIFY_PEERS notifier indicates that a device moved to a different physical link. and In real hardware such notifications are only generated when the device comes up or the address changes. So hypervisor could get the same behaviour by sending link up/down events, this is just an optimization so guest won't do unecessary stuff like try to reconfigure an IP address. Maybe LOCATION_CHANGE would be a better name? [...] We also use this in bonding failover, where the system location doesn't change but a different link is used. However, I do recognise that the name ought to indicate what kind of change happened and not what the expected action is. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [RFC v2 PATCH 5/4 PATCH] virtio-net: send gratuitous packet when needed
On 10/24/2011 01:25 PM, Michael S. Tsirkin wrote: On Mon, Oct 24, 2011 at 02:54:59PM +1030, Rusty Russell wrote: On Sat, 22 Oct 2011 13:43:11 +0800, Jason Wang jasow...@redhat.com wrote: This make let virtio-net driver can send gratituous packet by a new config bit - VIRTIO_NET_S_ANNOUNCE in each config update interrupt. When this bit is set by backend, the driver would schedule a workqueue to send gratituous packet through NETDEV_NOTIFY_PEERS. This feature is negotiated through bit VIRTIO_NET_F_GUEST_ANNOUNCE. Signed-off-by: Jason Wang jasow...@redhat.com This seems like a huge layering violation. Imagine this in real hardware, for example. commits 06c4648d46d1b757d6b9591a86810be79818b60c and 99606477a5888b0ead0284fecb13417b1da8e3af document the need for this: NETDEV_NOTIFY_PEERS notifier indicates that a device moved to a different physical link. and In real hardware such notifications are only generated when the device comes up or the address changes. So hypervisor could get the same behaviour by sending link up/down events, this is just an optimization so guest won't do unecessary stuff like try to reconfigure an IP address. Maybe LOCATION_CHANGE would be a better name? ANNOUNCE_SELF? There may be a good reason why virtual devices might want this kind of reconfiguration cheat, which is unnecessary for normal machines, I think yes, the difference with real hardware is guest can change location without link getting dropped. FWIW, Xen seems to use this capability too. So does ms netvsc. but it'd have to be spelled out clearly in the spec to justify it... Cheers, Rusty. Agree, and I'd like to see the spec too. The interface seems to involve the guest clearing the status bit when it detects an event? I would describe this in spec. The interface need guest to clear the status bit, this would let the back-end know it has finished the work as we may need to send the gratuitous packets many times. Also - how does it interact with the link up event? We probably don't want to schedule this when we detect a link status change or during initialization, as this patch seems to do? What if link goes down while the work is running? Is that OK? Looks like there's are duplications if guest enable arp_notify vm is started, but we need to handle the situation that resuming a stopped virtual machine. For the link down race, I don't see any real issue, either dropping or queued. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [RFC v2 PATCH 5/4 PATCH] virtio-net: send gratuitous packet when needed
On Mon, Oct 24, 2011 at 02:54:59PM +1030, Rusty Russell wrote: On Sat, 22 Oct 2011 13:43:11 +0800, Jason Wang jasow...@redhat.com wrote: This make let virtio-net driver can send gratituous packet by a new config bit - VIRTIO_NET_S_ANNOUNCE in each config update interrupt. When this bit is set by backend, the driver would schedule a workqueue to send gratituous packet through NETDEV_NOTIFY_PEERS. This feature is negotiated through bit VIRTIO_NET_F_GUEST_ANNOUNCE. Signed-off-by: Jason Wang jasow...@redhat.com This seems like a huge layering violation. Imagine this in real hardware, for example. commits 06c4648d46d1b757d6b9591a86810be79818b60c and 99606477a5888b0ead0284fecb13417b1da8e3af document the need for this: NETDEV_NOTIFY_PEERS notifier indicates that a device moved to a different physical link. and In real hardware such notifications are only generated when the device comes up or the address changes. So hypervisor could get the same behaviour by sending link up/down events, this is just an optimization so guest won't do unecessary stuff like try to reconfigure an IP address. Maybe LOCATION_CHANGE would be a better name? There may be a good reason why virtual devices might want this kind of reconfiguration cheat, which is unnecessary for normal machines, I think yes, the difference with real hardware is guest can change location without link getting dropped. FWIW, Xen seems to use this capability too. but it'd have to be spelled out clearly in the spec to justify it... Cheers, Rusty. Agree, and I'd like to see the spec too. The interface seems to involve the guest clearing the status bit when it detects an event? Also - how does it interact with the link up event? We probably don't want to schedule this when we detect a link status change or during initialization, as this patch seems to do? What if link goes down while the work is running? Is that OK? -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
