From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54194) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJINP-0000XM-E1 for qemu-devel@nongnu.org; Mon, 18 Aug 2014 04:33:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJINK-0002m3-El for qemu-devel@nongnu.org; Mon, 18 Aug 2014 04:33:15 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:4010) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJINI-0002kG-D7 for qemu-devel@nongnu.org; Mon, 18 Aug 2014 04:33:10 -0400 Message-ID: <53F1BA2A.8020008@huawei.com> Date: Mon, 18 Aug 2014 16:32:42 +0800 From: zhanghailiang MIME-Version: 1.0 References: <1408337205-10260-1-git-send-email-zhang.zhanghailiang@huawei.com> <53F1A363.8070009@redhat.com> In-Reply-To: <53F1A363.8070009@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net: Forbid dealing with packets when VM is not running List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: peter.maydell@linaro.org, mst@redhat.com, luonengjun@huawei.com, peter.huangpeng@huawei.com, qemu-devel@nongnu.org, stefanha@redhat.com On 2014/8/18 14:55, Jason Wang wrote: > On 08/18/2014 12:46 PM, zhanghailiang wrote: >> For all NICs(except virtio-net) emulated by qemu, >> Such as e1000, rtl8139, pcnet and ne2k_pci, >> Qemu can still receive packets when VM is not running. >> If this happened in *migration's* last PAUSE VM stage, >> The new dirty RAM related to the packets will be missed, >> And this will lead serious network fault in VM. >> >> To avoid this, we forbid receiving packets in generic net code when >> VM is not running. Also, when the runstate changes back to running, >> we definitely need to flush queues to get packets flowing again. > > You probably need a better title since it does not cover this change. >> Hmm, you are right, i will modify it, thanks.:) >> Here we implement this in the net layer: >> (1) Judge the vm runstate in qemu_can_send_packet >> (2) Add a member 'VMChangeStateEntry *vmstate' to struct NICState, >> Which will listen for VM runstate changes. >> (3) Register a handler function for VMstate change. >> When vm changes back to running, we flush all queues in the callback function. >> (4) Remove checking vm state in virtio_net_can_receive >> >> Signed-off-by: zhanghailiang >> --- >> hw/net/virtio-net.c | 4 ---- >> include/net/net.h | 2 ++ >> net/net.c | 32 ++++++++++++++++++++++++++++++++ >> 3 files changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 268eff9..287d762 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -839,10 +839,6 @@ static int virtio_net_can_receive(NetClientState *nc) >> VirtIODevice *vdev = VIRTIO_DEVICE(n); >> VirtIONetQueue *q = virtio_net_get_subqueue(nc); >> >> - if (!vdev->vm_running) { >> - return 0; >> - } >> - >> if (nc->queue_index>= n->curr_queues) { >> return 0; >> } >> diff --git a/include/net/net.h b/include/net/net.h >> index ed594f9..a294277 100644 >> --- a/include/net/net.h >> +++ b/include/net/net.h >> @@ -8,6 +8,7 @@ >> #include "net/queue.h" >> #include "migration/vmstate.h" >> #include "qapi-types.h" >> +#include "sysemu/sysemu.h" >> >> #define MAX_QUEUE_NUM 1024 >> >> @@ -96,6 +97,7 @@ typedef struct NICState { >> NICConf *conf; >> void *opaque; >> bool peer_deleted; >> + VMChangeStateEntry *vmstate; >> } NICState; >> >> NetClientState *qemu_find_netdev(const char *id); >> diff --git a/net/net.c b/net/net.c >> index 6d930ea..21f0d48 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -242,6 +242,29 @@ NetClientState *qemu_new_net_client(NetClientInfo *info, >> return nc; >> } >> >> +static void nic_vmstate_change_handler(void *opaque, >> + int running, >> + RunState state) >> +{ >> + NICState *nic = opaque; >> + NetClientState *nc; >> + int i, queues; >> + >> + if (!running) { >> + return; >> + } >> + >> + queues = MAX(1, nic->conf->peers.queues); >> + for (i = 0; i< queues; i++) { >> + nc =&nic->ncs[i]; >> + if (nc->receive_disabled >> + || (nc->info->can_receive&& !nc->info->can_receive(nc))) { >> + continue; >> + } >> + qemu_flush_queued_packets(nc); > > How about simply purge the receive queue during stop? If ok, there's no > need to introduce extra vmstate change handler. > I don't know whether it is OK to purge the receive packages, it was suggested by Stefan Hajnoczi, and i am waiting for his opinion .:) I think we still need the extra vmstate change handler, Without the change handler, we don't know if the VM will go to stop and the time when to call qemu_purge_queued_packets. >> + } >> +} >> + >> NICState *qemu_new_nic(NetClientInfo *info, >> NICConf *conf, >> const char *model, >> @@ -259,6 +282,8 @@ NICState *qemu_new_nic(NetClientInfo *info, >> nic->ncs = (void *)nic + info->size; >> nic->conf = conf; >> nic->opaque = opaque; >> + nic->vmstate = qemu_add_vm_change_state_handler(nic_vmstate_change_handler, >> + nic); >> > > Does this depend on other vm state change handler to be called first? I > mean virtio has its own vmstate_change handler and which seems to be > called after this. Is this an issue? > Yes, it is. The check vm state in virtio-net is unnecessary, Actually it will prevent the flushing process, this is why we do step 4 "Remove checking vm state in virtio_net_can_receive". Besides, i think it is OK to do common things in vmstate_change handler of generic net layer and do private things in their own vmstate_change handlers. :) >> for (i = 0; i< queues; i++) { >> qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, name, >> @@ -379,6 +404,7 @@ void qemu_del_nic(NICState *nic) >> qemu_free_net_client(nc); >> } >> >> + qemu_del_vm_change_state_handler(nic->vmstate); >> g_free(nic); >> } >> >> @@ -452,6 +478,12 @@ void qemu_set_vnet_hdr_len(NetClientState *nc, int len) >> >> int qemu_can_send_packet(NetClientState *sender) >> { >> + int vmstat = runstate_is_running(); >> + >> + if (!vmstat) { >> + return 0; >> + } >> + >> if (!sender->peer) { >> return 1; >> } > > > . >