Re: [Qemu-devel] [PATCH] net: Forbid dealing with packets when VM is not running

[zhanghailiang <zhang.zhanghailiang@huawei.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<zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>    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;
>>>>        }
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>