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