On 08/22/2014 05:21 PM, zhanghailiang wrote:
> On 2014/8/22 15:40, Jason Wang wrote:
>> On 08/21/2014 08:39 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.
>> Hi:
>>
>> Can you describe what will happen if you don't flush queues after vm is
>> started? Btw, the notifier dependency and impact on vhost should be
>> mentioned here as well.
>
> Hi Jason,
>
> There is no side-effect for vhost-net because in
> nic_vmstate_change_handler callback,
> We will check the return value of nc->info->can_receive before flush
> queues,
> If the vhost-net(virtio-net) is not ready, it will return 0, then we
> will not
> do the flush action.
Thanks for the clarification. Then the vmstate handler does nothing for
virtio-net :)
>
> Also i have tested this patch with vhost_net, include when multiqueue
> is on,
> Everything seems to be Ok!
>
> When nc->info->receive return 0, the receive_disabled will be set to 1,
> If this happened just before VM pause(is not running), after VM runstate
> change back to run, the receive_disabled is still 1, if qemu want to
> send packets to VM, it will always fail until something happen to
> clear it.
> So we'd better to clear it after runstate change back to run.
The same question as I replied in V1. Let's take virtio_net as an
example, when its receive() returns zero, it means its rx ring needs
refilling. And guest will kick qemu if it finishes the refilling, at
this time virtio_net_handle_rx() will call qemu_flush_queued_packets()
to clear receive_disabled.
So it looks safe to keep its value?
>
>>>
>>> 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<zhang.zhanghaili...@huawei.com>
>>> ---
>>> v2:
>>> - remove the superfluous check of nc->received_disabled
>>> ---
>>> hw/net/virtio-net.c | 4 ----
>>> include/net/net.h | 2 ++
>>> net/net.c | 31 +++++++++++++++++++++++++++++++
>>> 3 files changed, 33 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..113a37b 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -242,6 +242,28 @@ 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->info->can_receive&& !nc->info->can_receive(nc)) {
>>> + continue;
>>> + }
>>
>> Why not just using qemu_can_send_packet() here?
>
> qemu_can_send_packet will check the value of receive_disabled,
> Our purpose is to clear it, if its value is 1.
>
> May be we should check it as follow:
>
> if ((!nc->receive_disabled) || (nc->info->can_receive &&
> !nc->info->can_receive(nc)) {
> continue;
> }
> qemu_flush_queued_packets(nc);
>
>
>
>>> + qemu_flush_queued_packets(nc);
>>> + }
>>> +}
>>> +
>>> NICState *qemu_new_nic(NetClientInfo *info,
>>> NICConf *conf,
>>> const char *model,
>>> @@ -259,6 +281,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);
>>>
>>> for (i = 0; i< queues; i++) {
>>> qemu_net_client_setup(&nic->ncs[i], info, peers[i], model,
>>> name,
>>> @@ -379,6 +403,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 +477,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;
>>> + }
>>
>> I think you want "vmstart" here?
>
> NO, we want to check the change of VM runstate, not just *start*
> action.;)
>
Ok, but vmstat does not sound like a boolean value. You may just use
if(!runstate_is_running).
>>> +
>>> if (!sender->peer) {
>>> return 1;
>>> }
>>
>>
>> .
>>
>
>
