[Qemu-devel] Re: [PATCH] net: delay peer host device delete

[Michael S. Tsirkin]

On Mon, Sep 20, 2010 at 03:50:51PM -0500, Anthony Liguori wrote:
> On 09/20/2010 03:37 PM, Michael S. Tsirkin wrote:
> >On Mon, Sep 20, 2010 at 03:38:36PM -0500, Anthony Liguori wrote:
> >>On 09/20/2010 03:27 PM, Michael S. Tsirkin wrote:
> >>>On Mon, Sep 20, 2010 at 03:20:59PM -0500, Anthony Liguori wrote:
> >>>>On 09/20/2010 02:44 PM, Michael S. Tsirkin wrote:
> >>>>>>I think the only workable approach that doesn't involve new commands
> >>>>>>is to change the semantics of the existing ones.
> >>>>>>
> >>>>>>Make netdev_del work regardless of whether the device is still present.
> >>>>>>
> >>>>>>You would need to reference count the actual netdev structure and
> >>>>>>have each device using it unref on delete.  You make netdev_del mark
> >>>>>>the device as deleted and when a device is deleted, any calls into
> >>>>>>the device effectively become nops.
> >>>>>>
> >>>>>>You have to go through most of the cleanup process to ensure that
> >>>>>>tap device gets closed even before your reference count goes to
> >>>>>>zero.
> >>>>>I think you mean 'does not get closed': we need the fd to get the flags 
> >>>>>etc.
> >>>>No, I actually meant does get closed.
> >>>>
> >>>>When you do netdev_del, it should result in the fd getting closed.
> >>>>
> >>>>The actual netdev structure then becomes a zombie that's completely
> >>>>useless until the device goes away.
> >>>>
> >>>>>Note that it will mostly work unless when it'll crash.
> >>>>>Issue is we don't have any documentation so
> >>>>>people get the command set by trial and error.
> >>>>>
> >>>>>So how can we prove it's a user bug and not qemu bug?
> >>>>>I guess we should blame ourselves until proven innocent.
> >>>>Here's what I'm now suggesting:
> >>>>
> >>>>device_del ->   may or may not unplug a device from a guest when it
> >>>>returns.  To figure out if it does, you have to run info qdm.
> >>>I think it should also always unplug on guest reset.
> >>>
> >>>>netdev_del ->   always destroys a netdev device when it returns.  May
> >>>>be called at any point in time.  If you destroy a netdev while the
> >>>>device is still using it, all packets go into the bit bucket and the
> >>>>link status is modified to be unplugged.
> >>>One issue here is that we can't allow a new device with same name
> >>>to be created until the nic is destroyed.
> >>A new netdev device?  Why not?
> >Because it won't work: it will try to pair with existing nic device
> >(it is looked up by name) and that will fail.
> 
> No, netdev_del should remove the VLANClientState from the
> non_vlan_clients list.
> 
> It's no longer enumerable and it's no longer lookup-able.
> 
> The only reason it stays around it so that the device doesn't have a
> reference to a free pointer.  The only field that's ever looked at
> is is_deleted which is used by every function to turn around and
> implement a nop.
> 
> The VLANClientState is a hollow shell of it's former glorious self.
> The remainder of it's (hopefully short) life is merely so that we
> can avoid touching every device to teach them about disconnecting
> backends.

We'll have to tell them link is down, won't we?

> >>Because the fundamental problem is that device_del is too difficult
> >>to use.  You're just making netdev_del equally difficult to use.
> >>
> >>Try your patch with libvirt, don't load acpiphp in the guest, and
> >>then play around with virsh device_detach and device_attach.  All
> >>sorts of badness will ensue as libvirt tries to manage assignment of
> >>PCI slot numbers and such.
> >>
> >>Regards,
> >>
> >>Anthony Liguori
> >At some level, that's right. Is yours better?
> 
> device_del is still busted but at least netdev_del behaves the way
> libvirt expects it to.
> 
> >I guess the right thing is to wait for libvirt guys to tell
> >us what they prefer.
> 
> Yeah, I think some clarity is needed.
> 
> Regards,
> 
> Anthony Liguori
>