From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=41914 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P3XYg-00062C-Mr for qemu-devel@nongnu.org; Wed, 06 Oct 2010 13:13:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P3XYe-0001rX-Vn for qemu-devel@nongnu.org; Wed, 06 Oct 2010 13:13:38 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:52021) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P3XYe-0001rO-SI for qemu-devel@nongnu.org; Wed, 06 Oct 2010 13:13:36 -0400 Received: by yxf34 with SMTP id 34so3569045yxf.4 for ; Wed, 06 Oct 2010 10:13:36 -0700 (PDT) Message-ID: <4CACAE3F.3030904@codemonkey.ws> Date: Wed, 06 Oct 2010 12:13:35 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCHv4] net: delay freeing peer host device References: <20101006160405.GA12265@redhat.com> <4CACA53C.8080507@codemonkey.ws> <20101006165823.GA13486@redhat.com> In-Reply-To: <20101006165823.GA13486@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alex Williamson , qemu-devel@nongnu.org On 10/06/2010 11:58 AM, Michael S. Tsirkin wrote: > On Wed, Oct 06, 2010 at 11:35:08AM -0500, Anthony Liguori wrote: > >> On 10/06/2010 11:04 AM, Michael S. Tsirkin wrote: >> >>> With -netdev, virtio devices present offload >>> features to guest, depending on the backend used. >>> Thus, removing host netdev peer while guest is >>> active leads to guest-visible inconsistency and/or crashes. >>> >>> As a solution, while guest (NIC) peer device exists, >>> we prevent the host peer from being deleted. >>> This patch does this by adding peer_deleted flag in nic state: >>> if host device is going away while guest device >>> is around, set this flag and keep a shell of >>> the host device around for as long as guest device exists. >>> >>> The link is put down so all packets will get discarded. >>> >>> At the moment, management can detect that device deletion >>> is delayed by doing info net. As a next step, we shall add >>> commands that control hotplug/unplug without >>> removing the device, and an event to report that >>> guest has responded to the hotplug event. >>> >>> Signed-off-by: Michael S. Tsirkin >>> >> I don't think this is a very good idea. Making netdev_del >> conditionally succeed is going to break management tools and is very >> non-intuitive. >> >> Regards, >> >> Anthony Liguori >> > But this is not what this does. With this patch netdev_del always > succeeds, and clears the device state leaving just an empty shell around. > The shell goes away together with the NIC. > Sorry, I confused myself. I'm a little confused by the statement: "At the moment, management can detect that device deletion is delayed by doing info net." Is this still the case since you remove the netdev from the list? Regards, Anthony Liguori >>> --- >>> >>> Changes from v3: >>> make sure ->cleanup is called for NICs >>> Changes from v2: >>> fix crash on repeated netdev_del >>> >>> net.c | 49 ++++++++++++++++++++++++++++++++++++++++++------- >>> net.h | 1 + >>> 2 files changed, 43 insertions(+), 7 deletions(-) >>> >>> diff --git a/net.c b/net.c >>> index 3d0fde7..ed74c7f 100644 >>> --- a/net.c >>> +++ b/net.c >>> @@ -281,29 +281,64 @@ NICState *qemu_new_nic(NetClientInfo *info, >>> return nic; >>> } >>> >>> -void qemu_del_vlan_client(VLANClientState *vc) >>> +static void qemu_cleanup_vlan_client(VLANClientState *vc) >>> { >>> if (vc->vlan) { >>> QTAILQ_REMOVE(&vc->vlan->clients, vc, next); >>> } else { >>> - if (vc->send_queue) { >>> - qemu_del_net_queue(vc->send_queue); >>> - } >>> QTAILQ_REMOVE(&non_vlan_clients, vc, next); >>> - if (vc->peer) { >>> - vc->peer->peer = NULL; >>> - } >>> } >>> >>> if (vc->info->cleanup) { >>> vc->info->cleanup(vc); >>> } >>> +} >>> >>> +static void qemu_free_vlan_client(VLANClientState *vc) >>> +{ >>> + if (!vc->vlan) { >>> + if (vc->send_queue) { >>> + qemu_del_net_queue(vc->send_queue); >>> + } >>> + if (vc->peer) { >>> + vc->peer->peer = NULL; >>> + } >>> + } >>> qemu_free(vc->name); >>> qemu_free(vc->model); >>> qemu_free(vc); >>> } >>> >>> +void qemu_del_vlan_client(VLANClientState *vc) >>> +{ >>> + /* If there is a peer NIC, delete and cleanup client, but do not free. */ >>> + if (!vc->vlan&& vc->peer&& vc->peer->info->type == NET_CLIENT_TYPE_NIC) { >>> + NICState *nic = DO_UPCAST(NICState, nc, vc->peer); >>> + if (nic->peer_deleted) { >>> + return; >>> + } >>> + nic->peer_deleted = true; >>> + /* Let NIC know peer is gone. */ >>> + vc->peer->link_down = true; >>> + if (vc->peer->info->link_status_changed) { >>> + vc->peer->info->link_status_changed(vc->peer); >>> + } >>> + qemu_cleanup_vlan_client(vc); >>> + return; >>> + } >>> + >>> + /* If this is a peer NIC and peer has already been deleted, free it now. */ >>> + if (!vc->vlan&& vc->peer&& vc->info->type == NET_CLIENT_TYPE_NIC) { >>> + NICState *nic = DO_UPCAST(NICState, nc, vc); >>> + if (nic->peer_deleted) { >>> + qemu_free_vlan_client(vc->peer); >>> + } >>> + } >>> + >>> + qemu_cleanup_vlan_client(vc); >>> + qemu_free_vlan_client(vc); >>> +} >>> + >>> VLANClientState * >>> qemu_find_vlan_client_by_name(Monitor *mon, int vlan_id, >>> const char *client_str) >>> diff --git a/net.h b/net.h >>> index 518cf9c..44c31a9 100644 >>> --- a/net.h >>> +++ b/net.h >>> @@ -72,6 +72,7 @@ typedef struct NICState { >>> VLANClientState nc; >>> NICConf *conf; >>> void *opaque; >>> + bool peer_deleted; >>> } NICState; >>> >>> struct VLANState { >>>