| Message ID | 1398507552-50168-1-git-send-email-zhang.zhanghailiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
I'm ok with the patch idea. On Sat, Apr 26, 2014 at 06:19:12PM +0800, zhanghailiang wrote: > For e1000/rtl8139, qemu can still send/receive packets when VM is paused. ^^^^^^^^^ -> isn't running There are many kinds of RunState, "is paused" doesn't equal to "isn't running". > If this happened in *migration's* last PAUSE VM stage, the new dirty RAM related to the packets will be missed. > To avoid this, do things like virtio-net, forbid sending/receiving > packets when VM is suspend. ^^^^^^^^^^^ -> isn't running. > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > --- > hw/net/e1000.c | 5 +++-- > hw/net/rtl8139.c | 3 +++ > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index 8387443..94108fd 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -885,9 +885,10 @@ static int > e1000_can_receive(NetClientState *nc) > { > E1000State *s = qemu_get_nic_opaque(nc); > - > + int vmstat = runstate_is_running(); > + > return (s->mac_reg[STATUS] & E1000_STATUS_LU) && > - (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, 1); > + (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, 1) && vmstat; > } > > static uint64_t rx_desc_base(E1000State *s) > diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c > index 5329f44..3c60c84 100644 > --- a/hw/net/rtl8139.c > +++ b/hw/net/rtl8139.c > @@ -799,7 +799,10 @@ static int rtl8139_can_receive(NetClientState *nc) > { > RTL8139State *s = qemu_get_nic_opaque(nc); > int avail; > + int vmstat = runstate_is_running(); > > + if (!vmstat) > + return 0; if (!runstate_is_running()) return 0; > /* Receive (drop) packets if card is disabled. */ > if (!s->clock_enabled) > return 1; > -- > 1.7.12.4 >
On 26 April 2014 11:44, Amos Kong <akong@redhat.com> wrote: > > I'm ok with the patch idea. > > On Sat, Apr 26, 2014 at 06:19:12PM +0800, zhanghailiang wrote: >> For e1000/rtl8139, qemu can still send/receive packets when VM is paused. > ^^^^^^^^^ > -> isn't running > > There are many kinds of RunState, "is paused" doesn't equal to "isn't running". > >> If this happened in *migration's* last PAUSE VM stage, the new dirty RAM related to the packets will be missed. >> To avoid this, do things like virtio-net, forbid sending/receiving >> packets when VM is suspend. > ^^^^^^^^^^^ -> isn't running. Shouldn't this be handled in the generic net code rather than requiring every ethernet device model to include identical code? thanks -- PMM
On Sat, Apr 26, 2014 at 9:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 April 2014 11:44, Amos Kong <akong@redhat.com> wrote: >> >> I'm ok with the patch idea. >> >> On Sat, Apr 26, 2014 at 06:19:12PM +0800, zhanghailiang wrote: >>> For e1000/rtl8139, qemu can still send/receive packets when VM is paused. >> ^^^^^^^^^ >> -> isn't running >> >> There are many kinds of RunState, "is paused" doesn't equal to "isn't running". >> >>> If this happened in *migration's* last PAUSE VM stage, the new dirty RAM related to the packets will be missed. >>> To avoid this, do things like virtio-net, forbid sending/receiving >>> packets when VM is suspend. >> ^^^^^^^^^^^ -> isn't running. > > Shouldn't this be handled in the generic net code rather > than requiring every ethernet device model to include > identical code? > +1. > thanks > -- PMM >
Hi Amos: Thanks for replying. > I'm ok with the patch idea. > > On Sat, Apr 26, 2014 at 06:19:12PM +0800, zhanghailiang wrote: > > For e1000/rtl8139, qemu can still send/receive packets when VM is paused. > > ^^^^^^^^^ > -> > isn't running > > There are many kinds of RunState, "is paused" doesn't equal to "isn't running". Yes, you are right, this is my fault:) > > > If this happened in *migration's* last PAUSE VM stage, the new dirty RAM > related to the packets will be missed. > > To avoid this, do things like virtio-net, forbid sending/receiving > > packets when VM is suspend. > ^^^^^^^^^^^ -> isn't running. > > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > > > --- > > hw/net/e1000.c | 5 +++-- > > hw/net/rtl8139.c | 3 +++ > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 8387443..94108fd > > 100644 > > --- a/hw/net/e1000.c > > +++ b/hw/net/e1000.c > > @@ -885,9 +885,10 @@ static int > > e1000_can_receive(NetClientState *nc) { > > E1000State *s = qemu_get_nic_opaque(nc); > > - > > + int vmstat = runstate_is_running(); > > + > > return (s->mac_reg[STATUS] & E1000_STATUS_LU) && > > - (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, > 1); > > + (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, > 1) > > + && vmstat; > > } > > > > static uint64_t rx_desc_base(E1000State *s) diff --git > > a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 5329f44..3c60c84 100644 > > --- a/hw/net/rtl8139.c > > +++ b/hw/net/rtl8139.c > > @@ -799,7 +799,10 @@ static int rtl8139_can_receive(NetClientState > > *nc) { > > RTL8139State *s = qemu_get_nic_opaque(nc); > > int avail; > > + int vmstat = runstate_is_running(); > > > > + if (!vmstat) > > + return 0; > > if (!runstate_is_running()) > return 0; > > > /* Receive (drop) packets if card is disabled. */ > > if (!s->clock_enabled) > > return 1; > > -- > > 1.7.12.4 > > > > -- > Amos.
> On Sat, Apr 26, 2014 at 9:04 PM, Peter Maydell <peter.maydell@linaro.org> > wrote: > > On 26 April 2014 11:44, Amos Kong <akong@redhat.com> wrote: > >> > >> I'm ok with the patch idea. > >> > >> On Sat, Apr 26, 2014 at 06:19:12PM +0800, zhanghailiang wrote: > >>> For e1000/rtl8139, qemu can still send/receive packets when VM is > paused. > >> > ^^^^^^^^^ > >> -> > isn't > >> running > >> > >> There are many kinds of RunState, "is paused" doesn't equal to "isn't > running". > >> > >>> If this happened in *migration's* last PAUSE VM stage, the new dirty RAM > related to the packets will be missed. > >>> To avoid this, do things like virtio-net, forbid sending/receiving > >>> packets when VM is suspend. > >> ^^^^^^^^^^^ -> isn't running. > > > > Shouldn't this be handled in the generic net code rather than > > requiring every ethernet device model to include identical code? > > > > +1. > > > thanks > > -- PMM > > Hi, Thanks for replying. It is a good idea to handle the VM runstate check in generic net code. Before send such a patch, I will look into other network cards emulated by qemu, and test if they have the same problem. Best regards, ZhangHailiang
On Mon, Apr 28, 2014 at 03:22:34AM +0000, Zhanghailiang wrote: > > On Sat, Apr 26, 2014 at 9:04 PM, Peter Maydell <peter.maydell@linaro.org> > > wrote: > > > On 26 April 2014 11:44, Amos Kong <akong@redhat.com> wrote: > > >> > > >> I'm ok with the patch idea. > > >> > > >> On Sat, Apr 26, 2014 at 06:19:12PM +0800, zhanghailiang wrote: > > >>> For e1000/rtl8139, qemu can still send/receive packets when VM is > > paused. > > >> > > ^^^^^^^^^ > > >> -> > > isn't > > >> running > > >> > > >> There are many kinds of RunState, "is paused" doesn't equal to "isn't > > running". > > >> > > >>> If this happened in *migration's* last PAUSE VM stage, the new dirty RAM > > related to the packets will be missed. > > >>> To avoid this, do things like virtio-net, forbid sending/receiving > > >>> packets when VM is suspend. > > >> ^^^^^^^^^^^ -> isn't running. > > > > > > Shouldn't this be handled in the generic net code rather than > > > requiring every ethernet device model to include identical code? > > > > > > > +1. > > > > > thanks > > > -- PMM > > > > Hi, > Thanks for replying. > It is a good idea to handle the VM runstate check in generic net code. > Before send such a patch, I will look into other network cards emulated by qemu, and test if they have the same problem. Yes, please! virtio-net has a partial solution, it listens for VM runstate changes. But it's a bit buggy because it does not flush the peer's queue when runstate changes back to running. If you implement this in the net layer then that problem is easy to resolve since we can flush all queues when the guest resumes to get packets flowing again. Stefan
Hi Stefan, I have test other network cards, such as pcnet, ne2000, and they also have the problem. > On Mon, Apr 28, 2014 at 03:22:34AM +0000, Zhanghailiang wrote: > > > On Sat, Apr 26, 2014 at 9:04 PM, Peter Maydell > > > <peter.maydell@linaro.org> > > > wrote: > > > > On 26 April 2014 11:44, Amos Kong <akong@redhat.com> wrote: > > > >> > > > >> I'm ok with the patch idea. > > > >> > > > >> On Sat, Apr 26, 2014 at 06:19:12PM +0800, zhanghailiang wrote: > > > >>> For e1000/rtl8139, qemu can still send/receive packets when VM > > > >>> is > > > paused. > > > >> > > > ^^^^^^^^^ > > > >> > -> > > > isn't > > > >> running > > > >> > > > >> There are many kinds of RunState, "is paused" doesn't equal to > > > >> "isn't > > > running". > > > >> > > > >>> If this happened in *migration's* last PAUSE VM stage, the new > > > >>> dirty RAM > > > related to the packets will be missed. > > > >>> To avoid this, do things like virtio-net, forbid > > > >>> sending/receiving packets when VM is suspend. > > > >> ^^^^^^^^^^^ -> isn't running. > > > > > > > > Shouldn't this be handled in the generic net code rather than > > > > requiring every ethernet device model to include identical code? > > > > > > > > > > +1. > > > > > > > thanks > > > > -- PMM > > > > > > Hi, > > Thanks for replying. > > It is a good idea to handle the VM runstate check in generic net code. > > Before send such a patch, I will look into other network cards emulated by > qemu, and test if they have the same problem. > > Yes, please! > > virtio-net has a partial solution, it listens for VM runstate changes. > But it's a bit buggy because it does not flush the peer's queue when runstate > changes back to running. Agreed! > If you implement this in the net layer then that problem is easy to resolve since > we can flush all queues when the guest resumes to get packets flowing again. > Do you mean we should also listen for VM runstate changes in net layer, and when detect runstate changes back to running , we flush all queues actively? Am I misunderstanding? Or we can do it *before* qemu (exactly when it check if it can send packets) send packets to guest again, this way will be simple, but it also need know the change of runstate. Any idea? Thanks, zhanghailiang
On Thu, May 08, 2014 at 12:51:05PM +0000, Zhanghailiang wrote: > > If you implement this in the net layer then that problem is easy to resolve since > > we can flush all queues when the guest resumes to get packets flowing again. > > > Do you mean we should also listen for VM runstate changes in net layer, and when detect runstate changes back to running , we flush all queues actively? Am I misunderstanding? > Or we can do it *before* qemu (exactly when it check if it can send packets) send packets to guest again, this way will be simple, but it also need know the change of runstate. Any idea? When the runstate changes back to running, we definitely need to flush queues to get packets flowing again. I think the simplest way of doing that is in the net layer so individual NICs and netdevs don't have to duplicate this code. Stefan
On Wed, May 14, 2014 at 02:30:26PM +0200, Stefan Hajnoczi wrote: > On Thu, May 08, 2014 at 12:51:05PM +0000, Zhanghailiang wrote: > > > If you implement this in the net layer then that problem is easy to resolve since > > > we can flush all queues when the guest resumes to get packets flowing again. > > > > > Do you mean we should also listen for VM runstate changes in net layer, and when detect runstate changes back to running , we flush all queues actively? Am I misunderstanding? > > Or we can do it *before* qemu (exactly when it check if it can send packets) send packets to guest again, this way will be simple, but it also need know the change of runstate. Any idea? > > When the runstate changes back to running, we definitely need to flush > queues to get packets flowing again. I think the simplest way of doing > that is in the net layer so individual NICs and netdevs don't have to > duplicate this code. > > Stefan That will help with networking but not other devices. The issue isn't limited to networking at all. How about we stop all io threads with the vm? That will address the issue in a generic way.
On Wed, May 14, 2014 at 03:46:48PM +0300, Michael S. Tsirkin wrote: > On Wed, May 14, 2014 at 02:30:26PM +0200, Stefan Hajnoczi wrote: > > On Thu, May 08, 2014 at 12:51:05PM +0000, Zhanghailiang wrote: > > > > If you implement this in the net layer then that problem is easy to resolve since > > > > we can flush all queues when the guest resumes to get packets flowing again. > > > > > > > Do you mean we should also listen for VM runstate changes in net layer, and when detect runstate changes back to running , we flush all queues actively? Am I misunderstanding? > > > Or we can do it *before* qemu (exactly when it check if it can send packets) send packets to guest again, this way will be simple, but it also need know the change of runstate. Any idea? > > > > When the runstate changes back to running, we definitely need to flush > > queues to get packets flowing again. I think the simplest way of doing > > that is in the net layer so individual NICs and netdevs don't have to > > duplicate this code. > > > > Stefan > > That will help with networking but not other devices. > The issue isn't limited to networking at all. > How about we stop all io threads with the vm? > > That will address the issue in a generic way. I'm not sure if it works in all cases, for example iSCSI where we send nop keepalives. Stefan
On Mon, May 26, 2014 at 01:48:13PM +0200, Stefan Hajnoczi wrote: > On Wed, May 14, 2014 at 03:46:48PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 14, 2014 at 02:30:26PM +0200, Stefan Hajnoczi wrote: > > > On Thu, May 08, 2014 at 12:51:05PM +0000, Zhanghailiang wrote: > > > > > If you implement this in the net layer then that problem is easy to resolve since > > > > > we can flush all queues when the guest resumes to get packets flowing again. > > > > > > > > > Do you mean we should also listen for VM runstate changes in net layer, and when detect runstate changes back to running , we flush all queues actively? Am I misunderstanding? > > > > Or we can do it *before* qemu (exactly when it check if it can send packets) send packets to guest again, this way will be simple, but it also need know the change of runstate. Any idea? > > > > > > When the runstate changes back to running, we definitely need to flush > > > queues to get packets flowing again. I think the simplest way of doing > > > that is in the net layer so individual NICs and netdevs don't have to > > > duplicate this code. > > > > > > Stefan > > > > That will help with networking but not other devices. > > The issue isn't limited to networking at all. > > How about we stop all io threads with the vm? > > > > That will address the issue in a generic way. > > I'm not sure if it works in all cases, for example iSCSI where we send > nop keepalives. > > Stefan I am guessing that runs from the realtime clock? We definitely want to keep realtime clock going when VM is stopped, that's the definition.
On Mon, May 26, 2014 at 02:51:48PM +0300, Michael S. Tsirkin wrote: > On Mon, May 26, 2014 at 01:48:13PM +0200, Stefan Hajnoczi wrote: > > On Wed, May 14, 2014 at 03:46:48PM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 14, 2014 at 02:30:26PM +0200, Stefan Hajnoczi wrote: > > > > On Thu, May 08, 2014 at 12:51:05PM +0000, Zhanghailiang wrote: > > > > > > If you implement this in the net layer then that problem is easy to resolve since > > > > > > we can flush all queues when the guest resumes to get packets flowing again. > > > > > > > > > > > Do you mean we should also listen for VM runstate changes in net layer, and when detect runstate changes back to running , we flush all queues actively? Am I misunderstanding? > > > > > Or we can do it *before* qemu (exactly when it check if it can send packets) send packets to guest again, this way will be simple, but it also need know the change of runstate. Any idea? > > > > > > > > When the runstate changes back to running, we definitely need to flush > > > > queues to get packets flowing again. I think the simplest way of doing > > > > that is in the net layer so individual NICs and netdevs don't have to > > > > duplicate this code. > > > > > > > > Stefan > > > > > > That will help with networking but not other devices. > > > The issue isn't limited to networking at all. > > > How about we stop all io threads with the vm? > > > > > > That will address the issue in a generic way. > > > > I'm not sure if it works in all cases, for example iSCSI where we send > > nop keepalives. > > > > Stefan > > I am guessing that runs from the realtime clock? > We definitely want to keep realtime clock going when VM > is stopped, that's the definition. Yes. And this is a random example I picked but there are probably more cases. I think stopping event loops could be a good idea, but we need to audit the code carefully and probably introduce callbacks to pause things cleanly instead of just suspending threads. Stefan
diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 8387443..94108fd 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -885,9 +885,10 @@ static int e1000_can_receive(NetClientState *nc) { E1000State *s = qemu_get_nic_opaque(nc); - + int vmstat = runstate_is_running(); + return (s->mac_reg[STATUS] & E1000_STATUS_LU) && - (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, 1); + (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, 1) && vmstat; } static uint64_t rx_desc_base(E1000State *s) diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 5329f44..3c60c84 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -799,7 +799,10 @@ static int rtl8139_can_receive(NetClientState *nc) { RTL8139State *s = qemu_get_nic_opaque(nc); int avail; + int vmstat = runstate_is_running(); + if (!vmstat) + return 0; /* Receive (drop) packets if card is disabled. */ if (!s->clock_enabled) return 1;
For e1000/rtl8139, qemu can still send/receive packets when VM is paused. If this happened in *migration's* last PAUSE VM stage, the new dirty RAM related to the packets will be missed. To avoid this, do things like virtio-net, forbid sending/receiving packets when VM is suspend. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> --- hw/net/e1000.c | 5 +++-- hw/net/rtl8139.c | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-)