* [PATCH v3] virtio-net: prevent offloads reset on migration
@ 2019-10-10 12:11 Mikhail Sennikovsky
2019-10-10 12:11 ` Mikhail Sennikovsky
0 siblings, 1 reply; 7+ messages in thread
From: Mikhail Sennikovsky @ 2019-10-10 12:11 UTC (permalink / raw)
To: qemu-devel, jasowang, stefanha, mst, Dr. David Alan Gilbert
Cc: Mikhail Sennikovsky
Hi all,
This is the third version of the patch, implementing the approach
based on checking the VM runstate proposed by Jason Wang.
Regards,
Mikhail
Mikhail Sennikovsky (1):
virtio-net: prevent offloads reset on migration
hw/net/virtio-net.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
--
2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] virtio-net: prevent offloads reset on migration
2019-10-10 12:11 [PATCH v3] virtio-net: prevent offloads reset on migration Mikhail Sennikovsky
@ 2019-10-10 12:11 ` Mikhail Sennikovsky
2019-10-10 13:15 ` no-reply
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Mikhail Sennikovsky @ 2019-10-10 12:11 UTC (permalink / raw)
To: qemu-devel, jasowang, stefanha, mst, Dr. David Alan Gilbert
Cc: Mikhail Sennikovsky
Currently offloads disabled by guest via the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
command are not preserved on VM migration.
Instead all offloads reported by guest features (via VIRTIO_PCI_GUEST_FEATURES)
get enabled.
What happens is: first the VirtIONet::curr_guest_offloads gets restored
and offloads are getting set correctly:
#0 qemu_set_offload (nc=0x555556a11400, csum=1, tso4=0, tso6=0, ecn=0, ufo=0) at net/net.c:474
#1 virtio_net_apply_guest_offloads (n=0x555557701ca0) at hw/net/virtio-net.c:720
#2 virtio_net_post_load_device (opaque=0x555557701ca0, version_id=11) at hw/net/virtio-net.c:2334
#3 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577c80 <vmstate_virtio_net_device>, opaque=0x555557701ca0, version_id=11)
at migration/vmstate.c:168
#4 virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) at hw/virtio/virtio.c:2197
#5 virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
#6 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at migration/vmstate.c:143
#7 vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at migration/savevm.c:829
#8 qemu_loadvm_section_start_full (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2211
#9 qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2395
#10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
#11 process_incoming_migration_co (opaque=0x0) at migration/migration.c:449
However later on the features are getting restored, and offloads get reset to
everything supported by features:
#0 qemu_set_offload (nc=0x555556a11400, csum=1, tso4=1, tso6=1, ecn=0, ufo=0) at net/net.c:474
#1 virtio_net_apply_guest_offloads (n=0x555557701ca0) at hw/net/virtio-net.c:720
#2 virtio_net_set_features (vdev=0x555557701ca0, features=5104441767) at hw/net/virtio-net.c:773
#3 virtio_set_features_nocheck (vdev=0x555557701ca0, val=5104441767) at hw/virtio/virtio.c:2052
#4 virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) at hw/virtio/virtio.c:2220
#5 virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
#6 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at migration/vmstate.c:143
#7 vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at migration/savevm.c:829
#8 qemu_loadvm_section_start_full (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2211
#9 qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2395
#10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
#11 process_incoming_migration_co (opaque=0x0) at migration/migration.c:449
Fix this by making the virtio_net_set_features not reset
the curr_guest_offloads in case the VM runstate is RUN_STATE_INMIGRATE
Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>
---
hw/net/virtio-net.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index b9e1cd7..49be172 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -767,7 +767,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
- if (n->has_vnet_hdr) {
+ /*
+ * In case of RUN_STATE_INMIGRATE the virtio_net_set_features
+ * is called as part of VM state restore process.
+ * At this stage we do not want the curr_guest_offloads to be reset,
+ * i.e. want to preserve them in the same state as was set
+ * by the guest on the source machine.
+ */
+ if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
n->curr_guest_offloads =
virtio_net_guest_offloads_by_features(features);
virtio_net_apply_guest_offloads(n);
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: prevent offloads reset on migration
2019-10-10 12:11 ` Mikhail Sennikovsky
@ 2019-10-10 13:15 ` no-reply
2019-10-10 13:17 ` no-reply
2019-10-10 15:04 ` Michael S. Tsirkin
2 siblings, 0 replies; 7+ messages in thread
From: no-reply @ 2019-10-10 13:15 UTC (permalink / raw)
To: mikhail.sennikovskii
Cc: mst, jasowang, dgilbert, qemu-devel, mikhail.sennikovskii, stefanha
Patchew URL: https://patchew.org/QEMU/1570709475-32073-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC x86_64-softmmu/hw/timer/mc146818rtc.o
CC aarch64-softmmu/hw/char/virtio-serial-bus.o
/tmp/qemu-test/src/hw/net/virtio-net.c: In function 'virtio_net_set_features':
/tmp/qemu-test/src/hw/net/virtio-net.c:780:5: error: implicit declaration of function 'runstate_check' [-Werror=implicit-function-declaration]
if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
^
/tmp/qemu-test/src/hw/net/virtio-net.c:780:5: error: nested extern declaration of 'runstate_check' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: error: 'RUN_STATE_INMIGRATE' undeclared (first use in this function)
if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
^
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
make[1]: *** [hw/net/virtio-net.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC aarch64-softmmu/hw/core/machine-qmp-cmds.o
CC aarch64-softmmu/hw/core/numa.o
---
CC aarch64-softmmu/hw/dma/pxa2xx_dma.o
CC aarch64-softmmu/hw/gpio/omap_gpio.o
CC aarch64-softmmu/hw/gpio/imx_gpio.o
make: *** [x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
CC aarch64-softmmu/hw/gpio/bcm2835_gpio.o
CC aarch64-softmmu/hw/gpio/nrf51_gpio.o
---
CC aarch64-softmmu/hw/arm/highbank.o
CC aarch64-softmmu/hw/arm/integratorcp.o
/tmp/qemu-test/src/hw/net/virtio-net.c: In function 'virtio_net_set_features':
/tmp/qemu-test/src/hw/net/virtio-net.c:780:5: error: implicit declaration of function 'runstate_check' [-Werror=implicit-function-declaration]
if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
^
/tmp/qemu-test/src/hw/net/virtio-net.c:780:5: error: nested extern declaration of 'runstate_check' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: error: 'RUN_STATE_INMIGRATE' undeclared (first use in this function)
if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
^
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
make[1]: *** [hw/net/virtio-net.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC aarch64-softmmu/hw/arm/mainstone.o
make: *** [aarch64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 662, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=119d37d80ed3480586be3499dfe1868e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-4idbdkch/src/docker-src.2019-10-10-09.12.40.12656:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=119d37d80ed3480586be3499dfe1868e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-4idbdkch/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 2m22.220s
user 0m8.256s
The full log is available at
http://patchew.org/logs/1570709475-32073-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: prevent offloads reset on migration
2019-10-10 12:11 ` Mikhail Sennikovsky
2019-10-10 13:15 ` no-reply
@ 2019-10-10 13:17 ` no-reply
2019-10-10 15:04 ` Michael S. Tsirkin
2 siblings, 0 replies; 7+ messages in thread
From: no-reply @ 2019-10-10 13:17 UTC (permalink / raw)
To: mikhail.sennikovskii
Cc: mst, jasowang, dgilbert, qemu-devel, mikhail.sennikovskii, stefanha
Patchew URL: https://patchew.org/QEMU/1570709475-32073-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC x86_64-softmmu/hw/i386/acpi-build.o
CC x86_64-softmmu/monitor/misc.o
/tmp/qemu-test/src/hw/net/virtio-net.c: In function 'virtio_net_set_features':
/tmp/qemu-test/src/hw/net/virtio-net.c:780:29: error: implicit declaration of function 'runstate_check'; did you mean 'type_check'? [-Werror=implicit-function-declaration]
if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
^~~~~~~~~~~~~~
type_check
/tmp/qemu-test/src/hw/net/virtio-net.c:780:29: error: nested extern declaration of 'runstate_check' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: error: 'RUN_STATE_INMIGRATE' undeclared (first use in this function); did you mean 'RX_STATE__MAX'?
if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
^~~~~~~~~~~~~~~~~~~
RX_STATE__MAX
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
CC aarch64-softmmu/hw/input/pxa2xx_keypad.o
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/net/virtio-net.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC x86_64-softmmu/qapi/qapi-introspect.o
CC aarch64-softmmu/hw/input/tsc210x.o
---
CC aarch64-softmmu/hw/arm/fsl-imx6ul.o
CC aarch64-softmmu/hw/arm/mcimx6ul-evk.o
/tmp/qemu-test/src/hw/net/virtio-net.c: In function 'virtio_net_set_features':
/tmp/qemu-test/src/hw/net/virtio-net.c:780:29: error: implicit declaration of function 'runstate_check'; did you mean 'type_check'? [-Werror=implicit-function-declaration]
if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
^~~~~~~~~~~~~~
type_check
/tmp/qemu-test/src/hw/net/virtio-net.c:780:29: error: nested extern declaration of 'runstate_check' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: error: 'RUN_STATE_INMIGRATE' undeclared (first use in this function); did you mean 'RX_STATE__MAX'?
if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
^~~~~~~~~~~~~~~~~~~
RX_STATE__MAX
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/net/virtio-net.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC aarch64-softmmu/hw/arm/nrf51_soc.o
make: *** [Makefile:482: x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
make: *** [Makefile:482: aarch64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 662, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=13f31a23cdaa43fab126428102954053', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-8i4i6k8j/src/docker-src.2019-10-10-09.15.36.22582:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=13f31a23cdaa43fab126428102954053
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-8i4i6k8j/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 2m13.813s
user 0m8.487s
The full log is available at
http://patchew.org/logs/1570709475-32073-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: prevent offloads reset on migration
2019-10-10 12:11 ` Mikhail Sennikovsky
2019-10-10 13:15 ` no-reply
2019-10-10 13:17 ` no-reply
@ 2019-10-10 15:04 ` Michael S. Tsirkin
2019-10-10 15:58 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2019-10-10 15:04 UTC (permalink / raw)
To: Mikhail Sennikovsky
Cc: jasowang, qemu-devel, stefanha, Dr. David Alan Gilbert
On Thu, Oct 10, 2019 at 02:11:15PM +0200, Mikhail Sennikovsky wrote:
> Currently offloads disabled by guest via the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> command are not preserved on VM migration.
> Instead all offloads reported by guest features (via VIRTIO_PCI_GUEST_FEATURES)
> get enabled.
> What happens is: first the VirtIONet::curr_guest_offloads gets restored
> and offloads are getting set correctly:
>
> #0 qemu_set_offload (nc=0x555556a11400, csum=1, tso4=0, tso6=0, ecn=0, ufo=0) at net/net.c:474
> #1 virtio_net_apply_guest_offloads (n=0x555557701ca0) at hw/net/virtio-net.c:720
> #2 virtio_net_post_load_device (opaque=0x555557701ca0, version_id=11) at hw/net/virtio-net.c:2334
> #3 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577c80 <vmstate_virtio_net_device>, opaque=0x555557701ca0, version_id=11)
> at migration/vmstate.c:168
> #4 virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) at hw/virtio/virtio.c:2197
> #5 virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
> #6 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at migration/vmstate.c:143
> #7 vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at migration/savevm.c:829
> #8 qemu_loadvm_section_start_full (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2211
> #9 qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2395
> #10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
> #11 process_incoming_migration_co (opaque=0x0) at migration/migration.c:449
>
> However later on the features are getting restored, and offloads get reset to
> everything supported by features:
>
> #0 qemu_set_offload (nc=0x555556a11400, csum=1, tso4=1, tso6=1, ecn=0, ufo=0) at net/net.c:474
> #1 virtio_net_apply_guest_offloads (n=0x555557701ca0) at hw/net/virtio-net.c:720
> #2 virtio_net_set_features (vdev=0x555557701ca0, features=5104441767) at hw/net/virtio-net.c:773
> #3 virtio_set_features_nocheck (vdev=0x555557701ca0, val=5104441767) at hw/virtio/virtio.c:2052
> #4 virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) at hw/virtio/virtio.c:2220
> #5 virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
> #6 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at migration/vmstate.c:143
> #7 vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at migration/savevm.c:829
> #8 qemu_loadvm_section_start_full (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2211
> #9 qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2395
> #10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
> #11 process_incoming_migration_co (opaque=0x0) at migration/migration.c:449
>
> Fix this by making the virtio_net_set_features not reset
> the curr_guest_offloads in case the VM runstate is RUN_STATE_INMIGRATE
>
> Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>
I thought more about it and I really think the problem here is
different: we really should first set features and then apply offloads.
It is simply dumb that we are calling device specific
code before initializing generic virtio.
The logical order of initialization should be:
init base class
init derived class
instead we have it all spread out :(
What to do about it?
Could we maybe teach vmstate to record post load hooks
it needs to run *somewhere*?
Then we could load device specific state and record post load
hooks, then load generic state, run generic post load,
then run device specific post load hooks.
Dave, what do you think?
> ---
> hw/net/virtio-net.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index b9e1cd7..49be172 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -767,7 +767,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
> virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
>
> - if (n->has_vnet_hdr) {
> + /*
> + * In case of RUN_STATE_INMIGRATE the virtio_net_set_features
> + * is called as part of VM state restore process.
> + * At this stage we do not want the curr_guest_offloads to be reset,
> + * i.e. want to preserve them in the same state as was set
> + * by the guest on the source machine.
> + */
> + if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
> n->curr_guest_offloads =
> virtio_net_guest_offloads_by_features(features);
> virtio_net_apply_guest_offloads(n);
> --
> 2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: prevent offloads reset on migration
2019-10-10 15:04 ` Michael S. Tsirkin
@ 2019-10-10 15:58 ` Dr. David Alan Gilbert
2019-10-10 17:46 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-10 15:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, Mikhail Sennikovsky, qemu-devel, stefanha
* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Thu, Oct 10, 2019 at 02:11:15PM +0200, Mikhail Sennikovsky wrote:
> > Currently offloads disabled by guest via the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > command are not preserved on VM migration.
> > Instead all offloads reported by guest features (via VIRTIO_PCI_GUEST_FEATURES)
> > get enabled.
> > What happens is: first the VirtIONet::curr_guest_offloads gets restored
> > and offloads are getting set correctly:
> >
> > #0 qemu_set_offload (nc=0x555556a11400, csum=1, tso4=0, tso6=0, ecn=0, ufo=0) at net/net.c:474
> > #1 virtio_net_apply_guest_offloads (n=0x555557701ca0) at hw/net/virtio-net.c:720
> > #2 virtio_net_post_load_device (opaque=0x555557701ca0, version_id=11) at hw/net/virtio-net.c:2334
> > #3 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577c80 <vmstate_virtio_net_device>, opaque=0x555557701ca0, version_id=11)
> > at migration/vmstate.c:168
> > #4 virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) at hw/virtio/virtio.c:2197
> > #5 virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
> > #6 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at migration/vmstate.c:143
> > #7 vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at migration/savevm.c:829
> > #8 qemu_loadvm_section_start_full (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2211
> > #9 qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2395
> > #10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
> > #11 process_incoming_migration_co (opaque=0x0) at migration/migration.c:449
> >
> > However later on the features are getting restored, and offloads get reset to
> > everything supported by features:
> >
> > #0 qemu_set_offload (nc=0x555556a11400, csum=1, tso4=1, tso6=1, ecn=0, ufo=0) at net/net.c:474
> > #1 virtio_net_apply_guest_offloads (n=0x555557701ca0) at hw/net/virtio-net.c:720
> > #2 virtio_net_set_features (vdev=0x555557701ca0, features=5104441767) at hw/net/virtio-net.c:773
> > #3 virtio_set_features_nocheck (vdev=0x555557701ca0, val=5104441767) at hw/virtio/virtio.c:2052
> > #4 virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) at hw/virtio/virtio.c:2220
> > #5 virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
> > #6 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at migration/vmstate.c:143
> > #7 vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at migration/savevm.c:829
> > #8 qemu_loadvm_section_start_full (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2211
> > #9 qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2395
> > #10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
> > #11 process_incoming_migration_co (opaque=0x0) at migration/migration.c:449
> >
> > Fix this by making the virtio_net_set_features not reset
> > the curr_guest_offloads in case the VM runstate is RUN_STATE_INMIGRATE
> >
> > Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>
>
> I thought more about it and I really think the problem here is
> different: we really should first set features and then apply offloads.
>
> It is simply dumb that we are calling device specific
> code before initializing generic virtio.
>
> The logical order of initialization should be:
>
> init base class
> init derived class
>
>
> instead we have it all spread out :(
Yeh; we've hit problems there before.
>
> What to do about it?
> Could we maybe teach vmstate to record post load hooks
> it needs to run *somewhere*?
> Then we could load device specific state and record post load
> hooks, then load generic state, run generic post load,
> then run device specific post load hooks.
>
> Dave, what do you think?
I worry about changing all existing virtio devices; who knows
what existing ordering constraints those devices might have on their
post_load's - for example one field in the vmstate might rely on
another post_load having already run and set a flag; that's not
that unusual.
I can imagine having a Notifier* chain; you initialise it at the start
of virtio_load, provide a virtio_load_post_notifier_add(...) and any
post_load that wants delaying can add itself and then you call it
at the right point in virtio_load; but I'd be too scared to change
the ordering of all post_loads of all devices. You might be
OK to do that for the top level post_load of a virtio device,
because there are only a few to check.
Dave
>
>
> > ---
> > hw/net/virtio-net.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index b9e1cd7..49be172 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -767,7 +767,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> > n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
> > virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
> >
> > - if (n->has_vnet_hdr) {
> > + /*
> > + * In case of RUN_STATE_INMIGRATE the virtio_net_set_features
> > + * is called as part of VM state restore process.
> > + * At this stage we do not want the curr_guest_offloads to be reset,
> > + * i.e. want to preserve them in the same state as was set
> > + * by the guest on the source machine.
> > + */
> > + if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
> > n->curr_guest_offloads =
> > virtio_net_guest_offloads_by_features(features);
> > virtio_net_apply_guest_offloads(n);
> > --
> > 2.7.4
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: prevent offloads reset on migration
2019-10-10 15:58 ` Dr. David Alan Gilbert
@ 2019-10-10 17:46 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2019-10-10 17:46 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: jasowang, Mikhail Sennikovsky, qemu-devel, stefanha
On Thu, Oct 10, 2019 at 04:58:09PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Thu, Oct 10, 2019 at 02:11:15PM +0200, Mikhail Sennikovsky wrote:
> > > Currently offloads disabled by guest via the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > > command are not preserved on VM migration.
> > > Instead all offloads reported by guest features (via VIRTIO_PCI_GUEST_FEATURES)
> > > get enabled.
> > > What happens is: first the VirtIONet::curr_guest_offloads gets restored
> > > and offloads are getting set correctly:
> > >
> > > #0 qemu_set_offload (nc=0x555556a11400, csum=1, tso4=0, tso6=0, ecn=0, ufo=0) at net/net.c:474
> > > #1 virtio_net_apply_guest_offloads (n=0x555557701ca0) at hw/net/virtio-net.c:720
> > > #2 virtio_net_post_load_device (opaque=0x555557701ca0, version_id=11) at hw/net/virtio-net.c:2334
> > > #3 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577c80 <vmstate_virtio_net_device>, opaque=0x555557701ca0, version_id=11)
> > > at migration/vmstate.c:168
> > > #4 virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) at hw/virtio/virtio.c:2197
> > > #5 virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
> > > #6 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at migration/vmstate.c:143
> > > #7 vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at migration/savevm.c:829
> > > #8 qemu_loadvm_section_start_full (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2211
> > > #9 qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2395
> > > #10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
> > > #11 process_incoming_migration_co (opaque=0x0) at migration/migration.c:449
> > >
> > > However later on the features are getting restored, and offloads get reset to
> > > everything supported by features:
> > >
> > > #0 qemu_set_offload (nc=0x555556a11400, csum=1, tso4=1, tso6=1, ecn=0, ufo=0) at net/net.c:474
> > > #1 virtio_net_apply_guest_offloads (n=0x555557701ca0) at hw/net/virtio-net.c:720
> > > #2 virtio_net_set_features (vdev=0x555557701ca0, features=5104441767) at hw/net/virtio-net.c:773
> > > #3 virtio_set_features_nocheck (vdev=0x555557701ca0, val=5104441767) at hw/virtio/virtio.c:2052
> > > #4 virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) at hw/virtio/virtio.c:2220
> > > #5 virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
> > > #6 vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at migration/vmstate.c:143
> > > #7 vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at migration/savevm.c:829
> > > #8 qemu_loadvm_section_start_full (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2211
> > > #9 qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at migration/savevm.c:2395
> > > #10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
> > > #11 process_incoming_migration_co (opaque=0x0) at migration/migration.c:449
> > >
> > > Fix this by making the virtio_net_set_features not reset
> > > the curr_guest_offloads in case the VM runstate is RUN_STATE_INMIGRATE
> > >
> > > Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>
> >
> > I thought more about it and I really think the problem here is
> > different: we really should first set features and then apply offloads.
> >
> > It is simply dumb that we are calling device specific
> > code before initializing generic virtio.
> >
> > The logical order of initialization should be:
> >
> > init base class
> > init derived class
> >
> >
> > instead we have it all spread out :(
>
> Yeh; we've hit problems there before.
>
> >
> > What to do about it?
> > Could we maybe teach vmstate to record post load hooks
> > it needs to run *somewhere*?
> > Then we could load device specific state and record post load
> > hooks, then load generic state, run generic post load,
> > then run device specific post load hooks.
> >
> > Dave, what do you think?
>
> I worry about changing all existing virtio devices; who knows
> what existing ordering constraints those devices might have on their
> post_load's - for example one field in the vmstate might rely on
> another post_load having already run and set a flag; that's not
> that unusual.
>
> I can imagine having a Notifier* chain; you initialise it at the start
> of virtio_load, provide a virtio_load_post_notifier_add(...) and any
> post_load that wants delaying can add itself and then you call it
> at the right point in virtio_load; but I'd be too scared to change
> the ordering of all post_loads of all devices. You might be
> OK to do that for the top level post_load of a virtio device,
> because there are only a few to check.
>
> Dave
Okay so let's just add a new hook for that.
I'll send a patch that does this shortly.
Mikhail, I'll send a couple of untested patches,
please try them - either ack or rework in any way you like.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-10 17:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10 12:11 [PATCH v3] virtio-net: prevent offloads reset on migration Mikhail Sennikovsky
2019-10-10 12:11 ` Mikhail Sennikovsky
2019-10-10 13:15 ` no-reply
2019-10-10 13:17 ` no-reply
2019-10-10 15:04 ` Michael S. Tsirkin
2019-10-10 15:58 ` Dr. David Alan Gilbert
2019-10-10 17:46 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).