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 <jasowang@xxxxxxxxxx> 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 <jasowang@xxxxxxxxxx> >>>> >>>> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
