| Message ID | 1409667790-18015-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 02, 2014 at 03:23:10PM +0100, Stefan Hajnoczi wrote: > Do not modify guest memory or devices when the guest is stopped. > Currently the netdevs still send packets while the guest is stopped if > their file descriptor was being monitored for write (e.g. the socket > write buffer filled before the guest was stopped). > > Netdevs call qemu_flush_queued_packets() when the file descriptor > becomes writable again. Don't resume packet processing when this > happens. > > Instead we flush queues when the guest resumes. > > Cc: qemu-stable@nongnu.org > Reported-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > Note this fixes the transmit side. The receive side was recently fixed in > "net: Forbid dealing with packets when VM is not running". > > net/net.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/net/net.c b/net/net.c > index 6d930ea..74ec07a 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -47,6 +47,7 @@ > # define CONFIG_NET_BRIDGE > #endif > > +static VMChangeStateEntry *net_change_state_entry; > static QTAILQ_HEAD(, NetClientState) net_clients; > > const char *host_net_devices[] = { > @@ -506,6 +507,11 @@ void qemu_purge_queued_packets(NetClientState *nc) > > void qemu_flush_queued_packets(NetClientState *nc) > { > + /* Guest memory and devices must not be modified while stopped */ > + if (!runstate_is_running()) { > + return; > + } > + > nc->receive_disabled = 0; > > if (nc->peer && nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_HUBPORT) { > @@ -1168,6 +1174,25 @@ void qmp_set_link(const char *name, bool up, Error **errp) > } > } > > +/* Kick net clients when guest resumes. If a file descriptor was monitored for > + * writing before the guest was stopped, there will be nothing monitoring it > + * right now so a kick is required to get packets flowing again. > + */ > +static void net_vm_change_state_handler(void *opaque, int running, > + RunState state) > +{ > + NetClientState *nc; > + NetClientState *tmp; > + > + if (!running) { > + return; > + } > + > + QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) { > + qemu_flush_queued_packets(nc); > + } > +} > + > void net_cleanup(void) > { > NetClientState *nc; > @@ -1183,6 +1208,8 @@ void net_cleanup(void) > qemu_del_net_client(nc); > } > } > + > + qemu_del_vm_change_state_handler(net_change_state_entry); > } > > void net_check_clients(void) > @@ -1268,6 +1295,9 @@ int net_init_clients(void) > #endif > } > > + net_change_state_entry = > + qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL); > + > QTAILQ_INIT(&net_clients); > > if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1) > -- > 1.9.3
On 09/02/2014 10:23 PM, Stefan Hajnoczi wrote: > Do not modify guest memory or devices when the guest is stopped. > Currently the netdevs still send packets while the guest is stopped if > their file descriptor was being monitored for write (e.g. the socket > write buffer filled before the guest was stopped). > > Netdevs call qemu_flush_queued_packets() when the file descriptor > becomes writable again. Don't resume packet processing when this > happens. > > Instead we flush queues when the guest resumes. > > Cc: qemu-stable@nongnu.org > Reported-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > Note this fixes the transmit side. The receive side was recently fixed in > "net: Forbid dealing with packets when VM is not running". > > net/net.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/net/net.c b/net/net.c > index 6d930ea..74ec07a 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -47,6 +47,7 @@ > # define CONFIG_NET_BRIDGE > #endif > > +static VMChangeStateEntry *net_change_state_entry; > static QTAILQ_HEAD(, NetClientState) net_clients; > > const char *host_net_devices[] = { > @@ -506,6 +507,11 @@ void qemu_purge_queued_packets(NetClientState *nc) > > void qemu_flush_queued_packets(NetClientState *nc) > { > + /* Guest memory and devices must not be modified while stopped */ > + if (!runstate_is_running()) { > + return; > + } > + Consider migration case, this will prevent sent_cb(virtio_net_tx_complete) from being called at source. Since we don't migrate queue and async_tx. This may lead a interrupt lost in destination after migration? (Looks like virtio_net is the only user that uses async sending, not sure why this is needed)
On Tue, Sep 02, 2014 at 05:25:53PM +0300, Michael S. Tsirkin wrote: > On Tue, Sep 02, 2014 at 03:23:10PM +0100, Stefan Hajnoczi wrote: > > Do not modify guest memory or devices when the guest is stopped. > > Currently the netdevs still send packets while the guest is stopped if > > their file descriptor was being monitored for write (e.g. the socket > > write buffer filled before the guest was stopped). > > > > Netdevs call qemu_flush_queued_packets() when the file descriptor > > becomes writable again. Don't resume packet processing when this > > happens. > > > > Instead we flush queues when the guest resumes. > > > > Cc: qemu-stable@nongnu.org > > Reported-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Wait a second. I see a problem here. > > > --- > > Note this fixes the transmit side. The receive side was recently fixed in > > "net: Forbid dealing with packets when VM is not running". > > > > net/net.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/net/net.c b/net/net.c > > index 6d930ea..74ec07a 100644 > > --- a/net/net.c > > +++ b/net/net.c > > @@ -47,6 +47,7 @@ > > # define CONFIG_NET_BRIDGE > > #endif > > > > +static VMChangeStateEntry *net_change_state_entry; > > static QTAILQ_HEAD(, NetClientState) net_clients; > > > > const char *host_net_devices[] = { > > @@ -506,6 +507,11 @@ void qemu_purge_queued_packets(NetClientState *nc) > > > > void qemu_flush_queued_packets(NetClientState *nc) > > { > > + /* Guest memory and devices must not be modified while stopped */ > > + if (!runstate_is_running()) { > > + return; > > + } > > + > > nc->receive_disabled = 0; > > > > if (nc->peer && nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_HUBPORT) { virtio net really assumes that after qemu_flush_queued_packets all packets have been consumed. So if we migrate, we never resume and queued packets will never be completed. If we can not flush them out, so we really must go ahead and discard queued packets. And if you do this, no need for a state handler. So NAK, sorry about noticing this late. > > @@ -1168,6 +1174,25 @@ void qmp_set_link(const char *name, bool up, Error **errp) > > } > > } > > > > +/* Kick net clients when guest resumes. If a file descriptor was monitored for > > + * writing before the guest was stopped, there will be nothing monitoring it > > + * right now so a kick is required to get packets flowing again. > > + */ > > +static void net_vm_change_state_handler(void *opaque, int running, > > + RunState state) > > +{ > > + NetClientState *nc; > > + NetClientState *tmp; > > + > > + if (!running) { > > + return; > > + } > > + > > + QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) { > > + qemu_flush_queued_packets(nc); > > + } > > +} > > + > > void net_cleanup(void) > > { > > NetClientState *nc; > > @@ -1183,6 +1208,8 @@ void net_cleanup(void) > > qemu_del_net_client(nc); > > } > > } > > + > > + qemu_del_vm_change_state_handler(net_change_state_entry); > > } > > > > void net_check_clients(void) > > @@ -1268,6 +1295,9 @@ int net_init_clients(void) > > #endif > > } > > > > + net_change_state_entry = > > + qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL); > > + > > QTAILQ_INIT(&net_clients); > > > > if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1) > > -- > > 1.9.3
On Thu, Sep 04, 2014 at 12:55:38PM +0800, Jason Wang wrote: > On 09/02/2014 10:23 PM, Stefan Hajnoczi wrote: > > Do not modify guest memory or devices when the guest is stopped. > > Currently the netdevs still send packets while the guest is stopped if > > their file descriptor was being monitored for write (e.g. the socket > > write buffer filled before the guest was stopped). > > > > Netdevs call qemu_flush_queued_packets() when the file descriptor > > becomes writable again. Don't resume packet processing when this > > happens. > > > > Instead we flush queues when the guest resumes. > > > > Cc: qemu-stable@nongnu.org > > Reported-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > Note this fixes the transmit side. The receive side was recently fixed in > > "net: Forbid dealing with packets when VM is not running". > > > > net/net.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/net/net.c b/net/net.c > > index 6d930ea..74ec07a 100644 > > --- a/net/net.c > > +++ b/net/net.c > > @@ -47,6 +47,7 @@ > > # define CONFIG_NET_BRIDGE > > #endif > > > > +static VMChangeStateEntry *net_change_state_entry; > > static QTAILQ_HEAD(, NetClientState) net_clients; > > > > const char *host_net_devices[] = { > > @@ -506,6 +507,11 @@ void qemu_purge_queued_packets(NetClientState *nc) > > > > void qemu_flush_queued_packets(NetClientState *nc) > > { > > + /* Guest memory and devices must not be modified while stopped */ > > + if (!runstate_is_running()) { > > + return; > > + } > > + > > Consider migration case, this will prevent > sent_cb(virtio_net_tx_complete) from being called at source. Since we > don't migrate queue and async_tx. This may lead a interrupt lost in > destination after migration? > > (Looks like virtio_net is the only user that uses async sending, not > sure why this is needed) Good point, I noticed this too. Sorry about missing this the first time around.
diff --git a/net/net.c b/net/net.c index 6d930ea..74ec07a 100644 --- a/net/net.c +++ b/net/net.c @@ -47,6 +47,7 @@ # define CONFIG_NET_BRIDGE #endif +static VMChangeStateEntry *net_change_state_entry; static QTAILQ_HEAD(, NetClientState) net_clients; const char *host_net_devices[] = { @@ -506,6 +507,11 @@ void qemu_purge_queued_packets(NetClientState *nc) void qemu_flush_queued_packets(NetClientState *nc) { + /* Guest memory and devices must not be modified while stopped */ + if (!runstate_is_running()) { + return; + } + nc->receive_disabled = 0; if (nc->peer && nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_HUBPORT) { @@ -1168,6 +1174,25 @@ void qmp_set_link(const char *name, bool up, Error **errp) } } +/* Kick net clients when guest resumes. If a file descriptor was monitored for + * writing before the guest was stopped, there will be nothing monitoring it + * right now so a kick is required to get packets flowing again. + */ +static void net_vm_change_state_handler(void *opaque, int running, + RunState state) +{ + NetClientState *nc; + NetClientState *tmp; + + if (!running) { + return; + } + + QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) { + qemu_flush_queued_packets(nc); + } +} + void net_cleanup(void) { NetClientState *nc; @@ -1183,6 +1208,8 @@ void net_cleanup(void) qemu_del_net_client(nc); } } + + qemu_del_vm_change_state_handler(net_change_state_entry); } void net_check_clients(void) @@ -1268,6 +1295,9 @@ int net_init_clients(void) #endif } + net_change_state_entry = + qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL); + QTAILQ_INIT(&net_clients); if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)
Do not modify guest memory or devices when the guest is stopped. Currently the netdevs still send packets while the guest is stopped if their file descriptor was being monitored for write (e.g. the socket write buffer filled before the guest was stopped). Netdevs call qemu_flush_queued_packets() when the file descriptor becomes writable again. Don't resume packet processing when this happens. Instead we flush queues when the guest resumes. Cc: qemu-stable@nongnu.org Reported-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- Note this fixes the transmit side. The receive side was recently fixed in "net: Forbid dealing with packets when VM is not running". net/net.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)