From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55645) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJvCH-0006ve-Li for qemu-devel@nongnu.org; Tue, 19 Aug 2014 22:00:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJvCB-0002QK-E4 for qemu-devel@nongnu.org; Tue, 19 Aug 2014 22:00:21 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:5296) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJvC9-0002PA-HS for qemu-devel@nongnu.org; Tue, 19 Aug 2014 22:00:15 -0400 Message-ID: <53F400F1.3060408@huawei.com> Date: Wed, 20 Aug 2014 09:59:13 +0800 From: zhanghailiang MIME-Version: 1.0 References: <1408337205-10260-1-git-send-email-zhang.zhanghailiang@huawei.com> <53F1A363.8070009@redhat.com> <53F1BA2A.8020008@huawei.com> <53F1C3E2.8080708@redhat.com> In-Reply-To: <53F1C3E2.8080708@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 17:14, Jason Wang wrote: > On 08/18/2014 04:32 PM, zhanghailiang wrote: >> 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. >> > > Or you can do it in do_vm_stop(). Actually, the callback function was called in do_vm_stop indirectly: do_vm_stop--->vm_state_notify--->e->cb(e->opaque, running, state) And i think use the callbacks is more graceful.:) >>>> + } >>>> +} >>>> + >>>> 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". > > How about other handlers (especially kvm/xen specific ones)? If not, > looks like vm_start() is a more safer place since all handlers were > called before. >> >> 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. :) > > This is true only if there's no dependency. Virtio has a generic vmstate > change handler, a subtle change of your patch is even if vhost is > enabled, during vm start qemu will still process packets since you can > qemu_flush_queued_packets() before vhost_net is started (since virtio > vmstate change handler is called after). So probably we need only do > purging which can eliminate the processing during vm start. Hmm, i will check if this patch has side-effect for vhost_net, ;) Thanks zhanghailiang >> >>>> 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; >>>> } >>> >>> >>> . >>> >> >> > > > . >