| Message ID | 1408337205-10260-1-git-send-email-zhang.zhanghailiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
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. > > 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. > + } > +} > + > 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? > 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; > }
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. >> + } >> +} >> + >> 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; >> } > > > . >
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(). >>> + } >>> +} >>> + >>> 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. > >>> 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; >>> } >> >> >> . >> > >
* zhanghailiang (zhang.zhanghailiang@huawei.com) 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. I'd like to understand more about when exactly this happens. migration.c:migration_thread in the last step, takes the iothread, puts the runstate into RUN_STATE_FINISH_MIGRATE and only then calls qemu_savevm_state_complete before unlocking the iothread. If that's true, how does this problem happen - can the net code still receive packets with the iothread lock taken? qemu_savevm_state_complete does a call to the RAM code, so I think this should get any RAM changes that happened before the lock was taken. I'm gently worried about threading stuff as well - is all this net code running off fd handlers on the main thread or are there separate threads? Dave > 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. > > 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); > + } > +} > + > 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); > > 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; > } > -- > 1.7.12.4 > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Mon, Aug 18, 2014 at 04:32:42PM +0800, zhanghailiang wrote: > On 2014/8/18 14:55, Jason Wang wrote: > >On 08/18/2014 12:46 PM, zhanghailiang wrote: > >>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. qemu_flush_queued_packets() sets nc->received_disabled = 0. This may be needed to get packets flowing again if ->receive() previously returned 0. Purging the queue does not clear nc->received_disabled so it is not enough.
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; >>>> } >>> >>> >>> . >>> >> >> > > > . >
On 2014/8/19 20:29, Stefan Hajnoczi wrote: > On Mon, Aug 18, 2014 at 04:32:42PM +0800, zhanghailiang wrote: >> On 2014/8/18 14:55, Jason Wang wrote: >>> On 08/18/2014 12:46 PM, zhanghailiang wrote: >>>> 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. > > qemu_flush_queued_packets() sets nc->received_disabled = 0. This may be > needed to get packets flowing again if ->receive() previously returned 0. > > Purging the queue does not clear nc->received_disabled so it is not > enough. So this is the reason why we need to do flush after VM runstate come back to running again.:) Oh, In the above patch, we don't need check the nc->receive_disabled before qemu_flush_queued_packets(nc), but still need check nc->info->can_receive(nc), is it? Before send another patch, I will check if this patch has side-effect when we use vhost_net. Thanks, zhanghailiang
On 08/19/2014 08:29 PM, Stefan Hajnoczi wrote: > On Mon, Aug 18, 2014 at 04:32:42PM +0800, zhanghailiang wrote: >> On 2014/8/18 14:55, Jason Wang wrote: >>> On 08/18/2014 12:46 PM, zhanghailiang wrote: >>>> 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. > qemu_flush_queued_packets() sets nc->received_disabled = 0. This may be > needed to get packets flowing again if ->receive() previously returned 0. > > Purging the queue does not clear nc->received_disabled so it is not > enough. Confused. virtio_net_receive() only returns 0 when it does not have enough rx buffers. In this case, it just wait for the guest to refill and kick again. Its rx kick handler will call qemu_flush_queued_packets() to clear nc->received_disabled. So does usbnet and others. If nic_received_disabled is 1, it means the no available rx buffer. We need wait guest to do the processing and refilling. Then why need clear it after vm was started?
On Wed, Aug 20, 2014 at 11:17:56AM +0800, Jason Wang wrote: > On 08/19/2014 08:29 PM, Stefan Hajnoczi wrote: > > On Mon, Aug 18, 2014 at 04:32:42PM +0800, zhanghailiang wrote: > >> On 2014/8/18 14:55, Jason Wang wrote: > >>> On 08/18/2014 12:46 PM, zhanghailiang wrote: > >>>> 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. > > qemu_flush_queued_packets() sets nc->received_disabled = 0. This may be > > needed to get packets flowing again if ->receive() previously returned 0. > > > > Purging the queue does not clear nc->received_disabled so it is not > > enough. > > Confused. > > virtio_net_receive() only returns 0 when it does not have enough rx > buffers. In this case, it just wait for the guest to refill and kick > again. Its rx kick handler will call qemu_flush_queued_packets() to > clear nc->received_disabled. So does usbnet and others. > > If nic_received_disabled is 1, it means the no available rx buffer. We > need wait guest to do the processing and refilling. Then why need clear > it after vm was started? I took a look at other emulated NICs, they don't use return 0 in ->receive(). I think you are right, we don't need to worry about flushing. Stefan
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); + } +} + 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); 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; }
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. 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(-)