[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Re: [PATCH] vhost-net: Do not set features for backend
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] Re: [PATCH] vhost-net: Do not set features for backend when peer was deleted |
Date: |
Wed, 27 Oct 2010 20:09:17 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Oct 27, 2010 at 03:26:04PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 27, 2010 at 02:59:16PM +0800, Jason Wang wrote:
> > Michael S. Tsirkin writes:
> > > On Wed, Oct 27, 2010 at 12:43:51AM +0800, Jason Wang wrote:
> > > > When hot-unplug a virtio nic with vhost-net backend, guest may
> > > > continue to program the nic even if its peer have been deleted. We can
> > > > not set features at this time as vhost_net_ack_features() may still
> > > > try to use the tap related vhost_net structure which have been freed
> > > > in tap_cleanup(). And setting offload features for a deleted backend
> > > > is also meaningless in this situation
> > > >
> > > > Signed-off-by: Jason Wang <address@hidden>
> > >
> > > Hmm. Actually, this is not enough.
> > > We really must stop and cleanup vhost net.
> > >
> >
> > But this patch just prevent the guest from programming a nic with its
> > vhost-net backend deleted. The vhost net was still stopped and cleaned
> > when the peer have been removed.
>
> No, I think it was cleanup but not stopped first. E.g. try to migrate at
> this point and see it crash. I suspect we also leak fds for notifiers
> and what not.
Hmm. I missed the fact that we bring the link down, which
will stop vhost_net. So no, it's not broken.
But I'd rather we didn't look at peer_deleted and leave
this as an implementation detail in net.c: I think the
bug is in returning an invalid pointer from get_vhost_net.
Posted a patch to fix that.
> > > Two issues are
> > > 1. vhost_net_stop needs virtio device pointer
> > > 2. virtio net has a vhost_started flag
> > >
> > > Two ways to fix it that I see:
> > > 1. add a callback in nic and invoke when peer_deleted is set
> > > 2. add vhost_started and virtio device pointers in vhost
> > >
> > > Path 1 seems easier ...
> > >
> > > MST
> > >
> > > > ---
> > > > hw/virtio-net.c | 3 +++
> > > > 1 files changed, 3 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > > index 7e1688c..68c8e48 100644
> > > > --- a/hw/virtio-net.c
> > > > +++ b/hw/virtio-net.c
> > > > @@ -245,6 +245,9 @@ static void virtio_net_set_features(VirtIODevice
> > *vdev, uint32_t features)
> > > > {
> > > > VirtIONet *n = to_virtio_net(vdev);
> > > >
> > > > + if (n->nic->peer_deleted)
> > > > + return;
> > > > +
> > > > n->mergeable_rx_bufs = !!(features & (1 <<
> > VIRTIO_NET_F_MRG_RXBUF));
> > > >
> > > > if (n->has_vnet_hdr) {
> > >