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

[Anthony Liguori]

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.

> > 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