Re: [PULL 00/30] Next patches
On 6/22/23 18:54, Juan Quintela wrote: The following changes since commit b455ce4c2f300c8ba47cba7232dd03261368a4cb: Merge tag 'q800-for-8.1-pull-request' ofhttps://github.com/vivier/qemu-m68k into staging (2023-06-22 10:18:32 +0200) are available in the Git repository at: https://gitlab.com/juan.quintela/qemu.git tags/next-pull-request for you to fetch changes up to 23e4307eadc1497bd0a11ca91041768f15963b68: migration/rdma: Split qemu_fopen_rdma() into input/output functions (2023-06-22 18:11:58 +0200) Migration Pull request (20230621) take 2 In this pull request the only change is fixing 32 bits complitaion issue. Please apply. [take 1] - fix for multifd thread creation (fabiano) - dirtylimity (hyman) * migration-test will go on next PULL request, as it has failures. - Improve error description (tejus) - improve -incoming and set parameters before calling incoming (wei) - migration atomic counters reviewed patches (quintela) - migration-test refacttoring reviewed (quintela) New failure with check-cfi-x86_64: https://gitlab.com/qemu-project/qemu/-/jobs/4527202764#L188 /builds/qemu-project/qemu/build/pyvenv/bin/meson test --no-rebuild -t 0 --num-processes 1 --print-errorlogs 1/350 qemu:qtest+qtest-x86_64 / qtest-x86_64/qom-test OK 6.55s 8 subtests passed ▶ 2/350 ERROR:../tests/qtest/migration-test.c:320:check_guests_ram: assertion failed: (bad == 0) ERROR 2/350 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test ERROR 151.99s killed by signal 6 SIGABRT >>> G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh MALLOC_PERTURB_=3 QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_BINARY=./qemu-system-x86_64 /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k ― ✀ ― stderr: qemu-system-x86_64: Unable to read from socket: Connection reset by peer Memory content inconsistency at 4f65000 first_byte = 30 last_byte = 2f current = 88 hit_edge = 1 ** ERROR:../tests/qtest/migration-test.c:320:check_guests_ram: assertion failed: (bad == 0) (test program exited with status code -6) ―― r~
Re: [PATCH RFC 0/6] Switch iotests to pyvenv
Il gio 22 giu 2023, 23:18 John Snow ha scritto: > Possibly I could teach mkvenv a new trick, like "mkvenv init iotests" > and have the mkvenv script DTRT at that point, whatever that is -- > ideally exiting very quickly without doing anything. > Or maybe check itself should do the bootstrap if it's invoked from the venv?!? Paolo >
Re: [PATCH RFC 0/6] Switch iotests to pyvenv
On Thu, Jun 22, 2023 at 5:12 PM Paolo Bonzini wrote:
>
> On Thu, Jun 22, 2023 at 11:08 PM John Snow wrote:
> >
> > On Thu, Jun 22, 2023 at 5:05 PM Paolo Bonzini wrote:
> > >
> > > On Thu, Jun 22, 2023 at 11:03 PM John Snow wrote:
> > > > If we always install it in editable mode, and the path where it is
> > > > "installed" is what we expect it to be, it shouldn't have any problems
> > > > with being out of date I think. We could conceivably use the
> > > > "faux" package version the internal package has to signal when the
> > > > script needs to re-install it.
> > >
> > > Stupid question, why not treat it just like avocado?
> > >
> >
> > How do you mean? (i.e. installing it on-demand in reaction to "make
> > check-avocado"?)
>
> Yes, installing it on-demand the first time "make check-iotests" is
> run, using a "depend:" keyword argument in
> tests/qemu-iotests/meson.build.
>
> BTW,
>
> from distlib.scripts import ScriptMaker
> ScriptMaker('..', '.').make('foo.py')
>
> Seems to do the right thing as long as foo.py includes a shebang (I
> tested it inside a virtual environment).
>
> Paolo
That's possible, but it means that it will break if you run configure
and then immediately go to invoke iotests, unless we have a way to
have iotests bootstrap itself. Which I think can't be done through the
makefile, because we don't know which "make" to run in order to get
that to happen. (Or at least, I don't!)
Possibly I could teach mkvenv a new trick, like "mkvenv init iotests"
and have the mkvenv script DTRT at that point, whatever that is --
ideally exiting very quickly without doing anything.
Re: [PATCH RFC 0/6] Switch iotests to pyvenv
On Thu, Jun 22, 2023 at 11:08 PM John Snow wrote:
>
> On Thu, Jun 22, 2023 at 5:05 PM Paolo Bonzini wrote:
> >
> > On Thu, Jun 22, 2023 at 11:03 PM John Snow wrote:
> > > If we always install it in editable mode, and the path where it is
> > > "installed" is what we expect it to be, it shouldn't have any problems
> > > with being out of date I think. We could conceivably use the
> > > "faux" package version the internal package has to signal when the
> > > script needs to re-install it.
> >
> > Stupid question, why not treat it just like avocado?
> >
>
> How do you mean? (i.e. installing it on-demand in reaction to "make
> check-avocado"?)
Yes, installing it on-demand the first time "make check-iotests" is
run, using a "depend:" keyword argument in
tests/qemu-iotests/meson.build.
BTW,
from distlib.scripts import ScriptMaker
ScriptMaker('..', '.').make('foo.py')
Seems to do the right thing as long as foo.py includes a shebang (I
tested it inside a virtual environment).
Paolo
Re: [PATCH RFC 0/6] Switch iotests to pyvenv
On Thu, Jun 22, 2023 at 5:05 PM Paolo Bonzini wrote: > > On Thu, Jun 22, 2023 at 11:03 PM John Snow wrote: > > If we always install it in editable mode, and the path where it is > > "installed" is what we expect it to be, it shouldn't have any problems > > with being out of date I think. We could conceivably use the > > "faux" package version the internal package has to signal when the > > script needs to re-install it. > > Stupid question, why not treat it just like avocado? > How do you mean? (i.e. installing it on-demand in reaction to "make check-avocado"?)
Re: [PATCH RFC 0/6] Switch iotests to pyvenv
On Thu, Jun 22, 2023 at 11:03 PM John Snow wrote: > If we always install it in editable mode, and the path where it is > "installed" is what we expect it to be, it shouldn't have any problems > with being out of date I think. We could conceivably use the > "faux" package version the internal package has to signal when the > script needs to re-install it. Stupid question, why not treat it just like avocado?
Re: [PATCH RFC 0/6] Switch iotests to pyvenv
On Thu, Jun 22, 2023 at 5:24 AM Paolo Bonzini wrote: > > On Wed, Jun 21, 2023 at 9:08 AM Paolo Bonzini wrote: > > Maybe patch 4 can use distlib.scripts as well to create the check script in > > the build directory? (Yes that's another mkvenv functionality...) On a > > phone and don't have the docs at hand, so I am not sure. If not, your > > solution is good enough. > > Yeah, that's a possibility... we could "install" the iotests script. That might keep things simple. I'll investigate it. > > Apart from this the only issue is the speed. IIRC having a prebuilt .whl > > would fix it, I think for Meson we observed that the slow part was building > > the wheel. Possibilities: > > > > 1) using --no-pep517 if that also speeds it up? > > > > 2) already removing the sources to qemu.qmp since that's the plan anyway; > > and then, if you want editability you can install the package with --user > > --editable, i.e. outside the venv > > Nope, it's 3 second always and 1.5 even with the wheel. > > Maybe replace qemu.qmp with a wheel and leaving PYTHONPATH for the rest? > > Paolo > Hm, I guess so. It's just disappointing because I was really hoping to be able to use "pip install" to handle dependencies like a normal package instead of trying to shoulder that burden with an increasing amount of custom logic that's hard for anyone but me (or you, now) to maintain. It kind of defeats the point of having formatted it as a package to begin with. Maybe there's a sane way to amortize the cost of installation by not re-creating it after every call to configure instead -- the rest of the script is fast enough, perhaps we could default clear to *False* from now on and use the _get_version() bits to detect if the local internal package is already installed or not -- and if it is, just leave it alone. If we always install it in editable mode, and the path where it is "installed" is what we expect it to be, it shouldn't have any problems with being out of date I think. We could conceivably use the "faux" package version the internal package has to signal when the script needs to re-install it. Something like that? --js
[PATCH v2 3/5] migration: migrate 'blk' command option is deprecated.
Set the 'block' migration capability to 'true' instead.
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 7 +++
qapi/migration.json | 10 +++---
migration/migration.c | 5 +
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index cc0001041f..f727db958e 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -440,3 +440,10 @@ The new way to modify migration is using migration
parameters.
``inc`` functionality can be achieved by setting the
``block-incremental`` migration parameter to ``true``.
+``blk`` migrate command option (since 8.1)
+''
+
+The new way to modify migration is using migration parameters.
+``blk`` functionality can be achieved by setting the
+``block`` migration capability to ``true``.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index 8b30f748ef..291af9407e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1477,7 +1477,9 @@
#
# @uri: the Uniform Resource Identifier of the destination VM
#
-# @blk: do block migration (full disk copy)
+# @blk: do block migration (full disk copy). This option is
+# deprecated. Set the 'block' migration capability to 'true'
+# instead.
#
# @inc: incremental disk copy migration. This option is deprecated.
# Set the 'block-incremetantal' migration parameter to 'true'
@@ -1491,7 +1493,8 @@
# Features:
#
# @deprecated: option @inc should be enabled by setting the
-# 'block-incremental' migration parameter to 'true'.
+# 'block-incremental' migration parameter to 'true', option @blk
+# should be enabled by setting the 'block' capability to 'true'.
#
# Returns: nothing on success
#
@@ -1514,7 +1517,8 @@
# <- { "return": {} }
##
{ 'command': 'migrate',
- 'data': {'uri': 'str', '*blk': 'bool',
+ 'data': {'uri': 'str',
+ '*blk': { 'type': 'bool', 'features': ['deprecated'] },
'*inc': { 'type': 'bool', 'features': ['deprecated'] },
'*detach': 'bool', '*resume': 'bool' } }
diff --git a/migration/migration.c b/migration/migration.c
index abc40e6ef6..4c7e8ff5ee 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1563,6 +1563,11 @@ static bool migrate_prepare(MigrationState *s, bool blk,
bool blk_inc,
" instead.");
}
+if (blk) {
+warn_report("-blk migrate option is deprecated, set the "
+"'block' capability to 'true' instead.");
+}
+
if (resume) {
if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
error_setg(errp, "Cannot resume if there is no "
--
2.40.1
[PATCH v2 5/5] migration: Deprecate old compression method
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 8 +++
qapi/migration.json | 102 --
migration/options.c | 13 +
3 files changed, 86 insertions(+), 37 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 2d7c48185e..792de61c8b 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -457,3 +457,11 @@ Please see "QMP invocation for live storage migration with
``driver-mirror`` + NBD" in docs/interop/live-block-operations.rst for
a detailed explanation.
+old compression method (since 8.1)
+''
+
+Compression method fails too much. Too many races. We are going to
+remove it if nobody fixes it. For starters, migration-test
+compression tests are disabled becase they fail randomly. If you need
+compression, use multifd compression methods.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index 08dee855cb..11f759b90b 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -244,7 +244,9 @@
#
# @compression: migration compression statistics, only returned if
# compression feature is on and status is 'active' or 'completed'
-# (Since 3.1)
+# This feature is unreliable and not tested. It is recommended to
+# use multifd migration instead, which offers an alternative
+# reliable and tested compression implementation. (Since 3.1)
#
# @socket-address: Only used for tcp, to know what the real port is
# (Since 4.0)
@@ -272,8 +274,11 @@
#
# Features:
#
-# @deprecated: @disk migration is deprecated. Use driver-mirror
-# with NBD instead.
+# @deprecated: @disk migration is deprecated. Use driver-mirror with
+# NBD instead. @compression is unreliable and untested. It is
+# recommended to use multifd migration, which offers an
+# alternative compression implementation that is reliable and
+# tested.
#
# Since: 0.14
##
@@ -291,7 +296,7 @@
'*blocked-reasons': ['str'],
'*postcopy-blocktime': 'uint32',
'*postcopy-vcpu-blocktime': ['uint32'],
- '*compression': 'CompressionStats',
+ '*compression': { 'type': 'CompressionStats', 'features':
['deprecated'] },
'*socket-address': ['SocketAddress'],
'*dirty-limit-throttle-time-per-round': 'uint64',
'*dirty-limit-ring-full-time': 'uint64'} }
@@ -446,7 +451,8 @@
# compress and xbzrle are both on, compress only takes effect in
# the ram bulk stage, after that, it will be disabled and only
# xbzrle takes effect, this can help to minimize migration
-# traffic. The feature is disabled by default. (since 2.4 )
+# traffic. The feature is disabled by default. Obsolete. Use
+# multifd compression methods if needed. (since 2.4 )
#
# @events: generate events for each migration state change (since 2.4
# )
@@ -525,8 +531,9 @@
#
# Features:
#
-# @deprecated: @block migration is deprecated. Use driver-mirror
-# with NBD instead.
+# @deprecated: @block migration is deprecated. Use driver-mirror with
+# NBD instead. @compress is obsolete, use multifd compression
+# methods instead.
#
# @unstable: Members @x-colo and @x-ignore-shared are experimental.
#
@@ -534,7 +541,8 @@
##
{ 'enum': 'MigrationCapability',
'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
- 'compress', 'events', 'postcopy-ram',
+ { 'name': 'compress', 'features': [ 'deprecated' ] },
+ 'events', 'postcopy-ram',
{ 'name': 'x-colo', 'features': [ 'unstable' ] },
'release-ram',
{ 'name': 'block', 'features': [ 'deprecated' ] },
@@ -694,22 +702,24 @@
# migration, the compression level is an integer between 0 and 9,
# where 0 means no compression, 1 means the best compression
# speed, and 9 means best compression ratio which will consume
-# more CPU.
+# more CPU. Obsolete, see multifd compression if needed.
#
# @compress-threads: Set compression thread count to be used in live
# migration, the compression thread count is an integer between 1
-# and 255.
+# and 255. Obsolete, see multifd compression if needed.
#
# @compress-wait-thread: Controls behavior when all compression
# threads are currently busy. If true (default), wait for a free
# compression thread to become available; otherwise, send the page
-# uncompressed. (Since 3.1)
+# uncompressed. Obsolete, see multifd compression if
+# needed. (Since 3.1)
#
# @decompress-threads: Set decompression thread count to be used in
# live migration, the decompression thread count is an integer
# between 1 and 255. Usually, decompression is at least 4 times as
# fast as compression, so set the decompress-threads to the number
-# about 1/4 of compress-threads is adequate.
+# about 1/4 of compress-threads is adequate. Obsolete, see multifd
+# co
[PATCH v2 2/5] migration: migrate 'inc' command option is deprecated.
Set the 'block_incremental' migration parameter to 'true' instead.
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 7 +++
qapi/migration.json | 12 ++--
migration/migration.c | 6 ++
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index e1aa0eafc8..cc0001041f 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -433,3 +433,10 @@ Migration
``skipped`` field in Migration stats has been deprecated. It hasn't
been used for more than 10 years.
+``inc`` migrate command option (since 8.1)
+''
+
+The new way to modify migration is using migration parameters.
+``inc`` functionality can be achieved by setting the
+``block-incremental`` migration parameter to ``true``.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index ad8cc57071..8b30f748ef 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1479,13 +1479,20 @@
#
# @blk: do block migration (full disk copy)
#
-# @inc: incremental disk copy migration
+# @inc: incremental disk copy migration. This option is deprecated.
+# Set the 'block-incremetantal' migration parameter to 'true'
+# instead.
#
# @detach: this argument exists only for compatibility reasons and is
# ignored by QEMU
#
# @resume: resume one paused migration, default "off". (since 3.0)
#
+# Features:
+#
+# @deprecated: option @inc should be enabled by setting the
+# 'block-incremental' migration parameter to 'true'.
+#
# Returns: nothing on success
#
# Since: 0.14
@@ -1507,7 +1514,8 @@
# <- { "return": {} }
##
{ 'command': 'migrate',
- 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
+ 'data': {'uri': 'str', '*blk': 'bool',
+ '*inc': { 'type': 'bool', 'features': ['deprecated'] },
'*detach': 'bool', '*resume': 'bool' } }
##
diff --git a/migration/migration.c b/migration/migration.c
index 7a4ba2e846..abc40e6ef6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1557,6 +1557,12 @@ static bool migrate_prepare(MigrationState *s, bool blk,
bool blk_inc,
{
Error *local_err = NULL;
+if (blk_inc) {
+warn_report("-inc migrate option is deprecated, set the "
+"'block-incremental' migration parameter to 'true'"
+" instead.");
+}
+
if (resume) {
if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
error_setg(errp, "Cannot resume if there is no "
--
2.40.1
[PATCH v2 4/5] migration: Deprecate block migration
It is obsolete. It is better to use driver-mirror with NBD instead.
CC: Kevin Wolf
CC: Eric Blake
CC: Stefan Hajnoczi
CC: Hanna Czenczek
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 10 ++
qapi/migration.json | 30 +-
migration/block.c | 3 +++
migration/options.c | 9 -
4 files changed, 46 insertions(+), 6 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index f727db958e..2d7c48185e 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -447,3 +447,13 @@ The new way to modify migration is using migration
parameters.
``blk`` functionality can be achieved by setting the
``block`` migration capability to ``true``.
+block migration (since 8.1)
+'''
+
+Block migration is too inflexible. It needs to migrate all block
+devices or none.
+
+Please see "QMP invocation for live storage migration with
+``driver-mirror`` + NBD" in docs/interop/live-block-operations.rst for
+a detailed explanation.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index 291af9407e..08dee855cb 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -270,11 +270,16 @@
# average memory load of virtual CPU indirectly. Note that zero
# means guest doesn't dirty memory (since 8.1)
#
+# Features:
+#
+# @deprecated: @disk migration is deprecated. Use driver-mirror
+# with NBD instead.
+#
# Since: 0.14
##
{ 'struct': 'MigrationInfo',
'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
- '*disk': 'MigrationStats',
+ '*disk': { 'type': 'MigrationStats', 'features': ['deprecated'] },
'*vfio': 'VfioStats',
'*xbzrle-cache': 'XBZRLECacheStats',
'*total-time': 'int',
@@ -520,6 +525,9 @@
#
# Features:
#
+# @deprecated: @block migration is deprecated. Use driver-mirror
+# with NBD instead.
+#
# @unstable: Members @x-colo and @x-ignore-shared are experimental.
#
# Since: 1.2
@@ -529,7 +537,8 @@
'compress', 'events', 'postcopy-ram',
{ 'name': 'x-colo', 'features': [ 'unstable' ] },
'release-ram',
- 'block', 'return-path', 'pause-before-switchover', 'multifd',
+ { 'name': 'block', 'features': [ 'deprecated' ] },
+ 'return-path', 'pause-before-switchover', 'multifd',
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
{ 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
'validate-uuid', 'background-snapshot',
@@ -819,6 +828,9 @@
#
# Features:
#
+# @deprecated: Member @block-incremental is obsolete. Use
+# driver-mirror with NBD instead.
+#
# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
# are experimental.
#
@@ -834,7 +846,7 @@
'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
'downtime-limit',
{ 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
- 'block-incremental',
+ { 'name': 'block-incremental', 'features': [ 'deprecated' ] },
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
@@ -985,6 +997,9 @@
#
# Features:
#
+# @deprecated: Member @block-incremental is obsolete. Use
+# driver-mirror with NBD instead.
+#
# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
# are experimental.
#
@@ -1013,7 +1028,8 @@
'*downtime-limit': 'uint64',
'*x-checkpoint-delay': { 'type': 'uint32',
'features': [ 'unstable' ] },
-'*block-incremental': 'bool',
+'*block-incremental': { 'type': 'bool',
+'features': [ 'deprecated' ] },
'*multifd-channels': 'uint8',
'*xbzrle-cache-size': 'size',
'*max-postcopy-bandwidth': 'size',
@@ -1188,6 +1204,9 @@
#
# Features:
#
+# @deprecated: Member @block-incremental is obsolete. Use
+# driver-mirror with NBD instead.
+#
# @unstable: Members @x-checkpoint-delay and
# @x-vcpu-dirty-limit-period are experimental.
#
@@ -1213,7 +1232,8 @@
'*downtime-limit': 'uint64',
'*x-checkpoint-delay': { 'type': 'uint32',
'features': [ 'unstable' ] },
-'*block-incremental': 'bool',
+'*block-incremental': { 'type': 'bool',
+'features': [ 'deprecated' ] },
'*multifd-channels': 'uint8',
'*xbzrle-cache-size': 'size',
'*max-postcopy-bandwidth': 'size',
diff --git a/migration/block.c b/migration/block.c
index b29e80bdc4..a095024108 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -722,6 +722,9 @@ static int block_save_setup(QEMUFile *f, void *opaque)
trace_migration_block_save("setup"
[PATCH v2 1/5] migration: Use proper indentation for migration.json
We broke it with dirtyrate limit patches. Signed-off-by: Juan Quintela --- qapi/migration.json | 67 ++--- 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index 6ff39157ba..ad8cc57071 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -258,17 +258,17 @@ # blocked. Present and non-empty when migration is blocked. # (since 6.0) # -# @dirty-limit-throttle-time-per-round: Maximum throttle time (in microseconds) of virtual -# CPUs each dirty ring full round, which shows how -# MigrationCapability dirty-limit affects the guest -# during live migration. (since 8.1) +# @dirty-limit-throttle-time-per-round: Maximum throttle time (in +# microseconds) of virtual CPUs each dirty ring full round, which +# shows how MigrationCapability dirty-limit affects the guest +# during live migration. (since 8.1) # -# @dirty-limit-ring-full-time: Estimated average dirty ring full time (in microseconds) -# each dirty ring full round, note that the value equals -# dirty ring memory size divided by average dirty page rate -# of virtual CPU, which can be used to observe the average -# memory load of virtual CPU indirectly. Note that zero -# means guest doesn't dirty memory (since 8.1) +# @dirty-limit-ring-full-time: Estimated average dirty ring full time +# (in microseconds) each dirty ring full round, note that the +# value equals dirty ring memory size divided by average dirty +# page rate of virtual CPU, which can be used to observe the +# average memory load of virtual CPU indirectly. Note that zero +# means guest doesn't dirty memory (since 8.1) # # Since: 0.14 ## @@ -510,14 +510,13 @@ # (since 7.1) # # @dirty-limit: If enabled, migration will use the dirty-limit algo to -# throttle down guest instead of auto-converge algo. -# Throttle algo only works when vCPU's dirtyrate greater -# than 'vcpu-dirty-limit', read processes in guest os -# aren't penalized any more, so this algo can improve -# performance of vCPU during live migration. This is an -# optional performance feature and should not affect the -# correctness of the existing auto-converge algo. -# (since 8.1) +# throttle down guest instead of auto-converge algo. Throttle +# algo only works when vCPU's dirtyrate greater than +# 'vcpu-dirty-limit', read processes in guest os aren't penalized +# any more, so this algo can improve performance of vCPU during +# live migration. This is an optional performance feature and +# should not affect the correctness of the existing auto-converge +# algo. (since 8.1) # # Features: # @@ -811,17 +810,17 @@ # Nodes are mapped to their block device name if there is one, and # to their node name otherwise. (Since 5.2) # -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty limit during -# live migration. Should be in the range 1 to 1000ms, -# defaults to 1000ms. (Since 8.1) +# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty +# limit during live migration. Should be in the range 1 to 1000ms, +# defaults to 1000ms. (Since 8.1) # # @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration. -#Defaults to 1. (Since 8.1) +# Defaults to 1. (Since 8.1) # # Features: # # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period -#are experimental. +# are experimental. # # Since: 2.4 ## @@ -977,17 +976,17 @@ # Nodes are mapped to their block device name if there is one, and # to their node name otherwise. (Since 5.2) # -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty limit during -# live migration. Should be in the range 1 to 1000ms, -# defaults to 1000ms. (Since 8.1) +# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty +# limit during live migration. Should be in the range 1 to 1000ms, +# defaults to 1000ms. (Since 8.1) # # @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration. -#Defaults to 1. (Since 8.1) +# Defaults to 1. (Since 8.1) # # Features: # # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period -#are experimental. +# are experimental. # # TODO: either fuse back into MigrationParameters, or make # MigrationParameters members mandatory @@ -1180,17 +1179,17 @@ # Nodes are mapped to their block d
[PATCH v2 0/5] Migration deprecated parts
On this v2: - dropped -incoming deprecation Paolo came with a better solution using keyvalues. - skipped field is already ready for next pull request, so dropped. - dropped the RFC bits, nermal PATCH. - Assessed all the review comments. - Added indentation of migration.json. - Used the documentation pointer to substitute block migration. Please review. [v1] Hi this series describe the migration parts that have to be deprecated. - It is an rfc because I doubt that I did the deprecation process right. Hello Markus O:-) - skipped field: It is older than me, I have never know what it stands for. As far as I know it has always been zero. - inc/blk migrate command options. They are only used by block migration (that I deprecate on the following patch). And they are really bad. grep must_remove_block_options. - block migration. block jobs, whatever they are called this week are way more flexible. Current code works, but we broke it here and there, and really nobody has stand up to maintain it. It is quite contained and can be left there. Is anyone really using it? - old compression method. It don't work. See last try from Lukas to make a test that works reliabely. I failed with the same task years ago. It is really slow, and if compression is good for you, multifd + zlib is going to perform/compress way more. I don't know what to do with this code, really. * Remove it for this release? It don't work, and haven't work reliabely in quite a few time. * Deprecate it and remove in another couple of releases, i.e. normal deprecation. * Ideas? - -incoming if you need to set parameters (multifd cames to mind, and preempt has the same problem), you really needs to use defer. So what should we do here? This part is not urget, because management apps have a working option that are already using "defer", and the code simplifacation if we remove it is not so big. So we can leave it until 9.0 or whatever we think fit. What do you think? Later, Juan. Juan Quintela (5): migration: Use proper indentation for migration.json migration: migrate 'inc' command option is deprecated. migration: migrate 'blk' command option is deprecated. migration: Deprecate block migration migration: Deprecate old compression method docs/about/deprecated.rst | 32 ++ qapi/migration.json | 203 -- migration/block.c | 3 + migration/migration.c | 11 +++ migration/options.c | 22 - 5 files changed, 198 insertions(+), 73 deletions(-) base-commit: 5f9dd6a8ce3961db4ce47411ed2097ad88bdf5fc prerequisite-patch-id: 99c8bffa9428838925e330eb2881bab476122579 prerequisite-patch-id: 77ba427fd916aeb395e95aa0e7190f84e98e96ab prerequisite-patch-id: 9983d46fa438d7075a37be883529e37ae41e4228 prerequisite-patch-id: 207f7529924b12dcb57f6557d6db6f79ceb2d682 prerequisite-patch-id: 5ad1799a13845dbf893a28a202b51a6b50d95d90 prerequisite-patch-id: c51959aacd6d65ee84fcd4f1b2aed3dd6f6af879 prerequisite-patch-id: da9dbb6799b2da002c0896574334920097e4c50a prerequisite-patch-id: c1110ffafbaf5465fb277a20db809372291f7846 prerequisite-patch-id: 8307c92bedd07446214b35b40206eb6793a7384d prerequisite-patch-id: 0a6106cd4a508d5e700a7ff6c25edfdd03c8ca3d prerequisite-patch-id: 83205051de22382e75bf4acdf69e59315801fa0d prerequisite-patch-id: 8c9b3cba89d555c071a410041e6da41806106a7e prerequisite-patch-id: 0ff62a33b9a242226ccc1f5424a516de803c9fe5 prerequisite-patch-id: 25b8ae1ebe09ace14457c454cfcb23077c37346c prerequisite-patch-id: 466ea91d5be41fe345dacd4d17bbbe5ce13118c2 prerequisite-patch-id: d1045858f9729ac62eccf2e83ebf95cfebae2cb5 prerequisite-patch-id: 0276ec02073bda5426de39e2f2e81eef080b4f54 prerequisite-patch-id: 7afb4450a163cc1a63ea23831c50214966969131 prerequisite-patch-id: 06c053ce4f41db9675bd1778ae8f6a483641fcef prerequisite-patch-id: 13ea05d54d741ed08b3bfefa1fc8bedb9c81c782 prerequisite-patch-id: 99c4e2b7101bc8c4b9515129a1bbe6f068053dbf prerequisite-patch-id: 1e393a196dc7a1ee75f3cc3cebbb591c5422102f prerequisite-patch-id: 2cf497b41f5024ede0a224b1f5b172226067a534 prerequisite-patch-id: 2a70276ed61d33fc4f3b52560753c05d1cd413be prerequisite-patch-id: 17ec40f4388b62ba8bf3ac1546c6913f5d1f6079 prerequisite-patch-id: dba969ce9d6cf69c1319661a7d81b1c1c719804d prerequisite-patch-id: 8d800cda87167314f07320bdb3df936c323e4a40 prerequisite-patch-id: 25d4aaf54ea66f30e426fa38bdd4e0f47303c513 prerequisite-patch-id: 082c9d8584c1daff1e827e44ee3047178e7004a7 prerequisite-patch-id: 0ef73900899425ae2f00751347afdce3739aa954 prerequisite-patch-id: e7db4730b791b71aaf417ee0f65fb6304566aaf8 prerequisite-patch-id: 62d7f28f8196039507ffe362f97723395d7bb704 prerequisite-patch-id: ea8de47bcb54e33bcc67e59e9ed752a4d1fad703 prerequisite-patch-id: 497893ef92e1ea56bd8605e6990a05cb4c7f9293 prerequisite-patch-id: 3dc869c80ee568449bbfa2a9bc427524d0e8970b prerequisite-patch-id: 52c14b6fb14ed4ccd685385a9fbc6297b762c0ef prerequisite-patch-id: 23de8371e9e3277
Re: [RFC 4/6] migration: Deprecate -incoming
Peter Xu wrote: > On Thu, Jun 22, 2023 at 11:22:56AM +0200, Thomas Huth wrote: >> Then simply forbid "migrate_set_parameter multifd-channels ..." if the uri >> has been specified on the command line? > > Yeah, actually already in a pull (even though the pr may need a new one..): > > https://lore.kernel.org/r/20230622021320.66124-23-quint...@redhat.com That is a different problem, and different solution. It you try to set multifd_channels after migration has started, it just fails telling that you can't change it so late. Later, Juan.
Re: [RFC 4/6] migration: Deprecate -incoming
Peter Xu wrote:
> On Mon, Jun 12, 2023 at 10:51:08PM +0200, Juan Quintela wrote:
>> Peter Xu wrote:
>> > On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
>> >> Only "defer" is recommended. After setting all migation parameters,
>> >> start incoming migration with "migrate-incoming uri" command.
>> >>
>> >> Signed-off-by: Juan Quintela
>> >> ---
>> >> docs/about/deprecated.rst | 7 +++
>> >> softmmu/vl.c | 2 ++
>> >> 2 files changed, 9 insertions(+)
>> >>
>> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> >> index 47e98dc95e..518672722d 100644
>> >> --- a/docs/about/deprecated.rst
>> >> +++ b/docs/about/deprecated.rst
>> >> @@ -447,3 +447,10 @@ The new way to modify migration is using migration
>> >> parameters.
>> >> ``blk`` functionality can be acchieved using
>> >> ``migrate_set_parameter block-incremental true``.
>> >>
>> >> +``-incoming uri`` (since 8.1)
>> >> +'
>> >> +
>> >> +Everything except ``-incoming defer`` are deprecated. This allows to
>> >> +setup parameters before launching the proper migration with
>> >> +``migrate-incoming uri``.
>> >> +
>> >> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> >> index b0b96f67fa..7fe865ab59 100644
>> >> --- a/softmmu/vl.c
>> >> +++ b/softmmu/vl.c
>> >> @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp)
>> >> if (incoming) {
>> >> Error *local_err = NULL;
>> >> if (strcmp(incoming, "defer") != 0) {
>> >> +warn_report("-incoming %s is deprecated, use -incoming defer
>> >> and "
>> >> +" set the uri with migrate-incoming.", incoming);
>> >
>> > I still use uri for all my scripts, alongside with "-global migration.xxx"
>> > and it works.
>>
>> You know what you are doing (TM).
>> And remember that we don't support -gobal migration.x-foo.
>> Yes, I know, we should drop the "x-" prefixes.
>
> I hope they'll always be there. :) They're pretty handy for tests, when we
> want to boot a VM without the need to script the sequences of qmp cmds.
>
> Yes, we probably should just always drop the x-. We can always declare
> debugging purpose for all -global migration.* fields.
>
>>
>> > Shall we just leave it there? Or is deprecating it helps us in any form?
>>
>> See the patches two weeks ago when people complained that lisen(.., num)
>> was too low. And there are other parameters that work the same way
>> (that I convenientely had forgotten). So the easiest way to get things
>> right is to use "defer" always. Using -incoming "uri" should only be
>> for people that "know what they are doing", so we had to ways to do it:
>> - review all migration options and see which ones work without defer
>> and document it
>> - deprecate everything that is not defer.
>>
>> Anything else is not going to be very user unfriendly.
>> What do you think.
>
> IIRC Wei Wang had a series just for that, so after that patchset applied we
> should have fixed all issues cleanly?
No, what he does is using always a very big value for listen. But that
is it. Anyways, I don't know how to change the backlog listen value
without restarting the listen call.
> Is there one more thing that's not
> working right there?
Compression has other problems. But independentely of that, they have
the problem that we need to set the parameters before we call incoming.
>> PD. This series are RFC for multiple reasons O:-)
>
> Happy to know the rest (besides which I know will break my script :).
Thanks, Juan.
Re: [RFC 6/6] migration: Deprecated old compression method
Daniel P. Berrangé wrote:
> On Mon, Jun 12, 2023 at 09:33:44PM +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela
>> ---
>> docs/about/deprecated.rst | 8
>> qapi/migration.json | 92 ---
>> migration/options.c | 13 ++
>> 3 files changed, 79 insertions(+), 34 deletions(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index 173c5ba5cb..fe7f2bbde8 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -460,3 +460,11 @@ block migration (since 8.1)
>> Block migration is too inflexible. It needs to migrate all block
>> devices or none. Use driver_mirror+NBD instead.
>>
>> +old compression method (since 8.1)
>> +''
>> +
>> +Compression method fails too much. Too many races. We are going to
>> +remove it if nobody fixes it. For starters, migration-test
>> +compression tests are disabled becase they hand randomly. If you need
>> +compression, use multifd compression methods.
>> +
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index a8497de48d..40a8b5d124 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -244,6 +244,7 @@
>> #
>> # @compression: migration compression statistics, only returned if
>> # compression feature is on and status is 'active' or 'completed'
>> +# It is obsolete and deprecated. Use multifd compression methods.
>> # (Since 3.1)
>
> This doesn't give users an indication /why/ we're saying this. Instead
> I'd suggest
>
> This feature is unreliable and not tested. It is recommended to
> use multifd migration instead, which offers an alternative reliable
> and tested compression implementation.
Much better. Done, thanks.
>> # @deprecated: @disk migration is deprecated. Use driver_mirror+NBD
>> -# instead.
>> +# instead. @compression is obsolete use multifd compression
>> +# methods instead.
>
> For @deprecated, are we supposed to list multiple things at once, or
> use a separate @deprecated tag for each one ?
# @unstable: Members @x-colo and @x-ignore-shared are experimental.
This is the only example that I found that is similar.
Only one example. Markus?
>
> Again I'd suggest rewording
>
> @compression is unreliable and untested. It is recommended to
> use multifd migration, which offers an alternative compression
> implementation that is reliable and tested.
Done.
>> @@ -443,6 +443,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
>> Error **errp)
>> "Use driver_mirror+NBD instead.");
>> }
>>
>> +if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {
>
> Surely MIGRATION_CAPABILITY_COMPRESS not BLOCK ?
Good catch. Copy & paste to its best.
Thanks very much.
Re: [RFC 6/6] migration: Deprecated old compression method
Thomas Huth wrote:
> On 12/06/2023 21.33, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela
>> ---
>> docs/about/deprecated.rst | 8
>> qapi/migration.json | 92 ---
>> migration/options.c | 13 ++
>> 3 files changed, 79 insertions(+), 34 deletions(-)
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index 173c5ba5cb..fe7f2bbde8 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -460,3 +460,11 @@ block migration (since 8.1)
>> Block migration is too inflexible. It needs to migrate all block
>> devices or none. Use driver_mirror+NBD instead.
>> +old compression method (since 8.1)
>> +''
>> +
>> +Compression method fails too much. Too many races. We are going to
>> +remove it if nobody fixes it. For starters, migration-test
>> +compression tests are disabled becase they hand randomly. If you need
>
> "because they fail randomly" ?
yeap.
>> # @deprecated: @disk migration is deprecated. Use driver_mirror+NBD
>> -# instead.
>> +# instead. @compression is obsolete use multifd compression
>
> Use a dot or comma after "obsolete".
fixed.
>> @@ -503,6 +506,7 @@
>> # Features:
>> #
>> # @deprecated: @block migration is deprecated. Use driver_mirror+NBD
>> +# instead. @compress is obsolete use multifd compression methods
>
> dito
fixed.
>> -# @compress-threads: compression thread count
>> +# @compress-threads: compression thread count. Obsolote and
>
> Obsolete
Fixed.
>> @@ -1182,7 +1209,6 @@
>>'features': [ 'unstable' ] },
>> '*block-incremental': { 'type': 'bool',
>> 'features': [ 'deprecated' ] },
>> -'*block-incremental': 'bool',
>
> That hunk should go into a previous patch, I think.
Have found it already (it didn't compile).
Thanks, Juan.
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 05:33:29PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
> > I can try to move the todo even higher. Trying to list the initial goals
> > here:
> >
> > - One extra phase of handshake between src/dst (maybe the time to boost
> > QEMU_VM_FILE_VERSION) before anything else happens.
> >
> > - Dest shouldn't need to apply any cap/param, it should get all from src.
> > Dest still need to be setup with an URI and that should be all it needs.
> >
> > - Src shouldn't need to worry on the binary version of dst anymore as long
> > as dest qemu supports handshake, because src can fetch it from dest.
>
> I'm not sure that works in general. Even if we have a handshake and
> bi-directional comms for live migration, we still haave the save/restore
> to file codepath to deal with. The dst QEMU doesn't exist at the time
> the save process is done, so we can't add logic to VMSate handling that
> assumes knowledge of the dst version at time of serialization.
My current thought was still based on a new cap or anything the user would
need to specify first on both sides (but hopefully the last cap to set on
dest).
E.g. if with a new handshake cap we shouldn't set it on a exec: or file:
protocol migration, and it should just fail on qmp_migrate() telling that
the URI is not supported if the cap is set. Return path is definitely
required here.
>
> > - Handshake can always fail gracefully if anything wrong happened, it
> > normally should mean dest qemu is not compatible with src's setup (either
> > machine, device, or migration configs) for whatever reason. Src should
> > be able to get a solid error from dest if so.
> >
> > - Handshake protocol should always be self-bootstrap-able, it means when we
> > change the handshake protocol it should always works with old binaries.
> >
> > - When src is newer it should be able to know what's missing on dest and
> > skip the new bits.
> >
> > - When dst is newer it should all rely on src (which is older) and it
> > should always understand src's language.
>
> I'm not convinced it can reliably self-bootstrap in a backwards
> compatible manner, precisely because the current migration stream
> has no handshake and only requires a unidirectional channel.
Yes, please see above. I meant when we grow the handshake protocol we
should make sure we don't need anything new to be setup either on src/dst
of qemu. It won't apply to before-handshake binaries.
> I don't think its possible for QEMU to validate that it has a fully
> bi-directional channel, without adding timeouts to its detection which I
> think we should strive to avoid.
>
> I don't think we actually need self-bootstrapping anyway.
>
> I think the mgmt app can just indicate the new v2 bi-directional
> protocol when issuing the 'migrate' and 'migrate-incoming'
> commands. This becomes trivial when Het's refactoring of the
> migrate address QAPI is accepted:
>
> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04851.html
>
> eg:
>
> { "execute": "migrate",
> "arguments": {
> "channels": [ { "channeltype": "main",
> "addr": { "transport": "socket", "type": "inet",
>"host": "10.12.34.9",
> "port": "1050" } } ] } }
>
> note the 'channeltype' parameter here. If we declare the 'main'
> refers to the existing migration protocol, then we merely need
> to define a new 'channeltype' to use as an indicator for the
> v2 migration handshake protocol.
Using a new channeltype would also work at least on src qemu, but I'm not
sure on how dest qemu would know that it needs a handshake in that case,
because it knows nothing until the connection is established.
Maybe we still need QEMU_VM_FILE_VERSION to be boosted at least in this
case, so dest can read this at the very beginning, old binaries will fail
immediately, new binaries will start to talk with v2 language.
>
> > - All !main channels need to be established later than the handshake - if
> > we're going to do this anyway we probably should do it altogether to make
> > channels named, so each channel used in migration needs to have a common
> > header. Prepare to deprecate the old tricks of channel orderings.
>
> Once the primary channel involves a bi-directional handshake,
> we'll trivially ensure ordering - similar to how the existing
> code worked fnie in TLS mode which had a bi-directional TLS
> handshake.
I'm not sure I fully get it here.
IIUC tls handshake was mostly transparent to QEMU in this case while we're
relying on gnutls_handshake(). Here IIUC we need to design the roundtrip
messages to sync up two qemus well.
The round trip messages can contain a lot of things that can be useful to
us, besides knowing what features dest supports, what caps src use, we can
e.g. also provide a device tree dump from dest and try to match it on sr
Re: [RFC 5/6] migration: Deprecate block migration
Stefan Hajnoczi wrote: > On Mon, Jun 12, 2023 at 09:33:43PM +0200, Juan Quintela wrote: >> It is obsolete. It is better to use driver_mirror+NBD instead. >> >> CC: Kevin Wolf >> CC: Eric Blake >> CC: Stefan Hajnoczi >> CC: Hanna Czenczek >> >> Signed-off-by: Juan Quintela >> >> --- >> >> Can any of you give one example of how to use driver_mirror+NBD for >> deprecated.rst? > > Please see "QMP invocation for live storage migration with > ``drive-mirror`` + NBD" in docs/interop/live-block-operations.rst for a > detailed explanation. You put here drive-mirror, and everything else blockdev-mirror. It appears that blockdev-mirror is the new name from driver-mirror, but as the documentation says driver-mirror + NBD, I am changing to driver-mirror everywhere? Thanks, Juan.
Re: [RFC 4/6] migration: Deprecate -incoming
Juan Quintela wrote: > Only "defer" is recommended. After setting all migation parameters, > start incoming migration with "migrate-incoming uri" command. > > Signed-off-by: Juan Quintela Nack myself. Dropped on next submissiong. keyfile properties suggested by paolo is a much better suggestion. Thanks to everybody involved.
Re: [RFC 3/6] migration: migrate 'blk' command option is deprecated.
Daniel P. Berrangé wrote: > On Mon, Jun 12, 2023 at 09:33:41PM +0200, Juan Quintela wrote: >> Use 'migrate_set_capability block true' instead. >> >> Signed-off-by: Juan Quintela >> --- >> docs/about/deprecated.rst | 7 +++ >> qapi/migration.json | 11 +++ >> migration/migration.c | 5 + >> 3 files changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >> index c75a3a8f5a..47e98dc95e 100644 >> --- a/docs/about/deprecated.rst >> +++ b/docs/about/deprecated.rst >> @@ -440,3 +440,10 @@ The new way to modify migration is using migration >> parameters. >> ``inc`` functionality can be acchieved using >> ``migrate_set_parameter block-incremental true``. >> >> +``blk`` migrate command option (since 8.1) >> +'' >> + >> +The new way to modify migration is using migration parameters. >> +``blk`` functionality can be acchieved using >> +``migrate_set_parameter block-incremental true``. > > Same comments on rewording as the previous patch, so won't repeate them > all. Did the same than the previous one. Thanks.
Re: [RFC 2/6] migration: migrate 'inc' command option is deprecated.
Daniel P. Berrangé wrote: > On Mon, Jun 12, 2023 at 09:33:40PM +0200, Juan Quintela wrote: >> Use 'migrate_set_parameter block_incremental true' instead. >> >> Signed-off-by: Juan Quintela >> --- >> docs/about/deprecated.rst | 7 +++ >> qapi/migration.json | 11 +-- >> migration/migration.c | 5 + >> 3 files changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >> index e1aa0eafc8..c75a3a8f5a 100644 >> --- a/docs/about/deprecated.rst >> +++ b/docs/about/deprecated.rst >> @@ -433,3 +433,10 @@ Migration >> ``skipped`` field in Migration stats has been deprecated. It hasn't >> been used for more than 10 years. >> >> +``inc`` migrate command option (since 8.1) >> +'' >> + >> +The new way to modify migration is using migration parameters. >> +``inc`` functionality can be acchieved using >> +``migrate_set_parameter block-incremental true``. > > This is a HMP command, but the change affects QMP too. I'd suggest > > ``inc`` functionality can be achieved by setting the > ``block-incremental`` migration parameter to ``true``. Applied all suggestions. Thanks.
Re: [RFC 1/6] migration: skipped field is really obsolete.
Daniel P. Berrangé wrote: > On Mon, Jun 12, 2023 at 09:33:39PM +0200, Juan Quintela wrote: >> Has return zero for more than 10 years. Just mark it deprecated. > > Specifically we introduced the field in 1.5.0 > > commit f1c72795af573b24a7da5eb52375c9aba8a37972 > Author: Peter Lieven > Date: Tue Mar 26 10:58:37 2013 +0100 > > migration: do not sent zero pages in bulk stage > > during bulk stage of ram migration if a page is a > zero page do not send it at all. > the memory at the destination reads as zero anyway. > > even if there is an madvise with QEMU_MADV_DONTNEED > at the target upon receipt of a zero page I have observed > that the target starts swapping if the memory is overcommitted. > it seems that the pages are dropped asynchronously. > > this patch also updates QMP to return the number of > skipped pages in MigrationStats. > > > > but removed its usage in 1.5.3 > > commit 9ef051e5536b6368a1076046ec6c4ec4ac12b5c6 > Author: Peter Lieven > Date: Mon Jun 10 12:14:19 2013 +0200 > > Revert "migration: do not sent zero pages in bulk stage" > > Not sending zero pages breaks migration if a page is zero > at the source but not at the destination. This can e.g. happen > if different BIOS versions are used at source and destination. > It has also been reported that migration on pseries is completely > broken with this patch. > > This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972. Thanks for the history O:-) >> Signed-off-by: Juan Quintela >> --- >> docs/about/deprecated.rst | 10 ++ >> qapi/migration.json | 12 ++-- >> 2 files changed, 20 insertions(+), 2 deletions(-) > > Reviewed-by: Daniel P. Berrangé > > >> diff --git a/qapi/migration.json b/qapi/migration.json >> index cb7cd3e578..bcae193733 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -23,7 +23,8 @@ >> # >> # @duplicate: number of duplicate (zero) pages (since 1.2) >> # >> -# @skipped: number of skipped zero pages (since 1.5) >> +# @skipped: number of skipped zero pages. Don't use, only provided for >> +# compatibility (since 1.5) > > I'd say > >@skipped: number of skipped zero pages. Always zero, only provided for >compatibility (since 1.5) Changed. >> # >> # @normal: number of normal pages (since 1.2) >> # >> @@ -62,11 +63,18 @@ >> # between 0 and @dirty-sync-count * @multifd-channels. (since >> # 7.1) >> # >> +# Features: >> +# >> +# @deprecated: Member @skipped has not been used for a long time. > > @deprecated: Member @skipped is always zero since 1.5.3 Changed. Thanks.
[PULL 13/30] migration-test: Create arch_opts
This will contain the options needed for both source and target.
Reviewed-by: Peter Xu
Message-ID: <20230608224943.3877-6-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 79157d600b..4d8542f5c7 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -600,6 +600,8 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
{
g_autofree gchar *arch_source = NULL;
g_autofree gchar *arch_target = NULL;
+/* options for source and target */
+g_autofree gchar *arch_opts = NULL;
g_autofree gchar *cmd_source = NULL;
g_autofree gchar *cmd_target = NULL;
const gchar *ignore_stderr;
@@ -625,15 +627,13 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
assert(sizeof(x86_bootsect) == 512);
init_bootfile(bootpath, x86_bootsect, sizeof(x86_bootsect));
memory_size = "150M";
-arch_source = g_strdup_printf("-drive file=%s,format=raw", bootpath);
-arch_target = g_strdup(arch_source);
+arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
start_address = X86_TEST_MEM_START;
end_address = X86_TEST_MEM_END;
} else if (g_str_equal(arch, "s390x")) {
init_bootfile(bootpath, s390x_elf, sizeof(s390x_elf));
memory_size = "128M";
-arch_source = g_strdup_printf("-bios %s", bootpath);
-arch_target = g_strdup(arch_source);
+arch_opts = g_strdup_printf("-bios %s", bootpath);
start_address = S390_TEST_MEM_START;
end_address = S390_TEST_MEM_END;
} else if (strcmp(arch, "ppc64") == 0) {
@@ -641,20 +641,16 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
memory_size = "256M";
start_address = PPC_TEST_MEM_START;
end_address = PPC_TEST_MEM_END;
-arch_source = g_strdup_printf("-nodefaults "
- "-prom-env 'use-nvramrc?=true' -prom-env
"
+arch_source = g_strdup_printf("-prom-env 'use-nvramrc?=true' -prom-env
"
"'nvramrc=hex .\" _\" begin %x %x "
"do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
"until'", end_address, start_address);
-arch_target = g_strdup("-nodefaults");
+arch_opts = g_strdup("-nodefaults");
} else if (strcmp(arch, "aarch64") == 0) {
init_bootfile(bootpath, aarch64_kernel, sizeof(aarch64_kernel));
machine_opts = "-machine virt,gic-version=max";
memory_size = "150M";
-arch_source = g_strdup_printf("-cpu max "
- "-kernel %s",
- bootpath);
-arch_target = g_strdup(arch_source);
+arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
start_address = ARM_TEST_MEM_START;
end_address = ARM_TEST_MEM_END;
@@ -693,12 +689,14 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
"-name source,debug-threads=on "
"-m %s "
"-serial file:%s/src_serial "
- "%s %s %s %s",
+ "%s %s %s %s %s",
args->use_dirty_ring ?
",dirty-ring-size=4096" : "",
machine_opts ? machine_opts : "",
memory_size, tmpfs,
- arch_source, shmem_opts,
+ arch_opts ? arch_opts : "",
+ arch_source ? arch_source : "",
+ shmem_opts,
args->opts_source ? args->opts_source : "",
ignore_stderr);
if (!args->only_target) {
@@ -713,12 +711,14 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
"-m %s "
"-serial file:%s/dest_serial "
"-incoming %s "
- "%s %s %s %s",
+ "%s %s %s %s %s",
args->use_dirty_ring ?
",dirty-ring-size=4096" : "",
machine_opts ? machine_opts : "",
memory_size, tmpfs, uri,
- arch_target, shmem_opts,
+ arch_opts ? arch_opts : "",
+ arch_target ? arch_target : "",
+ shmem_opts,
[PULL 19/30] migration-test: simplify shmem_opts handling
Reviewed-by: Peter Xu
Message-ID: <20230608224943.3877-4-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index fbe9db23cf..e3e7d54216 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -704,9 +704,6 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
"-object memory-backend-file,id=mem0,size=%s"
",mem-path=%s,share=on -numa node,memdev=mem0",
memory_size, shmem_path);
-} else {
-shmem_path = NULL;
-shmem_opts = g_strdup("");
}
if (args->use_dirty_ring) {
@@ -722,7 +719,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
memory_size, tmpfs,
arch_opts ? arch_opts : "",
arch_source ? arch_source : "",
- shmem_opts,
+ shmem_opts ? shmem_opts : "",
args->opts_source ? args->opts_source : "",
ignore_stderr);
if (!args->only_target) {
@@ -742,7 +739,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
memory_size, tmpfs, uri,
arch_opts ? arch_opts : "",
arch_target ? arch_target : "",
- shmem_opts,
+ shmem_opts ? shmem_opts : "",
args->opts_target ? args->opts_target : "",
ignore_stderr);
*to = qtest_init(cmd_target);
--
2.40.1
[PULL 21/30] migration: Refactor repeated call of yank_unregister_instance
From: Tejus GK
In the function qmp_migrate(), yank_unregister_instance() gets called
twice which isn't required. Hence, refactoring it so that it gets called
during the local_error cleanup.
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Juan Quintela
Acked-by: Peter Xu
Signed-off-by: Tejus GK
Message-ID: <20230621130940.178659-3-tejus...@nutanix.com>
Signed-off-by: Juan Quintela
---
migration/migration.c | 4
1 file changed, 4 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index e6bff2e848..7a4ba2e846 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1676,15 +1676,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool
blk,
} else if (strstart(uri, "fd:", &p)) {
fd_start_outgoing_migration(s, p, &local_err);
} else {
-if (!(has_resume && resume)) {
-yank_unregister_instance(MIGRATION_YANK_INSTANCE);
-}
error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
"a valid migration protocol");
migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_FAILED);
block_cleanup_parameters();
-return;
}
if (local_err) {
--
2.40.1
[PULL 23/30] qtest/migration-tests.c: use "-incoming defer" for postcopy tests
From: Wei Wang
The Postcopy preempt capability is expected to be set before incoming
starts, so change the postcopy tests to start with deferred incoming and
call migrate-incoming after the cap has been set.
Why the existing tests (without this patch) didn't fail?
There could be two reasons:
1) "backlog" specifies the number of pending connections. As long as the
server accepts the connections faster than the clients side connecting,
connection will succeed. For the preempt test, it uses only 2 channels,
so very likely to not have pending connections.
2) per my tests (on kernel 6.2), the number of pending connections allowed
is actually "backlog + 1", which is 2 in this case.
That said, the implementation of socket_start_incoming_migration_internal
expects "migrate defer" to be used, and for safety, change the test to
work with the expected usage.
Signed-off-by: Wei Wang
Reviewed-by: Peter Xu
Reviewed-by: Juan Quintela
Message-ID: <20230606101910.20456-3-wei.w.w...@intel.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index e3e7d54216..c694685923 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1161,10 +1161,10 @@ static int migrate_postcopy_prepare(QTestState
**from_ptr,
QTestState **to_ptr,
MigrateCommon *args)
{
-g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+g_autofree char *uri = NULL;
QTestState *from, *to;
-if (test_migrate_start(&from, &to, uri, &args->start)) {
+if (test_migrate_start(&from, &to, "defer", &args->start)) {
return -1;
}
@@ -1183,9 +1183,13 @@ static int migrate_postcopy_prepare(QTestState
**from_ptr,
migrate_ensure_non_converge(from);
+qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
+ " 'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
+uri = migrate_get_socket_address(to, "socket-address");
migrate_qmp(from, uri, "{}");
wait_for_migration_pass(from);
--
2.40.1
[PULL 14/30] migration-test: machine_opts is really arch specific
And it needs to be in both source and target, so put it on arch_opts.
Reviewed-by: Peter Xu
Message-ID: <20230608224943.3877-7-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 4d8542f5c7..fc3337b7bb 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -609,7 +609,6 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
g_autofree char *shmem_opts = NULL;
g_autofree char *shmem_path = NULL;
const char *arch = qtest_get_arch();
-const char *machine_opts = NULL;
const char *memory_size;
if (args->use_shmem) {
@@ -637,7 +636,6 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
start_address = S390_TEST_MEM_START;
end_address = S390_TEST_MEM_END;
} else if (strcmp(arch, "ppc64") == 0) {
-machine_opts = "-machine vsmt=8";
memory_size = "256M";
start_address = PPC_TEST_MEM_START;
end_address = PPC_TEST_MEM_END;
@@ -645,12 +643,12 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
"'nvramrc=hex .\" _\" begin %x %x "
"do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
"until'", end_address, start_address);
-arch_opts = g_strdup("-nodefaults");
+arch_opts = g_strdup("-nodefaults -machine vsmt=8");
} else if (strcmp(arch, "aarch64") == 0) {
init_bootfile(bootpath, aarch64_kernel, sizeof(aarch64_kernel));
-machine_opts = "-machine virt,gic-version=max";
memory_size = "150M";
-arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
+arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
+"-kernel %s", bootpath);
start_address = ARM_TEST_MEM_START;
end_address = ARM_TEST_MEM_END;
@@ -685,14 +683,13 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
shmem_opts = g_strdup("");
}
-cmd_source = g_strdup_printf("-accel kvm%s -accel tcg %s "
+cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
"-name source,debug-threads=on "
"-m %s "
"-serial file:%s/src_serial "
"%s %s %s %s %s",
args->use_dirty_ring ?
",dirty-ring-size=4096" : "",
- machine_opts ? machine_opts : "",
memory_size, tmpfs,
arch_opts ? arch_opts : "",
arch_source ? arch_source : "",
@@ -706,7 +703,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
&got_src_stop);
}
-cmd_target = g_strdup_printf("-accel kvm%s -accel tcg %s "
+cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
"-name target,debug-threads=on "
"-m %s "
"-serial file:%s/dest_serial "
@@ -714,7 +711,6 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
"%s %s %s %s %s",
args->use_dirty_ring ?
",dirty-ring-size=4096" : "",
- machine_opts ? machine_opts : "",
memory_size, tmpfs, uri,
arch_opts ? arch_opts : "",
arch_target ? arch_target : "",
--
2.40.1
[PULL 10/30] migration: Extend query-migrate to provide dirty page limit info
From: Hyman Huang(黄勇)
Extend query-migrate to provide throttle time and estimated
ring full time with dirty-limit capability enabled, through which
we can observe if dirty limit take effect during live migration.
Signed-off-by: Hyman Huang(黄勇)
Reviewed-by: Markus Armbruster
Reviewed-by: Juan Quintela
Message-ID: <168733225273.5845.1587182678887974167...@git.sr.ht>
Signed-off-by: Juan Quintela
---
qapi/migration.json| 16 +-
include/sysemu/dirtylimit.h| 2 ++
migration/migration-hmp-cmds.c | 10 +
migration/migration.c | 10 +
softmmu/dirtylimit.c | 39 ++
5 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 621e6604c6..e9b24fc410 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -250,6 +250,18 @@
# blocked. Present and non-empty when migration is blocked.
# (since 6.0)
#
+# @dirty-limit-throttle-time-per-round: Maximum throttle time (in
microseconds) of virtual
+# CPUs each dirty ring full round, which
shows how
+# MigrationCapability dirty-limit
affects the guest
+# during live migration. (since 8.1)
+#
+# @dirty-limit-ring-full-time: Estimated average dirty ring full time (in
microseconds)
+# each dirty ring full round, note that the value
equals
+# dirty ring memory size divided by average dirty
page rate
+# of virtual CPU, which can be used to observe
the average
+# memory load of virtual CPU indirectly. Note
that zero
+# means guest doesn't dirty memory (since 8.1)
+#
# Since: 0.14
##
{ 'struct': 'MigrationInfo',
@@ -267,7 +279,9 @@
'*postcopy-blocktime' : 'uint32',
'*postcopy-vcpu-blocktime': ['uint32'],
'*compression': 'CompressionStats',
- '*socket-address': ['SocketAddress'] } }
+ '*socket-address': ['SocketAddress'],
+ '*dirty-limit-throttle-time-per-round': 'uint64',
+ '*dirty-limit-ring-full-time': 'uint64'} }
##
# @query-migrate:
diff --git a/include/sysemu/dirtylimit.h b/include/sysemu/dirtylimit.h
index 8d2c1f3a6b..d11edb 100644
--- a/include/sysemu/dirtylimit.h
+++ b/include/sysemu/dirtylimit.h
@@ -34,4 +34,6 @@ void dirtylimit_set_vcpu(int cpu_index,
void dirtylimit_set_all(uint64_t quota,
bool enable);
void dirtylimit_vcpu_execute(CPUState *cpu);
+uint64_t dirtylimit_throttle_time_per_round(void);
+uint64_t dirtylimit_ring_full_time(void);
#endif
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 35e8020bbf..c115ef2d23 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -190,6 +190,16 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
info->cpu_throttle_percentage);
}
+if (info->has_dirty_limit_throttle_time_per_round) {
+monitor_printf(mon, "dirty-limit throttle time: %" PRIu64 " us\n",
+ info->dirty_limit_throttle_time_per_round);
+}
+
+if (info->has_dirty_limit_ring_full_time) {
+monitor_printf(mon, "dirty-limit ring full time: %" PRIu64 " us\n",
+ info->dirty_limit_ring_full_time);
+}
+
if (info->has_postcopy_blocktime) {
monitor_printf(mon, "postcopy blocktime: %u\n",
info->postcopy_blocktime);
diff --git a/migration/migration.c b/migration/migration.c
index c101784dfa..719f91573f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -64,6 +64,7 @@
#include "yank_functions.h"
#include "sysemu/qtest.h"
#include "options.h"
+#include "sysemu/dirtylimit.h"
static NotifierList migration_state_notifiers =
NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -968,6 +969,15 @@ static void populate_ram_info(MigrationInfo *info,
MigrationState *s)
info->ram->dirty_pages_rate =
stat64_get(&mig_stats.dirty_pages_rate);
}
+
+if (migrate_dirty_limit() && dirtylimit_in_service()) {
+info->has_dirty_limit_throttle_time_per_round = true;
+info->dirty_limit_throttle_time_per_round =
+dirtylimit_throttle_time_per_round();
+
+info->has_dirty_limit_ring_full_time = true;
+info->dirty_limit_ring_full_time = dirtylimit_ring_full_time();
+}
}
static void populate_disk_info(MigrationInfo *info)
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index a6d854d161..3c275ee55b 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -565,6 +565,45 @@ out:
hmp_handle_error(mon, err);
}
+/* Return the max throttle time of each virtual CPU */
+uint64_t dirtylimit_throttle_time_per_round(void)
+{
+
[PULL 25/30] migration: Change qemu_file_transferred to noflush
We do a qemu_fclose() just after that, that also does a qemu_fflush(),
so remove one qemu_fflush().
Reviewed-by: Philippe Mathieu-Daudé
Message-ID: <20230530183941.7223-3-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
migration/savevm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index f26b455764..b2199d1039 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2952,7 +2952,7 @@ bool save_snapshot(const char *name, bool overwrite,
const char *vmstate,
goto the_end;
}
ret = qemu_savevm_state(f, errp);
-vm_state_size = qemu_file_transferred(f);
+vm_state_size = qemu_file_transferred_noflush(f);
ret2 = qemu_fclose(f);
if (ret < 0) {
goto the_end;
--
2.40.1
[PULL 17/30] migration-test: Add bootfile_create/delete() functions
The bootsector code is read only from the guest (otherwise we are
going to have problems with it being read from both source and
destination).
Create a single copy for all the tests.
Reviewed-by: Peter Xu
Message-ID: <20230608224943.3877-10-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 50 ++--
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 0f80dbfe80..eb6a11e758 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -111,14 +111,47 @@ static char *bootpath;
#include "tests/migration/aarch64/a-b-kernel.h"
#include "tests/migration/s390x/a-b-bios.h"
-static void init_bootfile(void *content, size_t len)
+static void bootfile_create(char *dir)
{
+const char *arch = qtest_get_arch();
+unsigned char *content;
+size_t len;
+
+bootpath = g_strdup_printf("%s/bootsect", dir);
+if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+/* the assembled x86 boot sector should be exactly one sector large */
+g_assert(sizeof(x86_bootsect) == 512);
+content = x86_bootsect;
+len = sizeof(x86_bootsect);
+} else if (g_str_equal(arch, "s390x")) {
+content = s390x_elf;
+len = sizeof(s390x_elf);
+} else if (strcmp(arch, "ppc64") == 0) {
+/*
+ * sane architectures can be programmed at the boot prompt
+ */
+return;
+} else if (strcmp(arch, "aarch64") == 0) {
+content = aarch64_kernel;
+len = sizeof(aarch64_kernel);
+g_assert(sizeof(aarch64_kernel) <= ARM_TEST_MAX_KERNEL_SIZE);
+} else {
+g_assert_not_reached();
+}
+
FILE *bootfile = fopen(bootpath, "wb");
g_assert_cmpint(fwrite(content, len, 1, bootfile), ==, 1);
fclose(bootfile);
}
+static void bootfile_delete(void)
+{
+unlink(bootpath);
+g_free(bootpath);
+bootpath = NULL;
+}
+
/*
* Wait for some output in the serial output file,
* we get an 'A' followed by an endless string of 'B's
@@ -622,15 +655,11 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
got_src_stop = false;
got_dst_resume = false;
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-/* the assembled x86 boot sector should be exactly one sector large */
-assert(sizeof(x86_bootsect) == 512);
-init_bootfile(x86_bootsect, sizeof(x86_bootsect));
memory_size = "150M";
arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
start_address = X86_TEST_MEM_START;
end_address = X86_TEST_MEM_END;
} else if (g_str_equal(arch, "s390x")) {
-init_bootfile(s390x_elf, sizeof(s390x_elf));
memory_size = "128M";
arch_opts = g_strdup_printf("-bios %s", bootpath);
start_address = S390_TEST_MEM_START;
@@ -645,14 +674,11 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
"until'", end_address, start_address);
arch_opts = g_strdup("-nodefaults -machine vsmt=8");
} else if (strcmp(arch, "aarch64") == 0) {
-init_bootfile(aarch64_kernel, sizeof(aarch64_kernel));
memory_size = "150M";
arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
"-kernel %s", bootpath);
start_address = ARM_TEST_MEM_START;
end_address = ARM_TEST_MEM_END;
-
-g_assert(sizeof(aarch64_kernel) <= ARM_TEST_MAX_KERNEL_SIZE);
} else {
g_assert_not_reached();
}
@@ -2493,9 +2519,6 @@ static QTestState *dirtylimit_start_vm(void)
const char *arch = qtest_get_arch();
assert((strcmp(arch, "x86_64") == 0));
-assert(sizeof(x86_bootsect) == 512);
-init_bootfile(x86_bootsect, sizeof(x86_bootsect));
-
cmd = g_strdup_printf("-accel kvm,dirty-ring-size=4096 "
"-name dirtylimit-test,debug-threads=on "
"-m 150M -smp 1 "
@@ -2671,7 +2694,7 @@ int main(int argc, char **argv)
g_get_tmp_dir(), err->message);
}
g_assert(tmpfs);
-bootpath = g_strdup_printf("%s/bootsect", tmpfs);
+bootfile_create(tmpfs);
module_call_init(MODULE_INIT_QOM);
@@ -2815,8 +2838,7 @@ int main(int argc, char **argv)
g_assert_cmpint(ret, ==, 0);
-cleanup("bootsect");
-g_free(bootpath);
+bootfile_delete();
ret = rmdir(tmpfs);
if (ret != 0) {
g_test_message("unable to rmdir: path (%s): %s",
--
2.40.1
[PULL 07/30] migration: Refactor auto-converge capability logic
From: Hyman Huang(黄勇)
Check if block migration is running before throttling
guest down in auto-converge way.
Note that this modification is kind of like code clean,
because block migration does not depend on auto-converge
capability, so the order of checks can be adjusted.
Signed-off-by: Hyman Huang(黄勇)
Acked-by: Peter Xu
Reviewed-by: Juan Quintela
Message-Id: <168618975839.6361.1740763387474768865...@git.sr.ht>
Signed-off-by: Juan Quintela
---
migration/ram.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 5283a75f02..78746849b5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -995,7 +995,11 @@ static void migration_trigger_throttle(RAMState *rs)
/* During block migration the auto-converge logic incorrectly detects
* that ram migration makes no progress. Avoid this by disabling the
* throttling logic during the bulk phase of block migration. */
-if (migrate_auto_converge() && !blk_mig_bulk_active()) {
+if (blk_mig_bulk_active()) {
+return;
+}
+
+if (migrate_auto_converge()) {
/* The following detection logic can be refined later. For now:
Check to see if the ratio between dirtied bytes and the approx.
amount of bytes that just got transferred since the last time
--
2.40.1
[PULL 26/30] migration: Use qemu_file_transferred_noflush() for block migration.
We only care about the amount of bytes transferred. Flushing is done
by the system somewhere else.
Reviewed-by: Fabiano Rosas
Signed-off-by: Juan Quintela
Message-ID: <20230530183941.7223-4-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
migration/block.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/migration/block.c b/migration/block.c
index b9580a6c7e..b29e80bdc4 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -748,7 +748,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
static int block_save_iterate(QEMUFile *f, void *opaque)
{
int ret;
-uint64_t last_bytes = qemu_file_transferred(f);
+uint64_t last_bytes = qemu_file_transferred_noflush(f);
trace_migration_block_save("iterate", block_mig_state.submitted,
block_mig_state.transferred);
@@ -800,7 +800,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
}
qemu_put_be64(f, BLK_MIG_FLAG_EOS);
-uint64_t delta_bytes = qemu_file_transferred(f) - last_bytes;
+uint64_t delta_bytes = qemu_file_transferred_noflush(f) - last_bytes;
return (delta_bytes > 0);
}
--
2.40.1
[PULL 22/30] migration: enforce multifd and postcopy preempt to be set before incoming
From: Wei Wang
qemu_start_incoming_migration needs to check the number of multifd
channels or postcopy ram channels to configure the backlog parameter (i.e.
the maximum length to which the queue of pending connections for sockfd
may grow) of listen(). So enforce the usage of postcopy-preempt and
multifd as below:
- need to use "-incoming defer" on the destination; and
- set_capability and set_parameter need to be done before migrate_incoming
Otherwise, disable the use of the features and report error messages to
remind users to adjust the commands.
Signed-off-by: Wei Wang
Reviewed-by: Peter Xu
Reviewed-by: Juan Quintela
Message-ID: <20230606101910.20456-2-wei.w.w...@intel.com>
Signed-off-by: Juan Quintela
Acked-by: Juan Quintela
---
migration/options.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/migration/options.c b/migration/options.c
index ba1010e08b..c072c2fab7 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -433,6 +433,11 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
MIGRATION_CAPABILITY_VALIDATE_UUID,
MIGRATION_CAPABILITY_ZERO_COPY_SEND);
+static bool migrate_incoming_started(void)
+{
+return !!migration_incoming_get_current()->transport_data;
+}
+
/**
* @migration_caps_check - check capability compatibility
*
@@ -556,6 +561,12 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
Error **errp)
error_setg(errp, "Postcopy preempt not compatible with compress");
return false;
}
+
+if (migrate_incoming_started()) {
+error_setg(errp,
+ "Postcopy preempt must be set before incoming starts");
+return false;
+}
}
if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
@@ -563,6 +574,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
Error **errp)
error_setg(errp, "Multifd is not compatible with compress");
return false;
}
+if (migrate_incoming_started()) {
+error_setg(errp, "Multifd must be set before incoming starts");
+return false;
+}
}
if (new_caps[MIGRATION_CAPABILITY_DIRTY_LIMIT]) {
--
2.40.1
[PULL 16/30] migration-test: bootpath is the same for all tests and for all archs
So just make it a global variable.
Reviewed-by: Peter Xu
Message-ID: <20230608224943.3877-9-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 40967fdffc..0f80dbfe80 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -102,6 +102,7 @@ static bool ufd_version_check(void)
#endif
static char *tmpfs;
+static char *bootpath;
/* The boot file modifies memory area in [start_address, end_address)
* repeatedly. It outputs a 'B' at a fixed rate while it's still running.
@@ -110,7 +111,7 @@ static char *tmpfs;
#include "tests/migration/aarch64/a-b-kernel.h"
#include "tests/migration/s390x/a-b-bios.h"
-static void init_bootfile(const char *bootpath, void *content, size_t len)
+static void init_bootfile(void *content, size_t len)
{
FILE *bootfile = fopen(bootpath, "wb");
@@ -605,7 +606,6 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
g_autofree gchar *cmd_source = NULL;
g_autofree gchar *cmd_target = NULL;
const gchar *ignore_stderr;
-g_autofree char *bootpath = NULL;
g_autofree char *shmem_opts = NULL;
g_autofree char *shmem_path = NULL;
const char *kvm_opts = NULL;
@@ -621,17 +621,16 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
got_src_stop = false;
got_dst_resume = false;
-bootpath = g_strdup_printf("%s/bootsect", tmpfs);
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
/* the assembled x86 boot sector should be exactly one sector large */
assert(sizeof(x86_bootsect) == 512);
-init_bootfile(bootpath, x86_bootsect, sizeof(x86_bootsect));
+init_bootfile(x86_bootsect, sizeof(x86_bootsect));
memory_size = "150M";
arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
start_address = X86_TEST_MEM_START;
end_address = X86_TEST_MEM_END;
} else if (g_str_equal(arch, "s390x")) {
-init_bootfile(bootpath, s390x_elf, sizeof(s390x_elf));
+init_bootfile(s390x_elf, sizeof(s390x_elf));
memory_size = "128M";
arch_opts = g_strdup_printf("-bios %s", bootpath);
start_address = S390_TEST_MEM_START;
@@ -646,7 +645,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
"until'", end_address, start_address);
arch_opts = g_strdup("-nodefaults -machine vsmt=8");
} else if (strcmp(arch, "aarch64") == 0) {
-init_bootfile(bootpath, aarch64_kernel, sizeof(aarch64_kernel));
+init_bootfile(aarch64_kernel, sizeof(aarch64_kernel));
memory_size = "150M";
arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
"-kernel %s", bootpath);
@@ -764,7 +763,6 @@ static void test_migrate_end(QTestState *from, QTestState
*to, bool test_dest)
qtest_quit(to);
-cleanup("bootsect");
cleanup("migsocket");
cleanup("src_serial");
cleanup("dest_serial");
@@ -2493,12 +2491,10 @@ static QTestState *dirtylimit_start_vm(void)
QTestState *vm = NULL;
g_autofree gchar *cmd = NULL;
const char *arch = qtest_get_arch();
-g_autofree char *bootpath = NULL;
assert((strcmp(arch, "x86_64") == 0));
-bootpath = g_strdup_printf("%s/bootsect", tmpfs);
assert(sizeof(x86_bootsect) == 512);
-init_bootfile(bootpath, x86_bootsect, sizeof(x86_bootsect));
+init_bootfile(x86_bootsect, sizeof(x86_bootsect));
cmd = g_strdup_printf("-accel kvm,dirty-ring-size=4096 "
"-name dirtylimit-test,debug-threads=on "
@@ -2514,7 +2510,6 @@ static QTestState *dirtylimit_start_vm(void)
static void dirtylimit_stop_vm(QTestState *vm)
{
qtest_quit(vm);
-cleanup("bootsect");
cleanup("vm_serial");
}
@@ -2676,6 +2671,7 @@ int main(int argc, char **argv)
g_get_tmp_dir(), err->message);
}
g_assert(tmpfs);
+bootpath = g_strdup_printf("%s/bootsect", tmpfs);
module_call_init(MODULE_INIT_QOM);
@@ -2819,6 +2815,8 @@ int main(int argc, char **argv)
g_assert_cmpint(ret, ==, 0);
+cleanup("bootsect");
+g_free(bootpath);
ret = rmdir(tmpfs);
if (ret != 0) {
g_test_message("unable to rmdir: path (%s): %s",
--
2.40.1
[PULL 01/30] migration/multifd: Rename threadinfo.c functions
From: Fabiano Rosas
We're about to add more functions to this file so make it use the same
coding style as the rest of the code.
Signed-off-by: Fabiano Rosas
Reviewed-by: Juan Quintela
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Peter Xu
Message-Id: <20230607161306.31425-2-faro...@suse.de>
Signed-off-by: Juan Quintela
---
migration/threadinfo.h | 5 ++---
migration/migration.c | 4 ++--
migration/multifd.c| 4 ++--
migration/threadinfo.c | 4 ++--
4 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/migration/threadinfo.h b/migration/threadinfo.h
index 4d69423c0a..8aa6999d58 100644
--- a/migration/threadinfo.h
+++ b/migration/threadinfo.h
@@ -23,6 +23,5 @@ struct MigrationThread {
QLIST_ENTRY(MigrationThread) node;
};
-MigrationThread *MigrationThreadAdd(const char *name, int thread_id);
-
-void MigrationThreadDel(MigrationThread *info);
+MigrationThread *migration_threads_add(const char *name, int thread_id);
+void migration_threads_remove(MigrationThread *info);
diff --git a/migration/migration.c b/migration/migration.c
index dc05c6f6ea..3a001dd042 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2922,7 +2922,7 @@ static void *migration_thread(void *opaque)
MigThrError thr_error;
bool urgent = false;
-thread = MigrationThreadAdd("live_migration", qemu_get_thread_id());
+thread = migration_threads_add("live_migration", qemu_get_thread_id());
rcu_register_thread();
@@ -3000,7 +3000,7 @@ static void *migration_thread(void *opaque)
migration_iteration_finish(s);
object_unref(OBJECT(s));
rcu_unregister_thread();
-MigrationThreadDel(thread);
+migration_threads_remove(thread);
return NULL;
}
diff --git a/migration/multifd.c b/migration/multifd.c
index 3387d8277f..4c6cee6547 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -651,7 +651,7 @@ static void *multifd_send_thread(void *opaque)
int ret = 0;
bool use_zero_copy_send = migrate_zero_copy_send();
-thread = MigrationThreadAdd(p->name, qemu_get_thread_id());
+thread = migration_threads_add(p->name, qemu_get_thread_id());
trace_multifd_send_thread_start(p->id);
rcu_register_thread();
@@ -767,7 +767,7 @@ out:
qemu_mutex_unlock(&p->mutex);
rcu_unregister_thread();
-MigrationThreadDel(thread);
+migration_threads_remove(thread);
trace_multifd_send_thread_end(p->id, p->num_packets,
p->total_normal_pages);
return NULL;
diff --git a/migration/threadinfo.c b/migration/threadinfo.c
index 1de8b31855..3dd9b14ae6 100644
--- a/migration/threadinfo.c
+++ b/migration/threadinfo.c
@@ -14,7 +14,7 @@
static QLIST_HEAD(, MigrationThread) migration_threads;
-MigrationThread *MigrationThreadAdd(const char *name, int thread_id)
+MigrationThread *migration_threads_add(const char *name, int thread_id)
{
MigrationThread *thread = g_new0(MigrationThread, 1);
thread->name = name;
@@ -25,7 +25,7 @@ MigrationThread *MigrationThreadAdd(const char *name, int
thread_id)
return thread;
}
-void MigrationThreadDel(MigrationThread *thread)
+void migration_threads_remove(MigrationThread *thread)
{
if (thread) {
QLIST_REMOVE(thread, node);
--
2.40.1
[PULL 24/30] qemu-file: Rename qemu_file_transferred_ fast -> noflush
Fast don't say much. Noflush indicates more clearly that it is like
qemu_file_transferred but without the flush.
Reviewed-by: Philippe Mathieu-Daudé
Message-ID: <20230530183941.7223-2-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
migration/qemu-file.h | 11 +--
migration/qemu-file.c | 2 +-
migration/savevm.c| 4 ++--
migration/vmstate.c | 4 ++--
4 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index e649718492..aa6eee66da 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -86,16 +86,15 @@ int qemu_fclose(QEMUFile *f);
uint64_t qemu_file_transferred(QEMUFile *f);
/*
- * qemu_file_transferred_fast:
+ * qemu_file_transferred_noflush:
*
- * As qemu_file_transferred except for writable
- * files, where no flush is performed and the reported
- * amount will include the size of any queued buffers,
- * on top of the amount actually transferred.
+ * As qemu_file_transferred except for writable files, where no flush
+ * is performed and the reported amount will include the size of any
+ * queued buffers, on top of the amount actually transferred.
*
* Returns: the total bytes transferred and queued
*/
-uint64_t qemu_file_transferred_fast(QEMUFile *f);
+uint64_t qemu_file_transferred_noflush(QEMUFile *f);
/*
* put_buffer without copying the buffer.
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index acc282654a..fdf115b5da 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -694,7 +694,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
return result;
}
-uint64_t qemu_file_transferred_fast(QEMUFile *f)
+uint64_t qemu_file_transferred_noflush(QEMUFile *f)
{
uint64_t ret = f->total_transferred;
int i;
diff --git a/migration/savevm.c b/migration/savevm.c
index bc284087f9..f26b455764 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -927,9 +927,9 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
JSONWriter *vmdesc)
{
-uint64_t old_offset = qemu_file_transferred_fast(f);
+uint64_t old_offset = qemu_file_transferred_noflush(f);
se->ops->save_state(f, se->opaque);
-uint64_t size = qemu_file_transferred_fast(f) - old_offset;
+uint64_t size = qemu_file_transferred_noflush(f) - old_offset;
if (vmdesc) {
json_writer_int64(vmdesc, "size", size);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index af01d54b6f..31842c3afb 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -361,7 +361,7 @@ int vmstate_save_state_v(QEMUFile *f, const
VMStateDescription *vmsd,
void *curr_elem = first_elem + size * i;
vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
-old_offset = qemu_file_transferred_fast(f);
+old_offset = qemu_file_transferred_noflush(f);
if (field->flags & VMS_ARRAY_OF_POINTER) {
assert(curr_elem);
curr_elem = *(void **)curr_elem;
@@ -391,7 +391,7 @@ int vmstate_save_state_v(QEMUFile *f, const
VMStateDescription *vmsd,
return ret;
}
-written_bytes = qemu_file_transferred_fast(f) - old_offset;
+written_bytes = qemu_file_transferred_noflush(f) - old_offset;
vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes,
i);
/* Compressed arrays only care about the first element */
--
2.40.1
[PULL 09/30] migration: Implement dirty-limit convergence algo
From: Hyman Huang(黄勇)
Implement dirty-limit convergence algo for live migration,
which is kind of like auto-converge algo but using dirty-limit
instead of cpu throttle to make migration convergent.
Enable dirty page limit if dirty_rate_high_cnt greater than 2
when dirty-limit capability enabled, Disable dirty-limit if
migration be canceled.
Note that "set_vcpu_dirty_limit", "cancel_vcpu_dirty_limit"
commands are not allowed during dirty-limit live migration.
Signed-off-by: Hyman Huang(黄勇)
Reviewed-by: Markus Armbruster
Message-ID: <168733225273.5845.1587182678887974167...@git.sr.ht>
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
---
migration/migration.c | 3 +++
migration/ram.c| 36
softmmu/dirtylimit.c | 29 +
migration/trace-events | 1 +
4 files changed, 69 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 3a001dd042..c101784dfa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -165,6 +165,9 @@ void migration_cancel(const Error *error)
if (error) {
migrate_set_error(current_migration, error);
}
+if (migrate_dirty_limit()) {
+qmp_cancel_vcpu_dirty_limit(false, -1, NULL);
+}
migrate_fd_cancel(current_migration);
}
diff --git a/migration/ram.c b/migration/ram.c
index b6559f9312..8a86363216 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -46,6 +46,7 @@
#include "qapi/error.h"
#include "qapi/qapi-types-migration.h"
#include "qapi/qapi-events-migration.h"
+#include "qapi/qapi-commands-migration.h"
#include "qapi/qmp/qerror.h"
#include "trace.h"
#include "exec/ram_addr.h"
@@ -59,6 +60,8 @@
#include "multifd.h"
#include "sysemu/runstate.h"
#include "options.h"
+#include "sysemu/dirtylimit.h"
+#include "sysemu/kvm.h"
#include "hw/boards.h" /* for machine_dump_guest_core() */
@@ -984,6 +987,37 @@ static void migration_update_rates(RAMState *rs, int64_t
end_time)
}
}
+/*
+ * Enable dirty-limit to throttle down the guest
+ */
+static void migration_dirty_limit_guest(void)
+{
+/*
+ * dirty page rate quota for all vCPUs fetched from
+ * migration parameter 'vcpu_dirty_limit'
+ */
+static int64_t quota_dirtyrate;
+MigrationState *s = migrate_get_current();
+
+/*
+ * If dirty limit already enabled and migration parameter
+ * vcpu-dirty-limit untouched.
+ */
+if (dirtylimit_in_service() &&
+quota_dirtyrate == s->parameters.vcpu_dirty_limit) {
+return;
+}
+
+quota_dirtyrate = s->parameters.vcpu_dirty_limit;
+
+/*
+ * Set all vCPU a quota dirtyrate, note that the second
+ * parameter will be ignored if setting all vCPU for the vm
+ */
+qmp_set_vcpu_dirty_limit(false, -1, quota_dirtyrate, NULL);
+trace_migration_dirty_limit_guest(quota_dirtyrate);
+}
+
static void migration_trigger_throttle(RAMState *rs)
{
uint64_t threshold = migrate_throttle_trigger_threshold();
@@ -1013,6 +1047,8 @@ static void migration_trigger_throttle(RAMState *rs)
trace_migration_throttle();
mig_throttle_guest_down(bytes_dirty_period,
bytes_dirty_threshold);
+} else if (migrate_dirty_limit()) {
+migration_dirty_limit_guest();
}
}
}
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 942d876523..a6d854d161 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -436,6 +436,23 @@ static void dirtylimit_cleanup(void)
dirtylimit_state_finalize();
}
+/*
+ * dirty page rate limit is not allowed to set if migration
+ * is running with dirty-limit capability enabled.
+ */
+static bool dirtylimit_is_allowed(void)
+{
+MigrationState *ms = migrate_get_current();
+
+if (migration_is_running(ms->state) &&
+(!qemu_thread_is_self(&ms->thread)) &&
+migrate_dirty_limit() &&
+dirtylimit_in_service()) {
+return false;
+}
+return true;
+}
+
void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index,
int64_t cpu_index,
Error **errp)
@@ -449,6 +466,12 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index,
return;
}
+if (!dirtylimit_is_allowed()) {
+error_setg(errp, "can't cancel dirty page rate limit while"
+ " migration is running");
+return;
+}
+
if (!dirtylimit_in_service()) {
return;
}
@@ -499,6 +522,12 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index,
return;
}
+if (!dirtylimit_is_allowed()) {
+error_setg(errp, "can't set dirty page rate limit while"
+ " migration is running");
+return;
+}
+
if (!dirty_rate) {
qmp_cancel_vcpu_dirty_limit(has_cpu_index, cpu_index, errp);
return;
diff --git a/migration/trace-events b/migration/trace-events
index cdaef7a1ea..c5cb280d95 1
[PULL 15/30] migration-test: Create kvm_opts
So arch_dirty_ring option becomes one option like the others.
Reviewed-by: Peter Xu
Message-ID: <20230608224943.3877-8-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index fc3337b7bb..40967fdffc 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -608,6 +608,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
g_autofree char *bootpath = NULL;
g_autofree char *shmem_opts = NULL;
g_autofree char *shmem_path = NULL;
+const char *kvm_opts = NULL;
const char *arch = qtest_get_arch();
const char *memory_size;
@@ -683,13 +684,16 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
shmem_opts = g_strdup("");
}
+if (args->use_dirty_ring) {
+kvm_opts = ",dirty-ring-size=4096";
+}
+
cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
"-name source,debug-threads=on "
"-m %s "
"-serial file:%s/src_serial "
"%s %s %s %s %s",
- args->use_dirty_ring ?
- ",dirty-ring-size=4096" : "",
+ kvm_opts ? kvm_opts : "",
memory_size, tmpfs,
arch_opts ? arch_opts : "",
arch_source ? arch_source : "",
@@ -709,8 +713,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
"-serial file:%s/dest_serial "
"-incoming %s "
"%s %s %s %s %s",
- args->use_dirty_ring ?
- ",dirty-ring-size=4096" : "",
+ kvm_opts ? kvm_opts : "",
memory_size, tmpfs, uri,
arch_opts ? arch_opts : "",
arch_target ? arch_target : "",
--
2.40.1
[PULL 27/30] qemu_file: Make qemu_file_is_writable() static
It is not used outside of qemu_file, and it shouldn't.
Signed-off-by: Juan Quintela
Message-ID: <20230530183941.7223-19-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
migration/qemu-file.h | 1 -
migration/qemu-file.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index aa6eee66da..a081ef6c3f 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -103,7 +103,6 @@ uint64_t qemu_file_transferred_noflush(QEMUFile *f);
void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
bool may_free);
bool qemu_file_mode_is_not_valid(const char *mode);
-bool qemu_file_is_writable(QEMUFile *f);
#include "migration/qemu-file-types.h"
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index fdf115b5da..9a89e17924 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -228,7 +228,7 @@ void qemu_file_set_error(QEMUFile *f, int ret)
qemu_file_set_error_obj(f, ret, NULL);
}
-bool qemu_file_is_writable(QEMUFile *f)
+static bool qemu_file_is_writable(QEMUFile *f)
{
return f->is_writable;
}
--
2.40.1
[PULL 12/30] migration-test: Make machine_opts regular with other options
Reviewed-by: Peter Xu
Signed-off-by: Juan Quintela
Message-ID: <20230608224943.3877-5-quint...@redhat.com>
---
tests/qtest/migration-test.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index c5e0c69c6b..79157d600b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -637,7 +637,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
start_address = S390_TEST_MEM_START;
end_address = S390_TEST_MEM_END;
} else if (strcmp(arch, "ppc64") == 0) {
-machine_opts = "vsmt=8";
+machine_opts = "-machine vsmt=8";
memory_size = "256M";
start_address = PPC_TEST_MEM_START;
end_address = PPC_TEST_MEM_END;
@@ -649,7 +649,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
arch_target = g_strdup("-nodefaults");
} else if (strcmp(arch, "aarch64") == 0) {
init_bootfile(bootpath, aarch64_kernel, sizeof(aarch64_kernel));
-machine_opts = "virt,gic-version=max";
+machine_opts = "-machine virt,gic-version=max";
memory_size = "150M";
arch_source = g_strdup_printf("-cpu max "
"-kernel %s",
@@ -689,14 +689,13 @@ static int test_migrate_start(QTestState **from,
QTestState **to,
shmem_opts = g_strdup("");
}
-cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
+cmd_source = g_strdup_printf("-accel kvm%s -accel tcg %s "
"-name source,debug-threads=on "
"-m %s "
"-serial file:%s/src_serial "
"%s %s %s %s",
args->use_dirty_ring ?
",dirty-ring-size=4096" : "",
- machine_opts ? " -machine " : "",
machine_opts ? machine_opts : "",
memory_size, tmpfs,
arch_source, shmem_opts,
@@ -709,7 +708,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
&got_src_stop);
}
-cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
+cmd_target = g_strdup_printf("-accel kvm%s -accel tcg %s "
"-name target,debug-threads=on "
"-m %s "
"-serial file:%s/dest_serial "
@@ -717,7 +716,6 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
"%s %s %s %s",
args->use_dirty_ring ?
",dirty-ring-size=4096" : "",
- machine_opts ? " -machine " : "",
machine_opts ? machine_opts : "",
memory_size, tmpfs, uri,
arch_target, shmem_opts,
--
2.40.1
[PULL 18/30] migration-test: dirtylimit checks for x86_64 arch before
So no need to assert we are in x86_64.
Once there, refactor the function to remove useless variables.
Reviewed-by: Peter Xu
Message-ID: <20230608224943.3877-11-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index eb6a11e758..fbe9db23cf 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2515,10 +2515,7 @@ static int64_t get_limit_rate(QTestState *who)
static QTestState *dirtylimit_start_vm(void)
{
QTestState *vm = NULL;
-g_autofree gchar *cmd = NULL;
-const char *arch = qtest_get_arch();
-
-assert((strcmp(arch, "x86_64") == 0));
+g_autofree gchar *
cmd = g_strdup_printf("-accel kvm,dirty-ring-size=4096 "
"-name dirtylimit-test,debug-threads=on "
"-m 150M -smp 1 "
--
2.40.1
[PULL 30/30] migration/rdma: Split qemu_fopen_rdma() into input/output functions
This is how everything else in QEMUFile is structured.
As a bonus they are three less lines of code.
Reviewed-by: Peter Xu
Message-ID: <20230530183941.7223-17-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
migration/qemu-file.h | 1 -
migration/qemu-file.c | 12
migration/rdma.c | 39 +++
3 files changed, 19 insertions(+), 33 deletions(-)
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 8b8b7d27fe..47015f5201 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -102,7 +102,6 @@ uint64_t qemu_file_transferred_noflush(QEMUFile *f);
*/
void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
bool may_free);
-bool qemu_file_mode_is_not_valid(const char *mode);
#include "migration/qemu-file-types.h"
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index d30bf3c377..19c33c9985 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -100,18 +100,6 @@ int qemu_file_shutdown(QEMUFile *f)
return 0;
}
-bool qemu_file_mode_is_not_valid(const char *mode)
-{
-if (mode == NULL ||
-(mode[0] != 'r' && mode[0] != 'w') ||
-mode[1] != 'b' || mode[2] != 0) {
-fprintf(stderr, "qemu_fopen: Argument validity check failed\n");
-return true;
-}
-
-return false;
-}
-
static QEMUFile *qemu_file_new_impl(QIOChannel *ioc, bool is_writable)
{
QEMUFile *f;
diff --git a/migration/rdma.c b/migration/rdma.c
index dd1c039e6c..ca430d319d 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4053,27 +4053,26 @@ static void qio_channel_rdma_register_types(void)
type_init(qio_channel_rdma_register_types);
-static QEMUFile *qemu_fopen_rdma(RDMAContext *rdma, const char *mode)
+static QEMUFile *rdma_new_input(RDMAContext *rdma)
{
-QIOChannelRDMA *rioc;
+QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
-if (qemu_file_mode_is_not_valid(mode)) {
-return NULL;
-}
+rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc));
+rioc->rdmain = rdma;
+rioc->rdmaout = rdma->return_path;
+qemu_file_set_hooks(rioc->file, &rdma_read_hooks);
-rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
+return rioc->file;
+}
-if (mode[0] == 'w') {
-rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc));
-rioc->rdmaout = rdma;
-rioc->rdmain = rdma->return_path;
-qemu_file_set_hooks(rioc->file, &rdma_write_hooks);
-} else {
-rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc));
-rioc->rdmain = rdma;
-rioc->rdmaout = rdma->return_path;
-qemu_file_set_hooks(rioc->file, &rdma_read_hooks);
-}
+static QEMUFile *rdma_new_output(RDMAContext *rdma)
+{
+QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
+
+rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc));
+rioc->rdmaout = rdma;
+rioc->rdmain = rdma->return_path;
+qemu_file_set_hooks(rioc->file, &rdma_write_hooks);
return rioc->file;
}
@@ -4099,9 +4098,9 @@ static void rdma_accept_incoming_migration(void *opaque)
return;
}
-f = qemu_fopen_rdma(rdma, "rb");
+f = rdma_new_input(rdma);
if (f == NULL) {
-fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma\n");
+fprintf(stderr, "RDMA ERROR: could not open RDMA for input\n");
qemu_rdma_cleanup(rdma);
return;
}
@@ -4224,7 +4223,7 @@ void rdma_start_outgoing_migration(void *opaque,
trace_rdma_start_outgoing_migration_after_rdma_connect();
-s->to_dst_file = qemu_fopen_rdma(rdma, "wb");
+s->to_dst_file = rdma_new_output(rdma);
migrate_fd_connect(s, NULL);
return;
return_path_err:
--
2.40.1
[PULL 02/30] migration/multifd: Protect accesses to migration_threads
From: Fabiano Rosas
This doubly linked list is common for all the multifd and migration
threads so we need to avoid concurrent access.
Add a mutex to protect the data from concurrent access. This fixes a
crash when removing two MigrationThread objects from the list at the
same time during cleanup of multifd threads.
Fixes: 671326201d ("migration: Introduce interface query-migrationthreads")
Signed-off-by: Fabiano Rosas
Reviewed-by: Peter Xu
Reviewed-by: Juan Quintela
Message-Id: <20230607161306.31425-3-faro...@suse.de>
Signed-off-by: Juan Quintela
---
migration/threadinfo.h | 2 --
migration/threadinfo.c | 15 ++-
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/migration/threadinfo.h b/migration/threadinfo.h
index 8aa6999d58..2f356ff312 100644
--- a/migration/threadinfo.h
+++ b/migration/threadinfo.h
@@ -10,8 +10,6 @@
* See the COPYING file in the top-level directory.
*/
-#include "qemu/queue.h"
-#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-migration.h"
diff --git a/migration/threadinfo.c b/migration/threadinfo.c
index 3dd9b14ae6..262990dd75 100644
--- a/migration/threadinfo.c
+++ b/migration/threadinfo.c
@@ -10,23 +10,35 @@
* See the COPYING file in the top-level directory.
*/
+#include "qemu/osdep.h"
+#include "qemu/queue.h"
+#include "qemu/lockable.h"
#include "threadinfo.h"
+QemuMutex migration_threads_lock;
static QLIST_HEAD(, MigrationThread) migration_threads;
+static void __attribute__((constructor)) migration_threads_init(void)
+{
+qemu_mutex_init(&migration_threads_lock);
+}
+
MigrationThread *migration_threads_add(const char *name, int thread_id)
{
MigrationThread *thread = g_new0(MigrationThread, 1);
thread->name = name;
thread->thread_id = thread_id;
-QLIST_INSERT_HEAD(&migration_threads, thread, node);
+WITH_QEMU_LOCK_GUARD(&migration_threads_lock) {
+QLIST_INSERT_HEAD(&migration_threads, thread, node);
+}
return thread;
}
void migration_threads_remove(MigrationThread *thread)
{
+QEMU_LOCK_GUARD(&migration_threads_lock);
if (thread) {
QLIST_REMOVE(thread, node);
g_free(thread);
@@ -39,6 +51,7 @@ MigrationThreadInfoList *qmp_query_migrationthreads(Error
**errp)
MigrationThreadInfoList **tail = &head;
MigrationThread *thread = NULL;
+QEMU_LOCK_GUARD(&migration_threads_lock);
QLIST_FOREACH(thread, &migration_threads, node) {
MigrationThreadInfo *info = g_new0(MigrationThreadInfo, 1);
info->name = g_strdup(thread->name);
--
2.40.1
[PULL 28/30] qemu-file: Simplify qemu_file_shutdown()
Reviewed-by: Peter Xu
Message-ID: <20230530183941.7223-20-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
migration/qemu-file.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 9a89e17924..4c577bdff8 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -65,8 +65,6 @@ struct QEMUFile {
*/
int qemu_file_shutdown(QEMUFile *f)
{
-int ret = 0;
-
/*
* We must set qemufile error before the real shutdown(), otherwise
* there can be a race window where we thought IO all went though
@@ -96,10 +94,10 @@ int qemu_file_shutdown(QEMUFile *f)
}
if (qio_channel_shutdown(f->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL) < 0) {
-ret = -EIO;
+return -EIO;
}
-return ret;
+return 0;
}
bool qemu_file_mode_is_not_valid(const char *mode)
--
2.40.1
[PULL 20/30] migration: Update error description whenever migration fails
From: Tejus GK
There are places in migration.c where the migration is marked failed with
MIGRATION_STATUS_FAILED, but the failure reason is never updated. Hence
libvirt doesn't know why the migration failed when it queries for it.
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Tejus GK
Message-ID: <20230621130940.178659-2-tejus...@nutanix.com>
Signed-off-by: Juan Quintela
---
migration/migration.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 719f91573f..e6bff2e848 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1679,7 +1679,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
if (!(has_resume && resume)) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
+error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
"a valid migration protocol");
migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_FAILED);
@@ -2066,7 +2066,7 @@ migration_wait_main_channel(MigrationState *ms)
* Switch from normal iteration to postcopy
* Returns non-0 on error
*/
-static int postcopy_start(MigrationState *ms)
+static int postcopy_start(MigrationState *ms, Error **errp)
{
int ret;
QIOChannelBuffer *bioc;
@@ -2176,7 +2176,7 @@ static int postcopy_start(MigrationState *ms)
*/
ret = qemu_file_get_error(ms->to_dst_file);
if (ret) {
-error_report("postcopy_start: Migration stream errored (pre package)");
+error_setg(errp, "postcopy_start: Migration stream errored (pre
package)");
goto fail_closefb;
}
@@ -2213,7 +2213,7 @@ static int postcopy_start(MigrationState *ms)
ret = qemu_file_get_error(ms->to_dst_file);
if (ret) {
-error_report("postcopy_start: Migration stream errored");
+error_setg(errp, "postcopy_start: Migration stream errored");
migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
MIGRATION_STATUS_FAILED);
}
@@ -2720,6 +2720,7 @@ typedef enum {
static MigIterateState migration_iteration_run(MigrationState *s)
{
uint64_t must_precopy, can_postcopy;
+Error *local_err = NULL;
bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy);
@@ -2742,8 +2743,9 @@ static MigIterateState
migration_iteration_run(MigrationState *s)
/* Still a significant amount to transfer */
if (!in_postcopy && must_precopy <= s->threshold_size &&
qatomic_read(&s->start_postcopy)) {
-if (postcopy_start(s)) {
-error_report("%s: postcopy failed to start", __func__);
+if (postcopy_start(s, &local_err)) {
+migrate_set_error(s, local_err);
+error_report_err(local_err);
}
return MIG_ITERATE_SKIP;
}
@@ -3234,8 +3236,10 @@ void migrate_fd_connect(MigrationState *s, Error
*error_in)
*/
if (migrate_postcopy_ram() || migrate_return_path()) {
if (open_return_path_on_source(s, !resume)) {
-error_report("Unable to open return-path for postcopy");
+error_setg(&local_err, "Unable to open return-path for postcopy");
migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+migrate_set_error(s, local_err);
+error_report_err(local_err);
migrate_fd_cleanup(s);
return;
}
@@ -3259,6 +3263,7 @@ void migrate_fd_connect(MigrationState *s, Error
*error_in)
}
if (multifd_save_setup(&local_err) != 0) {
+migrate_set_error(s, local_err);
error_report_err(local_err);
migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_FAILED);
--
2.40.1
[PULL 29/30] qemu-file: Make qemu_file_get_error_obj() static
It was not used outside of qemu_file.c anyways.
Reviewed-by: Peter Xu
Message-ID: <20230530183941.7223-21-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
migration/qemu-file.h | 1 -
migration/qemu-file.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index a081ef6c3f..8b8b7d27fe 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -128,7 +128,6 @@ void qemu_file_skip(QEMUFile *f, int size);
* accounting information tracks the total migration traffic.
*/
void qemu_file_credit_transfer(QEMUFile *f, size_t size);
-int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
int qemu_file_get_error_obj_any(QEMUFile *f1, QEMUFile *f2, Error **errp);
void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
void qemu_file_set_error(QEMUFile *f, int ret);
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 4c577bdff8..d30bf3c377 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -158,7 +158,7 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks
*hooks)
* is not 0.
*
*/
-int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
+static int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
{
if (errp) {
*errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
--
2.40.1
[PULL 11/30] migration-test: Be consistent for ppc
It makes no sense that we don't have the same configuration on both sides.
Reviewed-by: Laurent Vivier
Message-ID: <20230608224943.3877-2-quint...@redhat.com>
Signed-off-by: Juan Quintela
---
tests/qtest/migration-test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b0c355bbd9..c5e0c69c6b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -646,7 +646,7 @@ static int test_migrate_start(QTestState **from, QTestState
**to,
"'nvramrc=hex .\" _\" begin %x %x "
"do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
"until'", end_address, start_address);
-arch_target = g_strdup("");
+arch_target = g_strdup("-nodefaults");
} else if (strcmp(arch, "aarch64") == 0) {
init_bootfile(bootpath, aarch64_kernel, sizeof(aarch64_kernel));
machine_opts = "virt,gic-version=max";
--
2.40.1
[PULL 04/30] qapi/migration: Introduce x-vcpu-dirty-limit-period parameter
From: Hyman Huang(黄勇)
Introduce "x-vcpu-dirty-limit-period" migration experimental
parameter, which is in the range of 1 to 1000ms and used to
make dirtyrate calculation period configurable.
Currently with the "x-vcpu-dirty-limit-period" varies, the
total time of live migration changes, test results show the
optimal value of "x-vcpu-dirty-limit-period" ranges from
500ms to 1000 ms. "x-vcpu-dirty-limit-period" should be made
stable once it proves best value can not be determined with
developer's experiments.
Signed-off-by: Hyman Huang(黄勇)
Reviewed-by: Markus Armbruster
Reviewed-by: Juan Quintela
Message-Id: <168618975839.6361.1740763387474768865...@git.sr.ht>
Signed-off-by: Juan Quintela
---
qapi/migration.json| 34 +++---
migration/migration-hmp-cmds.c | 8
migration/options.c| 28
3 files changed, 63 insertions(+), 7 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 5bb5ab82a0..67c26d9dea 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -779,9 +779,14 @@
# Nodes are mapped to their block device name if there is one, and
# to their node name otherwise. (Since 5.2)
#
+# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty limit
during
+# live migration. Should be in the range 1 to
1000ms,
+# defaults to 1000ms. (Since 8.1)
+#
# Features:
#
-# @unstable: Member @x-checkpoint-delay is experimental.
+# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
+#are experimental.
#
# Since: 2.4
##
@@ -799,8 +804,9 @@
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
- 'multifd-zlib-level' ,'multifd-zstd-level',
- 'block-bitmap-mapping' ] }
+ 'multifd-zlib-level', 'multifd-zstd-level',
+ 'block-bitmap-mapping',
+ { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] } ]
}
##
# @MigrateSetParameters:
@@ -935,9 +941,14 @@
# Nodes are mapped to their block device name if there is one, and
# to their node name otherwise. (Since 5.2)
#
+# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty limit
during
+# live migration. Should be in the range 1 to
1000ms,
+# defaults to 1000ms. (Since 8.1)
+#
# Features:
#
-# @unstable: Member @x-checkpoint-delay is experimental.
+# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
+#are experimental.
#
# TODO: either fuse back into MigrationParameters, or make
# MigrationParameters members mandatory
@@ -972,7 +983,9 @@
'*multifd-compression': 'MultiFDCompression',
'*multifd-zlib-level': 'uint8',
'*multifd-zstd-level': 'uint8',
-'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
+'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
+'*x-vcpu-dirty-limit-period': { 'type': 'uint64',
+'features': [ 'unstable' ] } } }
##
# @migrate-set-parameters:
@@ -1127,9 +1140,14 @@
# Nodes are mapped to their block device name if there is one, and
# to their node name otherwise. (Since 5.2)
#
+# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty limit
during
+# live migration. Should be in the range 1 to
1000ms,
+# defaults to 1000ms. (Since 8.1)
+#
# Features:
#
-# @unstable: Member @x-checkpoint-delay is experimental.
+# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
+#are experimental.
#
# Since: 2.4
##
@@ -1161,7 +1179,9 @@
'*multifd-compression': 'MultiFDCompression',
'*multifd-zlib-level': 'uint8',
'*multifd-zstd-level': 'uint8',
-'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
+'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
+'*x-vcpu-dirty-limit-period': { 'type': 'uint64',
+'features': [ 'unstable' ] } } }
##
# @query-migrate-parameters:
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 9885d7c9f7..352e9ec716 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -364,6 +364,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict
*qdict)
}
}
}
+
+monitor_printf(mon, "%s: %" PRIu64 " ms\n",
+MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD),
+params->x_vcpu_dirty_limit_period);
}
qapi_free_MigrationParameters(params);
@@ -620,6 +624,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict
[PULL 03/30] softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit"
From: Hyman Huang(黄勇)
dirty_rate paraemter of hmp command "set_vcpu_dirty_limit" is invalid
if less than 0, so add parameter check for it.
Note that this patch also delete the unsolicited help message and
clean up the code.
Signed-off-by: Hyman Huang(黄勇)
Reviewed-by: Markus Armbruster
Reviewed-by: Peter Xu
Reviewed-by: Juan Quintela
Message-Id: <168618975839.6361.1740763387474768865...@git.sr.ht>
Signed-off-by: Juan Quintela
---
softmmu/dirtylimit.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 015a9038d1..e80201097a 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -515,14 +515,15 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict
*qdict)
int64_t cpu_index = qdict_get_try_int(qdict, "cpu_index", -1);
Error *err = NULL;
+if (dirty_rate < 0) {
+error_setg(&err, "invalid dirty page limit %" PRId64, dirty_rate);
+goto out;
+}
+
qmp_set_vcpu_dirty_limit(!!(cpu_index != -1), cpu_index, dirty_rate, &err);
-if (err) {
-hmp_handle_error(mon, err);
-return;
-}
-monitor_printf(mon, "[Please use 'info vcpu_dirty_limit' to query "
- "dirty limit for virtual CPU]\n");
+out:
+hmp_handle_error(mon, err);
}
static struct DirtyLimitInfo *dirtylimit_query_vcpu(int cpu_index)
--
2.40.1
[PULL 05/30] qapi/migration: Introduce vcpu-dirty-limit parameters
From: Hyman Huang(黄勇)
Introduce "vcpu-dirty-limit" migration parameter used
to limit dirty page rate during live migration.
"vcpu-dirty-limit" and "x-vcpu-dirty-limit-period" are
two dirty-limit-related migration parameters, which can
be set before and during live migration by qmp
migrate-set-parameters.
This two parameters are used to help implement the dirty
page rate limit algo of migration.
Signed-off-by: Hyman Huang(黄勇)
Acked-by: Peter Xu
Reviewed-by: Juan Quintela
Message-Id: <168618975839.6361.1740763387474768865...@git.sr.ht>
Signed-off-by: Juan Quintela
---
qapi/migration.json| 18 +++---
migration/migration-hmp-cmds.c | 8
migration/options.c| 21 +
3 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 67c26d9dea..e7243c0c0d 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -783,6 +783,9 @@
# live migration. Should be in the range 1 to
1000ms,
# defaults to 1000ms. (Since 8.1)
#
+# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration.
+#Defaults to 1. (Since 8.1)
+#
# Features:
#
# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
@@ -806,7 +809,8 @@
'max-cpu-throttle', 'multifd-compression',
'multifd-zlib-level', 'multifd-zstd-level',
'block-bitmap-mapping',
- { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] } ]
}
+ { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] },
+ 'vcpu-dirty-limit'] }
##
# @MigrateSetParameters:
@@ -945,6 +949,9 @@
# live migration. Should be in the range 1 to
1000ms,
# defaults to 1000ms. (Since 8.1)
#
+# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration.
+#Defaults to 1. (Since 8.1)
+#
# Features:
#
# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
@@ -985,7 +992,8 @@
'*multifd-zstd-level': 'uint8',
'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
'*x-vcpu-dirty-limit-period': { 'type': 'uint64',
-'features': [ 'unstable' ] } } }
+'features': [ 'unstable' ] },
+'*vcpu-dirty-limit': 'uint64'} }
##
# @migrate-set-parameters:
@@ -1144,6 +1152,9 @@
# live migration. Should be in the range 1 to
1000ms,
# defaults to 1000ms. (Since 8.1)
#
+# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration.
+#Defaults to 1. (Since 8.1)
+#
# Features:
#
# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
@@ -1181,7 +1192,8 @@
'*multifd-zstd-level': 'uint8',
'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
'*x-vcpu-dirty-limit-period': { 'type': 'uint64',
-'features': [ 'unstable' ] } } }
+'features': [ 'unstable' ] },
+'*vcpu-dirty-limit': 'uint64'} }
##
# @query-migrate-parameters:
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 352e9ec716..35e8020bbf 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -368,6 +368,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict
*qdict)
monitor_printf(mon, "%s: %" PRIu64 " ms\n",
MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD),
params->x_vcpu_dirty_limit_period);
+
+monitor_printf(mon, "%s: %" PRIu64 " MB/s\n",
+MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT),
+params->vcpu_dirty_limit);
}
qapi_free_MigrationParameters(params);
@@ -628,6 +632,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict
*qdict)
p->has_x_vcpu_dirty_limit_period = true;
visit_type_size(v, param, &p->x_vcpu_dirty_limit_period, &err);
break;
+case MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT:
+p->has_vcpu_dirty_limit = true;
+visit_type_size(v, param, &p->vcpu_dirty_limit, &err);
+break;
default:
assert(0);
}
diff --git a/migration/options.c b/migration/options.c
index 9743dea3ab..8acf5f1d2c 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -81,6 +81,7 @@
DEFINE_PROP_BOOL(name, MigrationState, capabilities[x], false)
#define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD 1000/* milliseconds */
+#define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT1 /* MB/s */
Property migration_properties[] = {
DEFINE_PROP_BOOL("store-global-state", MigrationState,
@@ -168,6 +169,9 @@ Property migration_properties[] = {
DEF
[PULL 08/30] migration: Put the detection logic before auto-converge checking
From: Hyman Huang(黄勇)
This commit is prepared for the implementation of dirty-limit
convergence algo.
The detection logic of throttling condition can apply to both
auto-converge and dirty-limit algo, putting it's position
before the checking logic for auto-converge feature.
Signed-off-by: Hyman Huang(黄勇)
Reviewed-by: Juan Quintela
Message-ID: <168733225273.5845.1587182678887974167...@git.sr.ht>
Signed-off-by: Juan Quintela
---
migration/ram.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 78746849b5..b6559f9312 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -999,17 +999,18 @@ static void migration_trigger_throttle(RAMState *rs)
return;
}
-if (migrate_auto_converge()) {
-/* The following detection logic can be refined later. For now:
- Check to see if the ratio between dirtied bytes and the approx.
- amount of bytes that just got transferred since the last time
- we were in this routine reaches the threshold. If that happens
- twice, start or increase throttling. */
-
-if ((bytes_dirty_period > bytes_dirty_threshold) &&
-(++rs->dirty_rate_high_cnt >= 2)) {
+/*
+ * The following detection logic can be refined later. For now:
+ * Check to see if the ratio between dirtied bytes and the approx.
+ * amount of bytes that just got transferred since the last time
+ * we were in this routine reaches the threshold. If that happens
+ * twice, start or increase throttling.
+ */
+if ((bytes_dirty_period > bytes_dirty_threshold) &&
+(++rs->dirty_rate_high_cnt >= 2)) {
+rs->dirty_rate_high_cnt = 0;
+if (migrate_auto_converge()) {
trace_migration_throttle();
-rs->dirty_rate_high_cnt = 0;
mig_throttle_guest_down(bytes_dirty_period,
bytes_dirty_threshold);
}
--
2.40.1
[PULL 06/30] migration: Introduce dirty-limit capability
From: Hyman Huang(黄勇)
Introduce migration dirty-limit capability, which can
be turned on before live migration and limit dirty
page rate durty live migration.
Introduce migrate_dirty_limit function to help check
if dirty-limit capability enabled during live migration.
Meanwhile, refactor vcpu_dirty_rate_stat_collect
so that period can be configured instead of hardcoded.
dirty-limit capability is kind of like auto-converge
but using dirty limit instead of traditional cpu-throttle
to throttle guest down. To enable this feature, turn on
the dirty-limit capability before live migration using
migrate-set-capabilities, and set the parameters
"x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
to speed up convergence.
Signed-off-by: Hyman Huang(黄勇)
Acked-by: Peter Xu
Reviewed-by: Juan Quintela
Message-Id: <168618975839.6361.1740763387474768865...@git.sr.ht>
Signed-off-by: Juan Quintela
---
qapi/migration.json | 12 +++-
migration/options.h | 1 +
migration/options.c | 23 +++
softmmu/dirtylimit.c | 18 ++
4 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index e7243c0c0d..621e6604c6 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -487,6 +487,16 @@
# and should not affect the correctness of postcopy migration.
# (since 7.1)
#
+# @dirty-limit: If enabled, migration will use the dirty-limit algo to
+# throttle down guest instead of auto-converge algo.
+# Throttle algo only works when vCPU's dirtyrate greater
+# than 'vcpu-dirty-limit', read processes in guest os
+# aren't penalized any more, so this algo can improve
+# performance of vCPU during live migration. This is an
+# optional performance feature and should not affect the
+# correctness of the existing auto-converge algo.
+# (since 8.1)
+#
# Features:
#
# @unstable: Members @x-colo and @x-ignore-shared are experimental.
@@ -502,7 +512,7 @@
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
{ 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
'validate-uuid', 'background-snapshot',
- 'zero-copy-send', 'postcopy-preempt'] }
+ 'zero-copy-send', 'postcopy-preempt', 'dirty-limit'] }
##
# @MigrationCapabilityStatus:
diff --git a/migration/options.h b/migration/options.h
index 45991af3c2..51964eff29 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -29,6 +29,7 @@ bool migrate_block(void);
bool migrate_colo(void);
bool migrate_compress(void);
bool migrate_dirty_bitmaps(void);
+bool migrate_dirty_limit(void);
bool migrate_events(void);
bool migrate_ignore_shared(void);
bool migrate_late_block_activate(void);
diff --git a/migration/options.c b/migration/options.c
index 8acf5f1d2c..ba1010e08b 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -27,6 +27,7 @@
#include "qemu-file.h"
#include "ram.h"
#include "options.h"
+#include "sysemu/kvm.h"
/* Maximum migrate downtime set to 2000 seconds */
#define MAX_MIGRATE_DOWNTIME_SECONDS 2000
@@ -194,6 +195,7 @@ Property migration_properties[] = {
DEFINE_PROP_MIG_CAP("x-zero-copy-send",
MIGRATION_CAPABILITY_ZERO_COPY_SEND),
#endif
+DEFINE_PROP_MIG_CAP("x-dirty-limit", MIGRATION_CAPABILITY_DIRTY_LIMIT),
DEFINE_PROP_END_OF_LIST(),
};
@@ -240,6 +242,13 @@ bool migrate_dirty_bitmaps(void)
return s->capabilities[MIGRATION_CAPABILITY_DIRTY_BITMAPS];
}
+bool migrate_dirty_limit(void)
+{
+MigrationState *s = migrate_get_current();
+
+return s->capabilities[MIGRATION_CAPABILITY_DIRTY_LIMIT];
+}
+
bool migrate_events(void)
{
MigrationState *s = migrate_get_current();
@@ -556,6 +565,20 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
Error **errp)
}
}
+if (new_caps[MIGRATION_CAPABILITY_DIRTY_LIMIT]) {
+if (new_caps[MIGRATION_CAPABILITY_AUTO_CONVERGE]) {
+error_setg(errp, "dirty-limit conflicts with auto-converge"
+ " either of then available currently");
+return false;
+}
+
+if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
+error_setg(errp, "dirty-limit requires KVM with accelerator"
+ " property 'dirty-ring-size' set");
+return false;
+}
+}
+
return true;
}
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index e80201097a..942d876523 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -24,6 +24,9 @@
#include "hw/boards.h"
#include "sysemu/kvm.h"
#include "trace.h"
+#include "migration/misc.h"
+#include "migration/migration.h"
+#include "migration/options.h"
/*
* Dirtylimit stop working if dirty page rate error
@@ -75,14 +78,21 @@ static bool dirtylimit_quit;
static void vcpu_dirty_rate_stat_collect(void)
{
+MigrationState *
[PULL 00/30] Next patches
The following changes since commit b455ce4c2f300c8ba47cba7232dd03261368a4cb: Merge tag 'q800-for-8.1-pull-request' of https://github.com/vivier/qemu-m68k into staging (2023-06-22 10:18:32 +0200) are available in the Git repository at: https://gitlab.com/juan.quintela/qemu.git tags/next-pull-request for you to fetch changes up to 23e4307eadc1497bd0a11ca91041768f15963b68: migration/rdma: Split qemu_fopen_rdma() into input/output functions (2023-06-22 18:11:58 +0200) Migration Pull request (20230621) take 2 In this pull request the only change is fixing 32 bits complitaion issue. Please apply. [take 1] - fix for multifd thread creation (fabiano) - dirtylimity (hyman) * migration-test will go on next PULL request, as it has failures. - Improve error description (tejus) - improve -incoming and set parameters before calling incoming (wei) - migration atomic counters reviewed patches (quintela) - migration-test refacttoring reviewed (quintela) Fabiano Rosas (2): migration/multifd: Rename threadinfo.c functions migration/multifd: Protect accesses to migration_threads Hyman Huang(黄勇) (8): softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit" qapi/migration: Introduce x-vcpu-dirty-limit-period parameter qapi/migration: Introduce vcpu-dirty-limit parameters migration: Introduce dirty-limit capability migration: Refactor auto-converge capability logic migration: Put the detection logic before auto-converge checking migration: Implement dirty-limit convergence algo migration: Extend query-migrate to provide dirty page limit info Juan Quintela (16): migration-test: Be consistent for ppc migration-test: Make machine_opts regular with other options migration-test: Create arch_opts migration-test: machine_opts is really arch specific migration-test: Create kvm_opts migration-test: bootpath is the same for all tests and for all archs migration-test: Add bootfile_create/delete() functions migration-test: dirtylimit checks for x86_64 arch before migration-test: simplify shmem_opts handling qemu-file: Rename qemu_file_transferred_ fast -> noflush migration: Change qemu_file_transferred to noflush migration: Use qemu_file_transferred_noflush() for block migration. qemu_file: Make qemu_file_is_writable() static qemu-file: Simplify qemu_file_shutdown() qemu-file: Make qemu_file_get_error_obj() static migration/rdma: Split qemu_fopen_rdma() into input/output functions Tejus GK (2): migration: Update error description whenever migration fails migration: Refactor repeated call of yank_unregister_instance Wei Wang (2): migration: enforce multifd and postcopy preempt to be set before incoming qtest/migration-tests.c: use "-incoming defer" for postcopy tests qapi/migration.json| 74 +--- include/sysemu/dirtylimit.h| 2 + migration/options.h| 1 + migration/qemu-file.h | 14 ++-- migration/threadinfo.h | 7 +- migration/block.c | 4 +- migration/migration-hmp-cmds.c | 26 +++ migration/migration.c | 40 +++ migration/multifd.c| 4 +- migration/options.c| 87 +++ migration/qemu-file.c | 24 ++- migration/ram.c| 59 +--- migration/rdma.c | 39 +-- migration/savevm.c | 6 +- migration/threadinfo.c | 19 - migration/vmstate.c| 4 +- softmmu/dirtylimit.c | 97 +++--- tests/qtest/migration-test.c | 123 ++--- migration/trace-events | 1 + 19 files changed, 472 insertions(+), 159 deletions(-) base-commit: 5f9dd6a8ce3961db4ce47411ed2097ad88bdf5fc prerequisite-patch-id: 99c8bffa9428838925e330eb2881bab476122579 prerequisite-patch-id: 77ba427fd916aeb395e95aa0e7190f84e98e96ab prerequisite-patch-id: 9983d46fa438d7075a37be883529e37ae41e4228 prerequisite-patch-id: 207f7529924b12dcb57f6557d6db6f79ceb2d682 prerequisite-patch-id: 5ad1799a13845dbf893a28a202b51a6b50d95d90 prerequisite-patch-id: c51959aacd6d65ee84fcd4f1b2aed3dd6f6af879 prerequisite-patch-id: da9dbb6799b2da002c0896574334920097e4c50a prerequisite-patch-id: c1110ffafbaf5465fb277a20db809372291f7846 prerequisite-patch-id: 8307c92bedd07446214b35b40206eb6793a7384d prerequisite-patch-id: 0a6106cd4a508d5e700a7ff6c25edfdd03c8ca3d prerequisite-patch-id: 83205051de22382e75bf4acdf69e59315801fa0d prerequisite-patch-id: 8c9b3cba89d555c071a410041e6da41806106a7e prerequisite-patch-id: 0ff62a33b9a242226ccc1f5424a516de803c9fe5 prerequisite-patch-id: 25b8ae1ebe09ace14457c454cfcb23077c37346c prerequisite-patch-id: 466ea91d5be41fe345dacd4d17bbbe5ce13118c2 prerequisite-patch-id: d1045858f9729ac62eccf2e83ebf95cfebae2cb5 prerequisite-patch-id: 0276ec0207
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
> I can try to move the todo even higher. Trying to list the initial goals
> here:
>
> - One extra phase of handshake between src/dst (maybe the time to boost
> QEMU_VM_FILE_VERSION) before anything else happens.
>
> - Dest shouldn't need to apply any cap/param, it should get all from src.
> Dest still need to be setup with an URI and that should be all it needs.
>
> - Src shouldn't need to worry on the binary version of dst anymore as long
> as dest qemu supports handshake, because src can fetch it from dest.
I'm not sure that works in general. Even if we have a handshake and
bi-directional comms for live migration, we still haave the save/restore
to file codepath to deal with. The dst QEMU doesn't exist at the time
the save process is done, so we can't add logic to VMSate handling that
assumes knowledge of the dst version at time of serialization.
> - Handshake can always fail gracefully if anything wrong happened, it
> normally should mean dest qemu is not compatible with src's setup (either
> machine, device, or migration configs) for whatever reason. Src should
> be able to get a solid error from dest if so.
>
> - Handshake protocol should always be self-bootstrap-able, it means when we
> change the handshake protocol it should always works with old binaries.
>
> - When src is newer it should be able to know what's missing on dest and
> skip the new bits.
>
> - When dst is newer it should all rely on src (which is older) and it
> should always understand src's language.
I'm not convinced it can reliably self-bootstrap in a backwards
compatible manner, precisely because the current migration stream
has no handshake and only requires a unidirectional channel. I
don't think its possible for QEMU to validate that it has a fully
bi-directional channel, without adding timeouts to its detection
which I think we should strive to avoid.
I don't think we actually need self-bootstrapping anyway.
I think the mgmt app can just indicate the new v2 bi-directional
protocol when issuing the 'migrate' and 'migrate-incoming'
commands. This becomes trivial when Het's refactoring of the
migrate address QAPI is accepted:
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04851.html
eg:
{ "execute": "migrate",
"arguments": {
"channels": [ { "channeltype": "main",
"addr": { "transport": "socket", "type": "inet",
"host": "10.12.34.9",
"port": "1050" } } ] } }
note the 'channeltype' parameter here. If we declare the 'main'
refers to the existing migration protocol, then we merely need
to define a new 'channeltype' to use as an indicator for the
v2 migration handshake protocol.
> - All !main channels need to be established later than the handshake - if
> we're going to do this anyway we probably should do it altogether to make
> channels named, so each channel used in migration needs to have a common
> header. Prepare to deprecate the old tricks of channel orderings.
Once the primary channel involves a bi-directional handshake,
we'll trivially ensure ordering - similar to how the existing
code worked fnie in TLS mode which had a bi-directional TLS
handshake.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 5:26 PM Peter Xu wrote: > PS: we may want to postpone this to be later than migration_object_init(), > when/if there's a real patch. Yes, that's true. > > > The only incompatibility is for people who are using "," in an URI, > > > which is rare and only an issue for the "exec" protocol. > > If we worry on breaking anyone, we can apply the keyval parsing only when > !exec. Not sure whether it matters a huge lot.. No, I don't think it does. > > Aha, that makes sense. And will allow us to deprecate/remove the > > --global migration.* stuff. > > We may still need a way to set the caps/params for src qemu?.. The source can use migrate_set_parameters normally, since migration has to be started from the monitor anyway. Paolo
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 10:59:58AM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 22, 2023 at 10:52:12AM +0200, Juan Quintela wrote:
> > Paolo Bonzini wrote:
> > > On 6/12/23 22:51, Juan Quintela wrote:
> > >>> Shall we just leave it there? Or is deprecating it helps us in any
> > >>> form?
> > >> See the patches two weeks ago when people complained that lisen(.., num)
> > >> was too low. And there are other parameters that work the same way
> > >> (that I convenientely had forgotten). So the easiest way to get things
> > >> right is to use "defer" always. Using -incoming "uri" should only be
> > >> for people that "know what they are doing", so we had to ways to do it:
> > >> - review all migration options and see which ones work without defer
> > >>and document it
> > >> - deprecate everything that is not defer.
> > >
> > > "-incoming " is literally the same as running "migrate-incoming"
> > > as the first thing on the monitor:
> > >
> > > if (incoming) {
> > > Error *local_err = NULL;
> > > if (strcmp(incoming, "defer") != 0) {
> > > qmp_migrate_incoming(incoming, &local_err);
> > > if (local_err) {
> > > error_reportf_err(local_err, "-incoming %s: ", incoming);
> > > exit(1);
> > > }
> > > }
> > > } else if (autostart) {
> > > qmp_cont(NULL);
> > > }
> > >
> > > It's the only piece of code which distinguishes "-incoming defer" from
> > > "-incoming ".
> > >
> > > So I'm not sure what the problem would be with keeping it?
> >
> > User friendliness.
> >
> > First of all, I use it all the time. And I know that it is useful for
> > developers. I was the one asking peter to implement -global
> > migration.foo to be able to test multifd with it.
> >
> > The problem is that if you use more than two channels with multifd, on
> > the incoming side, you need to do:
> >
> > - migrate_set_parameter multifd-channels 16
> > - migrate_incoming
> >
> > And people continue to do:
> >
> > - qemu -incoming
> > - migrate_set_parameter multifd-channels 16 (on the command line)
> >
> > And they complain that it fails, because we are calling listen with the
> > wrong value.
>
> IMHO if we want to improve user friendliness then arguing about use
> of the CLI vs QMP for migration is completely missing the bigger
> picture IMHO.
>
> I've mentioned several times before that the user should never need to
> set this multifd-channels parameter (nor many other parameters) on the
> destination in the first place.
>
> The QEMU migration stream should be changed to add a full
> bi-directional handshake, with negotiation of most parameters.
> IOW, the src QEMU should be configured with 16 channels, and
> it should connect the primary control channel, and then directly
> tell the dest that it wants to use 16 multifd channels.
>
> If we're expecting the user to pass this info across to the dest
> manually we've already spectacularly failed wrt user friendliness.
I can try to move the todo even higher. Trying to list the initial goals
here:
- One extra phase of handshake between src/dst (maybe the time to boost
QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src.
Dest still need to be setup with an URI and that should be all it needs.
- Src shouldn't need to worry on the binary version of dst anymore as long
as dest qemu supports handshake, because src can fetch it from dest.
- Handshake can always fail gracefully if anything wrong happened, it
normally should mean dest qemu is not compatible with src's setup (either
machine, device, or migration configs) for whatever reason. Src should
be able to get a solid error from dest if so.
- Handshake protocol should always be self-bootstrap-able, it means when we
change the handshake protocol it should always works with old binaries.
- When src is newer it should be able to know what's missing on dest and
skip the new bits.
- When dst is newer it should all rely on src (which is older) and it
should always understand src's language.
- All !main channels need to be established later than the handshake - if
we're going to do this anyway we probably should do it altogether to make
channels named, so each channel used in migration needs to have a common
header. Prepare to deprecate the old tricks of channel orderings.
Does it look reasonable? Anything to add/remove?
Thanks,
--
Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 11:22:56AM +0200, Thomas Huth wrote: > Then simply forbid "migrate_set_parameter multifd-channels ..." if the uri > has been specified on the command line? Yeah, actually already in a pull (even though the pr may need a new one..): https://lore.kernel.org/r/20230622021320.66124-23-quint...@redhat.com -- Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 12:01:55PM +0200, Juan Quintela wrote:
> Paolo Bonzini wrote:
> > On 6/22/23 10:52, Juan Quintela wrote:
> >> User friendliness.
> >> The problem is that if you use more than two channels with multifd, on
> >> the incoming side, you need to do:
> >
> > You're sacrificing user-friendliness for the 99.99% that don't use
> > multifd, for an error (i.e. it's not even fixing the issue) for the
> > 0.01% that use multifd. That's not user-friendly.
>
> You are forgeting of the 0.01% that uses postocopy preempt (that is easy
> just changing the default value to 2), and the 0.001% that uses
> compression (they have exactly the same problem with compress_threads,
> compress_zlib, etc).
>
> >> - migrate_set_parameter multifd-channels 16
> >> - migrate_incoming
> >>
> >>> The issue is not how many features the command line has, but how
> >>> they're implemented.
> >> Or if they are confusing for the user?
> >
> > Anyone using multifd is not a typical user anyway.
>
> >>> If they're just QMP wrappers and as such they're self-contained in
> >>> softmmu/vl.c, that's fine.
> >>>
> >>> In fact, even for parameters, we could use keyval to parse "-incoming"
> >> What is keyval?
> >
> > util/keyval.c and include/qemu/keyval.h. It parses a list of
> > key=value pairs into a QDict. Once you have removed the "source" key
> > from the QDict you can use a visitor to parse the rest into a
> > MigrateSetParameters. See the handling of QEMU_OPTION_audio, it could
> > be something like
> >
> >
> > case QEMU_OPTION_incoing: {
> > Visitor *v;
> > MigrateSetParameters *incoming_params = NULL;
> > QDict *dict = keyval_parse(optarg, "source", NULL,
> > &error_fatal);
> >
> > if (incoming) {
> > if (qdict_haskey(dict, "source")) {
> > error_setg(&error_fatal, "Parameter 'source'
> > is duplicate");
> > }
> > } else {
> > if (!qdict_haskey(dict, "source")) {
> > error_setg(&error_fatal, "Parameter 'source'
> > is missing");
> > }
> > runstate_set(RUN_STATE_INMIGRATE);
> > incoming = g_strdup(qdict_get_str(dict, "source"));
> > qdict_del(dict, "source");
> > }
> >
> > v = qobject_input_visitor_new_keyval(QOBJECT(dict));
> > qobject_unref(dict);
> > visit_type_MigrateSetParameters(v, NULL,
> > &incoming_params, &error_fatal);
> > visit_free(v);
> > qmp_migration_set_parameters(incoming_params,
PS: we may want to postpone this to be later than migration_object_init(),
when/if there's a real patch.
> > &error_fatal);
> > qapi_free_MigrateSetParameters(incoming_params);
> > }
> >
> >
> > For example "-incoming [source=]tcp:foo,multifd-channels=16" would
> > desugar to
> >
> > migrate_set_parameter multifd-channels 16
> > migrate_incoming tcp:foo
> >
> > The only incompatibility is for people who are using "," in an URI,
> > which is rare and only an issue for the "exec" protocol.
If we worry on breaking anyone, we can apply the keyval parsing only when
!exec. Not sure whether it matters a huge lot..
>
> Aha, that makes sense. And will allow us to deprecate/remove the
> --global migration.* stuff.
We may still need a way to set the caps/params for src qemu?..
Thanks,
--
Peter Xu
Re: [PATCH v2 06/20] qemu_file: total_transferred is not used anymore
On Thu, Jun 22, 2023 at 01:05:49AM +0200, Juan Quintela wrote:
> Peter Xu wrote:
> > On Tue, May 30, 2023 at 08:39:27PM +0200, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela
> >> ---
> >> migration/qemu-file.c | 4
> >> 1 file changed, 4 deletions(-)
> >>
> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> >> index eb0497e532..6b6deea19b 100644
> >> --- a/migration/qemu-file.c
> >> +++ b/migration/qemu-file.c
> >> @@ -41,9 +41,6 @@ struct QEMUFile {
> >> QIOChannel *ioc;
> >> bool is_writable;
> >>
> >> -/* The sum of bytes transferred on the wire */
> >> -uint64_t total_transferred;
> >> -
> >> int buf_index;
> >> int buf_size; /* 0 when writing */
> >> uint8_t buf[IO_BUF_SIZE];
> >> @@ -287,7 +284,6 @@ void qemu_fflush(QEMUFile *f)
> >> qemu_file_set_error_obj(f, -EIO, local_error);
> >> } else {
> >> uint64_t size = iov_size(f->iov, f->iovcnt);
> >> -f->total_transferred += size;
> >
> > I think this patch is another example why I think sometimes the way patch
> > is split are pretty much adding more complexity on review...
>
> It depends of taste.
>
> You are doing one thing in way1.
> Then you find a better way to do it, lets call it way2.
>
> Now we have two options to see how we arrived there.
>
> a- You got any declarations/definition/initializations for way2
> b- You write way2 alongside way1
> c- You test that both ways give the same result, and you see that they
>give the same result.
> d- you remove the way1.
>
> Or you squash the four patches in a single patch. But then the reviewer
> lost the place where one can see why it is the same than the old one.
For this patch to remove total_transferred, IMHO as a reviewer it'll be
easier to me if it's put in the same patch where it got replaced.
It might be different if we're going to remove a large chunk of code, but
for this patch it's a few lines of change.
>
> Sometimes is better the longer way, sometimes is better the short one.
>
> Clearly we don't agree about what is the best way in this case.
>
> > Here we removed a variable operation but it seems all fine if it's not used
> > anywhere. But it also means current code base (before this patch applied)
> > doesn't make sense already because it contains this useless addition. So
> > IMHO it means some previous patch does it just wrong.
>
> No. It is how it is developed. And being respectful with the
> reviewer. Given it enough information to do a proper review.
I never doubted about that. I trust that you were trying to provide the
best for a reviewer and I appreciate that.
>
> During the development of this series, there were lots of:
>
> if (old_counter != new_counter)
>printf("");
I assume you meant when debugging? I may not have read all the patches; I
hope we just don't do that if possible in any real git commit.
>
> traces were in the several thousand lines long. If I have to review
> that change, I would love any help that writer can give me. That is why
> it is done this way.
Yeah, I think it's probably that even from reviewers' perspective the need
can be different from individuals.
>
> > I think it means it's caused by a wrong split of patches, then each patch
> > stops to make much sense as a standalone one.
>
> It stops making sense if you want each feature to be a single patch.
> Before the patch no feature. After the patch full feature. That brings
> us to very long patches.
>
> What is easier to review (to do the same)
>
> a - 1 x 1000 lines patch
> b - 10 x 100 lines patch
>
> I will go with b any time. Except if the split is arbitrary.
AFAIU, this is a different thing. I'm never against splitting patch, but
about how to split. I was never asking for a 1000 LOC patch, right? :)
>
> > I can go back and try to find whatever patch on the list that will explain
> > this. But it'll also go into git log. Anyone reads this later will be
> > confused once again. Even harder for them to figure out what
> > happened.
>
> As said before, I completely disagree here. And what is worse. If it
> gets wrong, with your approach git bisect will not help as much than
> with my appreach.
>
> > Do you think we could reorganize the patches so each of a single patch
> > explains itself?
>
> No. See before. We go for a very spaguetti code to a much less
> spaguety code.
>
> > The other thing is about priority of patches - I still have ~80 patches
> > pending reviews on migration only.. Would you think it makes sense we pickg
> > up important ones first and merge them with higher priority?
>
> Ok, lets make this clear.
> This whole atomic migration counters started because the zero_page
> detection in multifd had the counters so wrong that meassuring speed
> become impossible.
>
> I haven't yet send the multifd zero pages. And why was it so
> complicated. Just on top of my memory.
>
> - how much data had we transferred. Hi
Re: [PULL 20/30] migration: Update error description whenever migration fails
On 22/06/23 7:43 am, Juan Quintela wrote:
> From: Tejus GK
>
> There are places in migration.c where the migration is marked failed with
> MIGRATION_STATUS_FAILED, but the failure reason is never updated. Hence
> libvirt doesn't know why the migration failed when it queries for it.
>
> Reviewed-by: Daniel P. Berrangé Reviewed-by: Juan Quintela
> Acked-by: Peter Xu
> Signed-off-by: Tejus GK
> Message-ID: <20230621130940.178659-2-tejus...@nutanix.com>
> Signed-off-by: Juan Quintela
> ---
> migration/migration.c | 19 ---
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 719f91573f..e6bff2e848 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1679,7 +1679,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool
> blk,
> if (!(has_resume && resume)) {
> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> }
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
> +error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
> "a valid migration protocol");
> migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>MIGRATION_STATUS_FAILED);
> @@ -2066,7 +2066,7 @@ migration_wait_main_channel(MigrationState *ms)
> * Switch from normal iteration to postcopy
> * Returns non-0 on error
> */
> -static int postcopy_start(MigrationState *ms)
> +static int postcopy_start(MigrationState *ms, Error **errp)
> {
> int ret;
> QIOChannelBuffer *bioc;
> @@ -2176,7 +2176,7 @@ static int postcopy_start(MigrationState *ms)
> */
> ret = qemu_file_get_error(ms->to_dst_file);
> if (ret) {
> -error_report("postcopy_start: Migration stream errored (pre
> package)");
> +error_setg(errp, "postcopy_start: Migration stream errored (pre
> package)");
> goto fail_closefb;
> }
>
> @@ -2213,7 +2213,7 @@ static int postcopy_start(MigrationState *ms)
>
> ret = qemu_file_get_error(ms->to_dst_file);
> if (ret) {
> -error_report("postcopy_start: Migration stream errored");
> +error_setg(errp, "postcopy_start: Migration stream errored");
> migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
>MIGRATION_STATUS_FAILED);
> }
> @@ -2720,6 +2720,7 @@ typedef enum {
> static MigIterateState migration_iteration_run(MigrationState *s)
> {
> uint64_t must_precopy, can_postcopy;
> +Error *local_err = NULL;
> bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
>
> qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy);
> @@ -2742,8 +2743,9 @@ static MigIterateState
> migration_iteration_run(MigrationState *s)
> /* Still a significant amount to transfer */
> if (!in_postcopy && must_precopy <= s->threshold_size &&
> qatomic_read(&s->start_postcopy)) {
> -if (postcopy_start(s)) {
> -error_report("%s: postcopy failed to start", __func__);
> +if (postcopy_start(s, &local_err)) {
> +migrate_set_error(s, local_err);
> +error_report_err(local_err);
> }
> return MIG_ITERATE_SKIP;
> }
> @@ -3234,8 +3236,10 @@ void migrate_fd_connect(MigrationState *s, Error
> *error_in)
> */
> if (migrate_postcopy_ram() || migrate_return_path()) {
> if (open_return_path_on_source(s, !resume)) {
> -error_report("Unable to open return-path for postcopy");
> +error_setg(&local_err, "Unable to open return-path for
> postcopy");
> migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> +migrate_set_error(s, local_err);
> +error_report_err(local_err);
> migrate_fd_cleanup(s);
> return;
> }
> @@ -3259,6 +3263,7 @@ void migrate_fd_connect(MigrationState *s, Error
> *error_in)
> }
>
> if (multifd_save_setup(&local_err) != 0) {
> +migrate_set_error(s, local_err);
> error_report_err(local_err);
> migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>MIGRATION_STATUS_FAILED);
Hi Juan,
I can see that the "Acked-by" by Peter has been added to this patch ( and the
next one in queue ), however, Peter hadn't Acked this patchset, but had "Acked"
a different but similar patchset which is still under review:
https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg04184.html
Regards,
Tejus
[PATCH 2/2] ide: Explicitly poll for BHs on cancel
When we still have an AIOCB registered for DMA operations, we try to
settle the respective operation by draining the BlockBackend associated
with the IDE device.
However, this assumes that every DMA operation is associated with some
I/O operation on the BlockBackend, and so settling the latter will
settle the former. That is not the case; for example, the guest is free
to issue a zero-length TRIM operation that will not result in any I/O
operation forwarded to the BlockBackend. In such a case, blk_drain()
will be a no-op if no other operations are in flight.
It is clear that if blk_drain() is a no-op, the value of
s->bus->dma->aiocb will not change between checking it in the `if`
condition and asserting that it is NULL after blk_drain().
To settle the DMA operation, we will thus need to explicitly invoke
aio_poll() ourselves, which will run any outstanding BHs (like
ide_trim_bh_cb()), until s->bus->dma->aiocb is NULL. To stop this from
being an infinite loop, assert that we made progress with every
aio_poll() call (i.e., invoked some BH).
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2029980
Signed-off-by: Hanna Reitz
Signed-off-by: Lukas Straub
---
hw/ide/core.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d172e70f1e..a5fd89ebdd 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -736,7 +736,17 @@ void ide_cancel_dma_sync(IDEState *s)
if (s->bus->dma->aiocb) {
trace_ide_cancel_dma_sync_remaining();
blk_drain(s->blk);
-assert(s->bus->dma->aiocb == NULL);
+
+/*
+ * Wait for potentially still-scheduled BHs, like ide_trim_bh_cb()
+ * (blk_drain() will only poll if there are in-flight requests on the
+ * BlockBackend, which there may not necessarily be, e.g. when the
+ * guest has issued a zero-length TRIM request)
+ */
+while (s->bus->dma->aiocb) {
+bool progress = aio_poll(qemu_get_aio_context(), true);
+assert(progress);
+}
}
}
--
2.39.2
pgpj6vMoQ4HKf.pgp
Description: OpenPGP digital signature
[PATCH 1/2] ide: Fix a rare hang during block draining
If the guest issues a discard during a block drain section, the
blk_aio_pdiscard() may not be processed, but queued instead.
And so the callback will never be called to issue the bh and decrease
the BB in-flight number again.
This causes a hang in the drain code, since it will wait forever for the
BB in-flight counter to decrease.
This reverts commit 7e5cdb34 "ide: Increment BB in-flight counter for TRIM BH"
to fix this hang. The bug fixed by that commit will be fixed differently
in the next commit.
Signed-off-by: Lukas Straub
---
hw/ide/core.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index de48ff9f86..d172e70f1e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -436,16 +436,12 @@ static const AIOCBInfo trim_aiocb_info = {
static void ide_trim_bh_cb(void *opaque)
{
TrimAIOCB *iocb = opaque;
-BlockBackend *blk = iocb->s->blk;
iocb->common.cb(iocb->common.opaque, iocb->ret);
qemu_bh_delete(iocb->bh);
iocb->bh = NULL;
qemu_aio_unref(iocb);
-
-/* Paired with an increment in ide_issue_trim() */
-blk_dec_in_flight(blk);
}
static void ide_issue_trim_cb(void *opaque, int ret)
@@ -516,9 +512,6 @@ BlockAIOCB *ide_issue_trim(
IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
TrimAIOCB *iocb;
-/* Paired with a decrement in ide_trim_bh_cb() */
-blk_inc_in_flight(s->blk);
-
iocb = blk_aio_get(&trim_aiocb_info, s->blk, cb, cb_opaque);
iocb->s = s;
iocb->bh = qemu_bh_new_guarded(ide_trim_bh_cb, iocb,
--
2.39.2
pgpsiHGM3Qy0x.pgp
Description: OpenPGP digital signature
Re: [RFC 4/6] migration: Deprecate -incoming
Paolo Bonzini wrote:
> On 6/22/23 10:52, Juan Quintela wrote:
>> User friendliness.
>> The problem is that if you use more than two channels with multifd, on
>> the incoming side, you need to do:
>
> You're sacrificing user-friendliness for the 99.99% that don't use
> multifd, for an error (i.e. it's not even fixing the issue) for the
> 0.01% that use multifd. That's not user-friendly.
You are forgeting of the 0.01% that uses postocopy preempt (that is easy
just changing the default value to 2), and the 0.001% that uses
compression (they have exactly the same problem with compress_threads,
compress_zlib, etc).
>> - migrate_set_parameter multifd-channels 16
>> - migrate_incoming
>>
>>> The issue is not how many features the command line has, but how
>>> they're implemented.
>> Or if they are confusing for the user?
>
> Anyone using multifd is not a typical user anyway.
>>> If they're just QMP wrappers and as such they're self-contained in
>>> softmmu/vl.c, that's fine.
>>>
>>> In fact, even for parameters, we could use keyval to parse "-incoming"
>> What is keyval?
>
> util/keyval.c and include/qemu/keyval.h. It parses a list of
> key=value pairs into a QDict. Once you have removed the "source" key
> from the QDict you can use a visitor to parse the rest into a
> MigrateSetParameters. See the handling of QEMU_OPTION_audio, it could
> be something like
>
>
> case QEMU_OPTION_incoing: {
> Visitor *v;
> MigrateSetParameters *incoming_params = NULL;
> QDict *dict = keyval_parse(optarg, "source", NULL,
> &error_fatal);
>
> if (incoming) {
> if (qdict_haskey(dict, "source")) {
> error_setg(&error_fatal, "Parameter 'source'
> is duplicate");
> }
> } else {
> if (!qdict_haskey(dict, "source")) {
> error_setg(&error_fatal, "Parameter 'source'
> is missing");
> }
> runstate_set(RUN_STATE_INMIGRATE);
> incoming = g_strdup(qdict_get_str(dict, "source"));
> qdict_del(dict, "source");
> }
>
> v = qobject_input_visitor_new_keyval(QOBJECT(dict));
> qobject_unref(dict);
> visit_type_MigrateSetParameters(v, NULL,
> &incoming_params, &error_fatal);
> visit_free(v);
> qmp_migration_set_parameters(incoming_params,
> &error_fatal);
> qapi_free_MigrateSetParameters(incoming_params);
> }
>
>
> For example "-incoming [source=]tcp:foo,multifd-channels=16" would
> desugar to
>
> migrate_set_parameter multifd-channels 16
> migrate_incoming tcp:foo
>
> The only incompatibility is for people who are using "," in an URI,
> which is rare and only an issue for the "exec" protocol.
Aha, that makes sense. And will allow us to deprecate/remove the
--global migration.* stuff.
Thanks very much.
See why this was an RFC O:-)
Later, Juan.
>
> Paolo
>
>>> and
>>> set the parameters in the same place as above. That would remove the need
>>> for "-global migration".
>> Could you elaborate?
>
>
>
>> The other option that I can think of is changing the error messages for
>> migrate_check_parameters() and give instructions that you can't set
>> multifd channels once that you have started incoming migration.
>> Explaining there to use migrate_incoming command?
>> Later, Juan.
>>
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 10:52:12AM +0200, Juan Quintela wrote:
> Paolo Bonzini wrote:
> > On 6/12/23 22:51, Juan Quintela wrote:
> >>> Shall we just leave it there? Or is deprecating it helps us in any form?
> >> See the patches two weeks ago when people complained that lisen(.., num)
> >> was too low. And there are other parameters that work the same way
> >> (that I convenientely had forgotten). So the easiest way to get things
> >> right is to use "defer" always. Using -incoming "uri" should only be
> >> for people that "know what they are doing", so we had to ways to do it:
> >> - review all migration options and see which ones work without defer
> >>and document it
> >> - deprecate everything that is not defer.
> >
> > "-incoming " is literally the same as running "migrate-incoming"
> > as the first thing on the monitor:
> >
> > if (incoming) {
> > Error *local_err = NULL;
> > if (strcmp(incoming, "defer") != 0) {
> > qmp_migrate_incoming(incoming, &local_err);
> > if (local_err) {
> > error_reportf_err(local_err, "-incoming %s: ", incoming);
> > exit(1);
> > }
> > }
> > } else if (autostart) {
> > qmp_cont(NULL);
> > }
> >
> > It's the only piece of code which distinguishes "-incoming defer" from
> > "-incoming ".
> >
> > So I'm not sure what the problem would be with keeping it?
>
> User friendliness.
>
> First of all, I use it all the time. And I know that it is useful for
> developers. I was the one asking peter to implement -global
> migration.foo to be able to test multifd with it.
>
> The problem is that if you use more than two channels with multifd, on
> the incoming side, you need to do:
>
> - migrate_set_parameter multifd-channels 16
> - migrate_incoming
>
> And people continue to do:
>
> - qemu -incoming
> - migrate_set_parameter multifd-channels 16 (on the command line)
>
> And they complain that it fails, because we are calling listen with the
> wrong value.
IMHO if we want to improve user friendliness then arguing about use
of the CLI vs QMP for migration is completely missing the bigger
picture IMHO.
I've mentioned several times before that the user should never need to
set this multifd-channels parameter (nor many other parameters) on the
destination in the first place.
The QEMU migration stream should be changed to add a full
bi-directional handshake, with negotiation of most parameters.
IOW, the src QEMU should be configured with 16 channels, and
it should connect the primary control channel, and then directly
tell the dest that it wants to use 16 multifd channels.
If we're expecting the user to pass this info across to the dest
manually we've already spectacularly failed wrt user friendliness.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC 4/6] migration: Deprecate -incoming
On 6/22/23 10:52, Juan Quintela wrote:
User friendliness.
The problem is that if you use more than two channels with multifd, on
the incoming side, you need to do:
You're sacrificing user-friendliness for the 99.99% that don't use
multifd, for an error (i.e. it's not even fixing the issue) for the
0.01% that use multifd. That's not user-friendly.
- migrate_set_parameter multifd-channels 16
- migrate_incoming
The issue is not how many features the command line has, but how
they're implemented.
Or if they are confusing for the user?
Anyone using multifd is not a typical user anyway.
If they're just QMP wrappers and as such they're self-contained in
softmmu/vl.c, that's fine.
In fact, even for parameters, we could use keyval to parse "-incoming"
What is keyval?
util/keyval.c and include/qemu/keyval.h. It parses a list of key=value
pairs into a QDict. Once you have removed the "source" key from the
QDict you can use a visitor to parse the rest into a
MigrateSetParameters. See the handling of QEMU_OPTION_audio, it could
be something like
case QEMU_OPTION_incoing: {
Visitor *v;
MigrateSetParameters *incoming_params = NULL;
QDict *dict = keyval_parse(optarg, "source", NULL,
&error_fatal);
if (incoming) {
if (qdict_haskey(dict, "source")) {
error_setg(&error_fatal, "Parameter 'source' is
duplicate");
}
} else {
if (!qdict_haskey(dict, "source")) {
error_setg(&error_fatal, "Parameter 'source' is
missing");
}
runstate_set(RUN_STATE_INMIGRATE);
incoming = g_strdup(qdict_get_str(dict, "source"));
qdict_del(dict, "source");
}
v = qobject_input_visitor_new_keyval(QOBJECT(dict));
qobject_unref(dict);
visit_type_MigrateSetParameters(v, NULL,
&incoming_params, &error_fatal);
visit_free(v);
qmp_migration_set_parameters(incoming_params,
&error_fatal);
qapi_free_MigrateSetParameters(incoming_params);
}
For example "-incoming [source=]tcp:foo,multifd-channels=16" would
desugar to
migrate_set_parameter multifd-channels 16
migrate_incoming tcp:foo
The only incompatibility is for people who are using "," in an URI,
which is rare and only an issue for the "exec" protocol.
Paolo
and
set the parameters in the same place as above. That would remove the need
for "-global migration".
Could you elaborate?
The other option that I can think of is changing the error messages for
migrate_check_parameters() and give instructions that you can't set
multifd channels once that you have started incoming migration.
Explaining there to use migrate_incoming command?
Later, Juan.
Re: [PATCH RFC 0/6] Switch iotests to pyvenv
On Wed, Jun 21, 2023 at 9:08 AM Paolo Bonzini wrote: > Maybe patch 4 can use distlib.scripts as well to create the check script in > the build directory? (Yes that's another mkvenv functionality...) On a phone > and don't have the docs at hand, so I am not sure. If not, your solution is > good enough. > > Apart from this the only issue is the speed. IIRC having a prebuilt .whl > would fix it, I think for Meson we observed that the slow part was building > the wheel. Possibilities: > > 1) using --no-pep517 if that also speeds it up? > > 2) already removing the sources to qemu.qmp since that's the plan anyway; and > then, if you want editability you can install the package with --user > --editable, i.e. outside the venv Nope, it's 3 second always and 1.5 even with the wheel. Maybe replace qemu.qmp with a wheel and leaving PYTHONPATH for the rest? Paolo > Paolo > >> >> John Snow (6): >> experiment: add mkvenv install >> build, tests: Add qemu in-tree packages to pyvenv at configure time. >> iotests: get rid of '..' in path environment output >> iotests: use the correct python to run linters >> iotests: use pyvenv/bin/python3 to launch child test processes >> iotests: don't add qemu.git/python to PYTHONPATH >> >> configure | 31 +++ >> python/scripts/mkvenv.py | 40 +++ >> tests/qemu-iotests/linters.py | 2 +- >> tests/qemu-iotests/testenv.py | 21 -- >> 4 files changed, 87 insertions(+), 7 deletions(-) >> >> -- >> 2.40.1 >> >>
Re: [RFC 4/6] migration: Deprecate -incoming
On 22/06/2023 10.52, Juan Quintela wrote:
Paolo Bonzini wrote:
On 6/12/23 22:51, Juan Quintela wrote:
Shall we just leave it there? Or is deprecating it helps us in any form?
See the patches two weeks ago when people complained that lisen(.., num)
was too low. And there are other parameters that work the same way
(that I convenientely had forgotten). So the easiest way to get things
right is to use "defer" always. Using -incoming "uri" should only be
for people that "know what they are doing", so we had to ways to do it:
- review all migration options and see which ones work without defer
and document it
- deprecate everything that is not defer.
"-incoming " is literally the same as running "migrate-incoming"
as the first thing on the monitor:
if (incoming) {
Error *local_err = NULL;
if (strcmp(incoming, "defer") != 0) {
qmp_migrate_incoming(incoming, &local_err);
if (local_err) {
error_reportf_err(local_err, "-incoming %s: ", incoming);
exit(1);
}
}
} else if (autostart) {
qmp_cont(NULL);
}
It's the only piece of code which distinguishes "-incoming defer" from
"-incoming ".
So I'm not sure what the problem would be with keeping it?
User friendliness.
First of all, I use it all the time. And I know that it is useful for
developers. I was the one asking peter to implement -global
migration.foo to be able to test multifd with it.
The problem is that if you use more than two channels with multifd, on
the incoming side, you need to do:
- migrate_set_parameter multifd-channels 16
- migrate_incoming
And people continue to do:
- qemu -incoming
- migrate_set_parameter multifd-channels 16 (on the command line)
And they complain that it fails, because we are calling listen with the
wrong value.
Then simply forbid "migrate_set_parameter multifd-channels ..." if the uri
has been specified on the command line?
Thomas
Re: [RFC 4/6] migration: Deprecate -incoming
Paolo Bonzini wrote:
> On 6/12/23 22:51, Juan Quintela wrote:
>>> Shall we just leave it there? Or is deprecating it helps us in any form?
>> See the patches two weeks ago when people complained that lisen(.., num)
>> was too low. And there are other parameters that work the same way
>> (that I convenientely had forgotten). So the easiest way to get things
>> right is to use "defer" always. Using -incoming "uri" should only be
>> for people that "know what they are doing", so we had to ways to do it:
>> - review all migration options and see which ones work without defer
>>and document it
>> - deprecate everything that is not defer.
>
> "-incoming " is literally the same as running "migrate-incoming"
> as the first thing on the monitor:
>
> if (incoming) {
> Error *local_err = NULL;
> if (strcmp(incoming, "defer") != 0) {
> qmp_migrate_incoming(incoming, &local_err);
> if (local_err) {
> error_reportf_err(local_err, "-incoming %s: ", incoming);
> exit(1);
> }
> }
> } else if (autostart) {
> qmp_cont(NULL);
> }
>
> It's the only piece of code which distinguishes "-incoming defer" from
> "-incoming ".
>
> So I'm not sure what the problem would be with keeping it?
User friendliness.
First of all, I use it all the time. And I know that it is useful for
developers. I was the one asking peter to implement -global
migration.foo to be able to test multifd with it.
The problem is that if you use more than two channels with multifd, on
the incoming side, you need to do:
- migrate_set_parameter multifd-channels 16
- migrate_incoming
And people continue to do:
- qemu -incoming
- migrate_set_parameter multifd-channels 16 (on the command line)
And they complain that it fails, because we are calling listen with the
wrong value.
> The issue is
> not how many features the command line has, but how they're implemented.
Or if they are confusing for the user?
> If they're just QMP wrappers and as such they're self-contained in
> softmmu/vl.c, that's fine.
>
> In fact, even for parameters, we could use keyval to parse "-incoming"
What is keyval?
> and
> set the parameters in the same place as above. That would remove the need
> for "-global migration".
Could you elaborate?
The other option that I can think of is changing the error messages for
migrate_check_parameters() and give instructions that you can't set
multifd channels once that you have started incoming migration.
Explaining there to use migrate_incoming command?
Later, Juan.
Re: [RFC 4/6] migration: Deprecate -incoming
On 6/21/23 09:08, Thomas Huth wrote:
if (strcmp(incoming, "defer") != 0) {
+ warn_report("-incoming %s is deprecated, use -incoming
defer and "
+ " set the uri with migrate-incoming.",
incoming);
qmp_migrate_incoming(incoming, &local_err);
if (local_err) {
error_reportf_err(local_err, "-incoming %s: ",
incoming);
Could we maybe keep at least the smallest set of necessary parameters
around? I'm often doing a "-incoming tcp:0:1234" for doing quick sanity
checks with migration, not caring about other migration parameters, so
if that could continue to work, that would be very appreciated.
Yeah, this is throwing the baby out with the bathwater.
Paolo
Re: [RFC 4/6] migration: Deprecate -incoming
On 6/12/23 22:51, Juan Quintela wrote:
Shall we just leave it there? Or is deprecating it helps us in any form?
See the patches two weeks ago when people complained that lisen(.., num)
was too low. And there are other parameters that work the same way
(that I convenientely had forgotten). So the easiest way to get things
right is to use "defer" always. Using -incoming "uri" should only be
for people that "know what they are doing", so we had to ways to do it:
- review all migration options and see which ones work without defer
and document it
- deprecate everything that is not defer.
"-incoming " is literally the same as running "migrate-incoming"
as the first thing on the monitor:
if (incoming) {
Error *local_err = NULL;
if (strcmp(incoming, "defer") != 0) {
qmp_migrate_incoming(incoming, &local_err);
if (local_err) {
error_reportf_err(local_err, "-incoming %s: ", incoming);
exit(1);
}
}
} else if (autostart) {
qmp_cont(NULL);
}
It's the only piece of code which distinguishes "-incoming defer" from
"-incoming ".
So I'm not sure what the problem would be with keeping it? The issue is
not how many features the command line has, but how they're implemented.
If they're just QMP wrappers and as such they're self-contained in
softmmu/vl.c, that's fine.
In fact, even for parameters, we could use keyval to parse "-incoming" and
set the parameters in the same place as above. That would remove the need
for "-global migration".
Paolo
Re: [PULL 00/30] Next patches
Richard Henderson wrote: > On 6/22/23 04:12, Juan Quintela wrote: >> The following changes since commit 67fe6ae41da64368bc4936b196fee2bf61f8c720: >>Merge tag 'pull-tricore-20230621-1' >> ofhttps://github.com/bkoppelmann/qemu into staging (2023-06-21 >> 20:08:48 +0200) >> are available in the Git repository at: >>https://gitlab.com/juan.quintela/qemu.git tags/next-pull-request >> for you to fetch changes up to >> c53dc569d0a0fb76eaa83f353253a897914948f9: >>migration/rdma: Split qemu_fopen_rdma() into input/output >> functions (2023-06-22 02:45:30 +0200) >> >> Migration Pull request (20230621) >> In this pull request: >> - fix for multifd thread creation (fabiano) >> - dirtylimity (hyman) >>* migration-test will go on next PULL request, as it has failures. >> - Improve error description (tejus) >> - improve -incoming and set parameters before calling incoming (wei) >> - migration atomic counters reviewed patches (quintela) >> - migration-test refacttoring reviewed (quintela) >> Please apply. > > You really need to test at least one 32-bit host regularly. > It should be trivial for you to do an i686 build somewhere. > > https://gitlab.com/qemu-project/qemu/-/jobs/4518975360#L4817 > https://gitlab.com/qemu-project/qemu/-/jobs/4518975263#L3486 > https://gitlab.com/qemu-project/qemu/-/jobs/4518975261#L3145 > https://gitlab.com/qemu-project/qemu/-/jobs/4518975298#L3372 > https://gitlab.com/qemu-project/qemu/-/jobs/4518975301#L3221 > > ../softmmu/dirtylimit.c:558:58: error: format specifies type 'long' > but the argument has type 'int64_t' (aka 'long long') > [-Werror,-Wformat] > error_setg(&err, "invalid dirty page limit %ld", dirty_rate); >~~~ ^~ >%lld Grrr, sorry. Will not happen again. Later, Juan. > > > r~
