[PATCH 0/6] Introduce multifd zero page checking.

[PATCH 0/6] Introduce multifd zero page checking.

[Hao Xiang]

This patchset is based on Juan Quintela's old series here
https://lore.kernel.org/all/20220802063907.18882-1-quintela@redhat.com/

In the multifd live migration model, there is a single migration main
thread scanning the page map, queuing the pages to multiple multifd
sender threads. The migration main thread runs zero page checking on
every page before queuing the page to the sender threads. Zero page
checking is a CPU intensive task and hence having a single thread doing
all that doesn't scale well. This change introduces a new function
to run the zero page checking on the multifd sender threads. This
patchset also lays the ground work for future changes to offload zero
page checking task to accelerator hardwares.

Use two Intel 4th generation Xeon servers for testing.

Architecture:        x86_64
CPU(s):              192
Thread(s) per core:  2
Core(s) per socket:  48
Socket(s):           2
NUMA node(s):        2
Vendor ID:           GenuineIntel
CPU family:          6
Model:               143
Model name:          Intel(R) Xeon(R) Platinum 8457C
Stepping:            8
CPU MHz:             2538.624
CPU max MHz:         3800.0000
CPU min MHz:         800.0000

Perform multifd live migration with below setup:
1. VM has 100GB memory. All pages in the VM are zero pages.
2. Use tcp socket for live migratio.
3. Use 4 multifd channels and zero page checking on migration main thread.
4. Use 1/2/4 multifd channels and zero page checking on multifd sender
threads.
5. Record migration total time from sender QEMU console's "info migrate"
command.
6. Calculate throughput with "100GB / total time".

+------------------------------------------------------+
|zero-page-checking | total-time(ms) | throughput(GB/s)|
+------------------------------------------------------+
|main-thread        | 9629           | 10.38GB/s       |
+------------------------------------------------------+
|multifd-1-threads  | 6182           | 16.17GB/s       |
+------------------------------------------------------+
|multifd-2-threads  | 4643           | 21.53GB/s       |
+------------------------------------------------------+
|multifd-4-threads  | 4143           | 24.13GB/s       |
+------------------------------------------------------+

Apply this patchset on top of commit
39a6e4f87e7b75a45b08d6dc8b8b7c2954c87440

Hao Xiang (6):
  migration/multifd: Add new migration option multifd-zero-page.
  migration/multifd: Add zero pages and zero bytes counter to migration
    status interface.
  migration/multifd: Support for zero pages transmission in multifd
    format.
  migration/multifd: Zero page transmission on the multifd thread.
  migration/multifd: Enable zero page checking from multifd threads.
  migration/multifd: Add a new migration test case for legacy zero page
    checking.

 migration/migration-hmp-cmds.c |  11 ++++
 migration/multifd.c            | 106 ++++++++++++++++++++++++++++-----
 migration/multifd.h            |  22 ++++++-
 migration/options.c            |  20 +++++++
 migration/options.h            |   1 +
 migration/ram.c                |  49 ++++++++++++---
 migration/trace-events         |   8 +--
 qapi/migration.json            |  39 ++++++++++--
 tests/qtest/migration-test.c   |  26 ++++++++
 9 files changed, 249 insertions(+), 33 deletions(-)

-- 
2.30.2

[PATCH 1/6] migration/multifd: Add new migration option multifd-zero-page.

[Hao Xiang]

This new parameter controls where the zero page checking is running. If
this parameter is set to true, zero page checking is done in the multifd
sender threads. If this parameter is set to false, zero page checking is
done in the migration main thread.

Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
---
 migration/migration-hmp-cmds.c |  7 +++++++
 migration/options.c            | 20 ++++++++++++++++++++
 migration/options.h            |  1 +
 qapi/migration.json            | 24 +++++++++++++++++++++---
 4 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 99b49df5dd..8b0c205a41 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -344,6 +344,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION),
             MultiFDCompression_str(params->multifd_compression));
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_ZERO_PAGE),
+            params->multifd_zero_page ? "on" : "off");
         monitor_printf(mon, "%s: %" PRIu64 " bytes\n",
             MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
             params->xbzrle_cache_size);
@@ -634,6 +637,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_multifd_zstd_level = true;
         visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
         break;
+    case MIGRATION_PARAMETER_MULTIFD_ZERO_PAGE:
+        p->has_multifd_zero_page = true;
+        visit_type_bool(v, param, &p->multifd_zero_page, &err);
+        break;
     case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
         p->has_xbzrle_cache_size = true;
         if (!visit_type_size(v, param, &cache_size, &err)) {
diff --git a/migration/options.c b/migration/options.c
index 3e3e0b93b4..cb18a41267 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -179,6 +179,8 @@ Property migration_properties[] = {
     DEFINE_PROP_MIG_MODE("mode", MigrationState,
                       parameters.mode,
                       MIG_MODE_NORMAL),
+    DEFINE_PROP_BOOL("multifd-zero-page", MigrationState,
+                     parameters.multifd_zero_page, true),
 
     /* Migration capabilities */
     DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
@@ -903,6 +905,13 @@ uint64_t migrate_xbzrle_cache_size(void)
     return s->parameters.xbzrle_cache_size;
 }
 
+bool migrate_multifd_zero_page(void)
+{
+    MigrationState *s = migrate_get_current();
+
+    return s->parameters.multifd_zero_page;
+}
+
 /* parameter setters */
 
 void migrate_set_block_incremental(bool value)
@@ -1013,6 +1022,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit;
     params->has_mode = true;
     params->mode = s->parameters.mode;
+    params->has_multifd_zero_page = true;
+    params->multifd_zero_page = s->parameters.multifd_zero_page;
 
     return params;
 }
@@ -1049,6 +1060,7 @@ void migrate_params_init(MigrationParameters *params)
     params->has_x_vcpu_dirty_limit_period = true;
     params->has_vcpu_dirty_limit = true;
     params->has_mode = true;
+    params->has_multifd_zero_page = true;
 }
 
 /*
@@ -1350,6 +1362,10 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     if (params->has_mode) {
         dest->mode = params->mode;
     }
+
+    if (params->has_multifd_zero_page) {
+        dest->multifd_zero_page = params->multifd_zero_page;
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -1494,6 +1510,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     if (params->has_mode) {
         s->parameters.mode = params->mode;
     }
+
+    if (params->has_multifd_zero_page) {
+        s->parameters.multifd_zero_page = params->multifd_zero_page;
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
diff --git a/migration/options.h b/migration/options.h
index 246c160aee..c080a6ba18 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -93,6 +93,7 @@ const char *migrate_tls_authz(void);
 const char *migrate_tls_creds(void);
 const char *migrate_tls_hostname(void);
 uint64_t migrate_xbzrle_cache_size(void);
+bool migrate_multifd_zero_page(void);
 
 /* parameters setters */
 
diff --git a/qapi/migration.json b/qapi/migration.json
index 819708321d..ff033a0344 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -874,6 +874,11 @@
 # @mode: Migration mode. See description in @MigMode. Default is 'normal'.
 #        (Since 8.2)
 #
+# @multifd-zero-page: Multifd zero page checking. If the parameter is true,
+#     zero page checking is done on the multifd sender thread. If the parameter
+#     is false, zero page checking is done on the migration main thread. Default
+#     is set to true. (Since 9.0)
+#
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated.  Use
@@ -907,7 +912,8 @@
            'block-bitmap-mapping',
            { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] },
            'vcpu-dirty-limit',
-           'mode'] }
+           'mode',
+           'multifd-zero-page'] }
 
 ##
 # @MigrateSetParameters:
@@ -1062,6 +1068,11 @@
 # @mode: Migration mode. See description in @MigMode. Default is 'normal'.
 #        (Since 8.2)
 #
+# @multifd-zero-page: Multifd zero page checking. If the parameter is true,
+#     zero page checking is done on the multifd sender thread. If the parameter
+#     is false, zero page checking is done on the migration main thread. Default
+#     is set to true. (Since 9.0)
+#
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated.  Use
@@ -1115,7 +1126,8 @@
             '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
                                             'features': [ 'unstable' ] },
             '*vcpu-dirty-limit': 'uint64',
-            '*mode': 'MigMode'} }
+            '*mode': 'MigMode',
+            '*multifd-zero-page': 'bool'} }
 
 ##
 # @migrate-set-parameters:
@@ -1290,6 +1302,11 @@
 # @mode: Migration mode. See description in @MigMode. Default is 'normal'.
 #        (Since 8.2)
 #
+# @multifd-zero-page: Multifd zero page checking. If the parameter is true,
+#     zero page checking is done on the multifd sender thread. If the parameter
+#     is false, zero page checking is done on the migration main thread. Default
+#     is set to true. (Since 9.0)
+#
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated.  Use
@@ -1340,7 +1357,8 @@
             '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
                                             'features': [ 'unstable' ] },
             '*vcpu-dirty-limit': 'uint64',
-            '*mode': 'MigMode'} }
+            '*mode': 'MigMode',
+            '*multifd-zero-page': 'bool'} }
 
 ##
 # @query-migrate-parameters:
-- 
2.30.2

Re: [PATCH 1/6] migration/multifd: Add new migration option multifd-zero-page.

[Peter Xu]

On Tue, Feb 06, 2024 at 11:19:03PM +0000, Hao Xiang wrote:
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 819708321d..ff033a0344 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -874,6 +874,11 @@
>  # @mode: Migration mode. See description in @MigMode. Default is 'normal'.
>  #        (Since 8.2)
>  #
> +# @multifd-zero-page: Multifd zero page checking. If the parameter is true,
> +#     zero page checking is done on the multifd sender thread. If the parameter
> +#     is false, zero page checking is done on the migration main thread. Default
> +#     is set to true. (Since 9.0)

I replied somewhere before on this, but I can try again..

Do you think it'll be better to introduce a generic parameter for zero page
detection?

  - "none" if disabled,
  - "legacy" for main thread,
  - "multifd" for multifd (software-based).

A string could work, but maybe cleaner to introduce
@MigrationZeroPageDetector enum?

When you add more, you can keep extending that with the single field
("multifd-dsa", etc.).

-- 
Peter Xu

Re: [External] Re: [PATCH 1/6] migration/multifd: Add new migration option multifd-zero-page.

[Hao Xiang]

On Tue, Feb 6, 2024 at 7:45 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Feb 06, 2024 at 11:19:03PM +0000, Hao Xiang wrote:
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 819708321d..ff033a0344 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -874,6 +874,11 @@
> >  # @mode: Migration mode. See description in @MigMode. Default is 'normal'.
> >  #        (Since 8.2)
> >  #
> > +# @multifd-zero-page: Multifd zero page checking. If the parameter is true,
> > +#     zero page checking is done on the multifd sender thread. If the parameter
> > +#     is false, zero page checking is done on the migration main thread. Default
> > +#     is set to true. (Since 9.0)
>
> I replied somewhere before on this, but I can try again..
>
> Do you think it'll be better to introduce a generic parameter for zero page
> detection?
>
>   - "none" if disabled,
>   - "legacy" for main thread,
>   - "multifd" for multifd (software-based).
>
> A string could work, but maybe cleaner to introduce
> @MigrationZeroPageDetector enum?
>
> When you add more, you can keep extending that with the single field
> ("multifd-dsa", etc.).
>
> --
> Peter Xu
>

Sorry I overlooked the previous email. This sounds like a good idea.

[PATCH 2/6] migration/multifd: Add zero pages and zero bytes counter to migration status interface.

[Hao Xiang]

This change extends the MigrationStatus interface to track zero pages
and zero bytes counter.

Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
---
 qapi/migration.json | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index ff033a0344..69366fe3f4 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -63,6 +63,10 @@
 #     between 0 and @dirty-sync-count * @multifd-channels.  (since
 #     7.1)
 #
+# @zero: number of zero pages (since 9.0)
+#
+# @zero-bytes: number of zero bytes sent (since 9.0)
+#
 # Features:
 #
 # @deprecated: Member @skipped is always zero since 1.5.3
@@ -81,7 +85,8 @@
            'multifd-bytes': 'uint64', 'pages-per-second': 'uint64',
            'precopy-bytes': 'uint64', 'downtime-bytes': 'uint64',
            'postcopy-bytes': 'uint64',
-           'dirty-sync-missed-zero-copy': 'uint64' } }
+           'dirty-sync-missed-zero-copy': 'uint64',
+           'zero': 'int', 'zero-bytes': 'int' } }
 
 ##
 # @XBZRLECacheStats:
@@ -332,6 +337,8 @@
 #           "duplicate":123,
 #           "normal":123,
 #           "normal-bytes":123456,
+#           "zero":123,
+#           "zero-bytes":123456,
 #           "dirty-sync-count":15
 #         }
 #      }
@@ -358,6 +365,8 @@
 #             "duplicate":123,
 #             "normal":123,
 #             "normal-bytes":123456,
+#             "zero":123,
+#             "zero-bytes":123456,
 #             "dirty-sync-count":15
 #          }
 #       }
@@ -379,6 +388,8 @@
 #             "duplicate":123,
 #             "normal":123,
 #             "normal-bytes":123456,
+#             "zero":123,
+#             "zero-bytes":123456,
 #             "dirty-sync-count":15
 #          },
 #          "disk":{
@@ -405,6 +416,8 @@
 #             "duplicate":10,
 #             "normal":3333,
 #             "normal-bytes":3412992,
+#             "zero":3333,
+#             "zero-bytes":3412992,
 #             "dirty-sync-count":15
 #          },
 #          "xbzrle-cache":{
-- 
2.30.2

Re: [PATCH 2/6] migration/multifd: Add zero pages and zero bytes counter to migration status interface.

[Peter Xu]

On Tue, Feb 06, 2024 at 11:19:04PM +0000, Hao Xiang wrote:
> This change extends the MigrationStatus interface to track zero pages
> and zero bytes counter.
> 
> Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

When post anything QAPI relevant, please always remember to copy QAPI
maintainers too, thanks.

$ ./scripts/get_maintainer.pl -f qapi/migration.json 
Eric Blake <eblake@redhat.com> (supporter:QAPI Schema)
Markus Armbruster <armbru@redhat.com> (supporter:QAPI Schema)
Peter Xu <peterx@redhat.com> (maintainer:Migration)
Fabiano Rosas <farosas@suse.de> (maintainer:Migration)
qemu-devel@nongnu.org (open list:All patches CC here)

-- 
Peter Xu

Re: [PATCH 2/6] migration/multifd: Add zero pages and zero bytes counter to migration status interface.

[Peter Xu]

On Wed, Feb 07, 2024 at 12:13:10PM +0800, Peter Xu wrote:
> On Tue, Feb 06, 2024 at 11:19:04PM +0000, Hao Xiang wrote:
> > This change extends the MigrationStatus interface to track zero pages
> > and zero bytes counter.
> > 
> > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>

I'll need to scratch this, sorry..

The issue is I forgot we have "duplicate" which is exactly "zero
page"s.. See:

    info->ram->duplicate = stat64_get(&mig_stats.zero_pages);

If you think the name too confusing and want a replacement, maybe it's fine
and maybe we can do that.  Then we can keep this zero page counter
introduced, reporting the same value as duplicates, then with a follow up
patch to deprecate "duplicate" parameter.  See an exmaple on how to
deprecate in 7b24d326348e1672.

One thing I'm not sure is whether Libvirt will be fine on losing
"duplicates" after 2+ QEMU major releases.  Copy Jiri for this.  My
understanding is that Libvirt should be keeping an eye on deprecation list
and react, but I'd like to double check..

Or we can keep using "duplicates", but I agree it just reads weird..

Thanks,

-- 
Peter Xu

Re: Re: [PATCH 2/6] migration/multifd: Add zero pages and zero bytes counter to migration status interface.

[Jiri Denemark]

On Wed, Feb 07, 2024 at 12:37:15 +0800, Peter Xu wrote:
> On Wed, Feb 07, 2024 at 12:13:10PM +0800, Peter Xu wrote:
> > On Tue, Feb 06, 2024 at 11:19:04PM +0000, Hao Xiang wrote:
> > > This change extends the MigrationStatus interface to track zero pages
> > > and zero bytes counter.
> > > 
> > > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
> > 
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> I'll need to scratch this, sorry..
> 
> The issue is I forgot we have "duplicate" which is exactly "zero
> page"s.. See:
> 
>     info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
> 
> If you think the name too confusing and want a replacement, maybe it's fine
> and maybe we can do that.  Then we can keep this zero page counter
> introduced, reporting the same value as duplicates, then with a follow up
> patch to deprecate "duplicate" parameter.  See an exmaple on how to
> deprecate in 7b24d326348e1672.
> 
> One thing I'm not sure is whether Libvirt will be fine on losing
> "duplicates" after 2+ QEMU major releases.  Copy Jiri for this.  My
> understanding is that Libvirt should be keeping an eye on deprecation list
> and react, but I'd like to double check..

This should not be a big deal as we can internally map either one
(depending on what QEMU supports) to the same libvirt's field. AFAIK
there is a consensus on Cc-ing libvirt-devel on patches that deprecate
QEMU interfaces so that we can update our code in time before the
deprecated interface is dropped.

BTW, libvirt maps "duplicate" to:

/**
 * VIR_DOMAIN_JOB_MEMORY_CONSTANT:
 *
 * virDomainGetJobStats field: number of pages filled with a constant
 * byte (all bytes in a single page are identical) transferred since the
 * beginning of the migration job, as VIR_TYPED_PARAM_ULLONG.
 *
 * The most common example of such pages are zero pages, i.e., pages filled
 * with zero bytes.
 *
 * Since: 1.0.3
 */
# define VIR_DOMAIN_JOB_MEMORY_CONSTANT          "memory_constant"

Jirka

Re: [External] Re: Re: [PATCH 2/6] migration/multifd: Add zero pages and zero bytes counter to migration status interface.

[Hao Xiang]

On Wed, Feb 7, 2024 at 12:41 AM Jiri Denemark <jdenemar@redhat.com> wrote:
>
> On Wed, Feb 07, 2024 at 12:37:15 +0800, Peter Xu wrote:
> > On Wed, Feb 07, 2024 at 12:13:10PM +0800, Peter Xu wrote:
> > > On Tue, Feb 06, 2024 at 11:19:04PM +0000, Hao Xiang wrote:
> > > > This change extends the MigrationStatus interface to track zero pages
> > > > and zero bytes counter.
> > > >
> > > > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
> > >
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> > I'll need to scratch this, sorry..
> >
> > The issue is I forgot we have "duplicate" which is exactly "zero
> > page"s.. See:
> >
> >     info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
> >
> > If you think the name too confusing and want a replacement, maybe it's fine
> > and maybe we can do that.  Then we can keep this zero page counter
> > introduced, reporting the same value as duplicates, then with a follow up
> > patch to deprecate "duplicate" parameter.  See an exmaple on how to
> > deprecate in 7b24d326348e1672.
> >
> > One thing I'm not sure is whether Libvirt will be fine on losing
> > "duplicates" after 2+ QEMU major releases.  Copy Jiri for this.  My
> > understanding is that Libvirt should be keeping an eye on deprecation list
> > and react, but I'd like to double check..
>
> This should not be a big deal as we can internally map either one
> (depending on what QEMU supports) to the same libvirt's field. AFAIK
> there is a consensus on Cc-ing libvirt-devel on patches that deprecate
> QEMU interfaces so that we can update our code in time before the
> deprecated interface is dropped.
>
> BTW, libvirt maps "duplicate" to:
>
> /**
>  * VIR_DOMAIN_JOB_MEMORY_CONSTANT:
>  *
>  * virDomainGetJobStats field: number of pages filled with a constant
>  * byte (all bytes in a single page are identical) transferred since the
>  * beginning of the migration job, as VIR_TYPED_PARAM_ULLONG.
>  *
>  * The most common example of such pages are zero pages, i.e., pages filled
>  * with zero bytes.
>  *
>  * Since: 1.0.3
>  */
> # define VIR_DOMAIN_JOB_MEMORY_CONSTANT          "memory_constant"
>
> Jirka
>

Interesting. I didn't notice the existence of "duplicate" for zero
pages. I do think the name is quite confusing. I will create the
"zero/zero_bytes" counter and a separate commit to deprecate
"duplicate". Will add libvirt devs per instruction above.

Re: [External] Re: Re: [PATCH 2/6] migration/multifd: Add zero pages and zero bytes counter to migration status interface.

[Peter Xu]

On Wed, Feb 07, 2024 at 03:44:18PM -0800, Hao Xiang wrote:
> On Wed, Feb 7, 2024 at 12:41 AM Jiri Denemark <jdenemar@redhat.com> wrote:
> >
> > On Wed, Feb 07, 2024 at 12:37:15 +0800, Peter Xu wrote:
> > > On Wed, Feb 07, 2024 at 12:13:10PM +0800, Peter Xu wrote:
> > > > On Tue, Feb 06, 2024 at 11:19:04PM +0000, Hao Xiang wrote:
> > > > > This change extends the MigrationStatus interface to track zero pages
> > > > > and zero bytes counter.
> > > > >
> > > > > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
> > > >
> > > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > >
> > > I'll need to scratch this, sorry..
> > >
> > > The issue is I forgot we have "duplicate" which is exactly "zero
> > > page"s.. See:
> > >
> > >     info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
> > >
> > > If you think the name too confusing and want a replacement, maybe it's fine
> > > and maybe we can do that.  Then we can keep this zero page counter
> > > introduced, reporting the same value as duplicates, then with a follow up
> > > patch to deprecate "duplicate" parameter.  See an exmaple on how to
> > > deprecate in 7b24d326348e1672.
> > >
> > > One thing I'm not sure is whether Libvirt will be fine on losing
> > > "duplicates" after 2+ QEMU major releases.  Copy Jiri for this.  My
> > > understanding is that Libvirt should be keeping an eye on deprecation list
> > > and react, but I'd like to double check..
> >
> > This should not be a big deal as we can internally map either one
> > (depending on what QEMU supports) to the same libvirt's field. AFAIK
> > there is a consensus on Cc-ing libvirt-devel on patches that deprecate

I see.

> > QEMU interfaces so that we can update our code in time before the
> > deprecated interface is dropped.

Right.

What I mostly worried is "old libvirt" + "new qemu", where the old libvirt
only knows "duplicates", while the new (after 2 releases) will only report
"zeros".

> >
> > BTW, libvirt maps "duplicate" to:
> >
> > /**
> >  * VIR_DOMAIN_JOB_MEMORY_CONSTANT:
> >  *
> >  * virDomainGetJobStats field: number of pages filled with a constant
> >  * byte (all bytes in a single page are identical) transferred since the
> >  * beginning of the migration job, as VIR_TYPED_PARAM_ULLONG.
> >  *
> >  * The most common example of such pages are zero pages, i.e., pages filled
> >  * with zero bytes.
> >  *
> >  * Since: 1.0.3
> >  */
> > # define VIR_DOMAIN_JOB_MEMORY_CONSTANT          "memory_constant"
> >
> > Jirka
> >
> 
> Interesting. I didn't notice the existence of "duplicate" for zero
> pages. I do think the name is quite confusing. I will create the
> "zero/zero_bytes" counter and a separate commit to deprecate
> "duplicate". Will add libvirt devs per instruction above.

Yeah, please go ahead, and I hope my worry is not a real concern above; we
can figure that out later.  Even without deprecating "duplicate", maybe
it'll at least still be worthwhile we start having "zeros" reported
alongside.  Then after 10/20/30/N years we always have a chance to
deprecate the other one, just a matter of compatible window.

Thanks,

-- 
Peter Xu

[PATCH 3/6] migration/multifd: Support for zero pages transmission in multifd format.

[Hao Xiang]

This change adds zero page counters and updates multifd send/receive
tracing format to track the newly added counters.

Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
---
 migration/migration-hmp-cmds.c |  4 ++++
 migration/multifd.c            | 43 ++++++++++++++++++++++++++--------
 migration/multifd.h            | 17 +++++++++++++-
 migration/trace-events         |  8 +++----
 4 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 8b0c205a41..2dd99b0509 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -111,6 +111,10 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->ram->normal);
         monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
                        info->ram->normal_bytes >> 10);
+        monitor_printf(mon, "zero: %" PRIu64 " pages\n",
+                       info->ram->zero);
+        monitor_printf(mon, "zero bytes: %" PRIu64 " kbytes\n",
+                       info->ram->zero_bytes >> 10);
         monitor_printf(mon, "dirty sync count: %" PRIu64 "\n",
                        info->ram->dirty_sync_count);
         monitor_printf(mon, "page size: %" PRIu64 " kbytes\n",
diff --git a/migration/multifd.c b/migration/multifd.c
index 25cbc6dc6b..a20d0ed10e 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -264,6 +264,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
     packet->flags = cpu_to_be32(p->flags);
     packet->pages_alloc = cpu_to_be32(p->pages->allocated);
     packet->normal_pages = cpu_to_be32(p->normal_num);
+    packet->zero_pages = cpu_to_be32(p->zero_num);
     packet->next_packet_size = cpu_to_be32(p->next_packet_size);
     packet->packet_num = cpu_to_be64(p->packet_num);
 
@@ -317,18 +318,26 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
     p->normal_num = be32_to_cpu(packet->normal_pages);
     if (p->normal_num > packet->pages_alloc) {
         error_setg(errp, "multifd: received packet "
-                   "with %u pages and expected maximum pages are %u",
+                   "with %u normal pages and expected maximum pages are %u",
                    p->normal_num, packet->pages_alloc) ;
         return -1;
     }
 
-    p->next_packet_size = be32_to_cpu(packet->next_packet_size);
-    p->packet_num = be64_to_cpu(packet->packet_num);
+    p->zero_num = be32_to_cpu(packet->zero_pages);
+    if (p->zero_num > packet->pages_alloc - p->normal_num) {
+        error_setg(errp, "multifd: received packet "
+                   "with %u zero pages and expected maximum zero pages are %u",
+                   p->zero_num, packet->pages_alloc - p->normal_num) ;
+        return -1;
+    }
 
-    if (p->normal_num == 0) {
+    if (p->normal_num == 0 && p->zero_num == 0) {
         return 0;
     }
 
+    p->next_packet_size = be32_to_cpu(packet->next_packet_size);
+    p->packet_num = be64_to_cpu(packet->packet_num);
+
     /* make sure that ramblock is 0 terminated */
     packet->ramblock[255] = 0;
     p->block = qemu_ram_block_by_name(packet->ramblock);
@@ -430,6 +439,7 @@ static int multifd_send_pages(void)
     p->packet_num = multifd_send_state->packet_num++;
     multifd_send_state->pages = p->pages;
     p->pages = pages;
+
     qemu_mutex_unlock(&p->mutex);
     qemu_sem_post(&p->sem);
 
@@ -551,6 +561,8 @@ void multifd_save_cleanup(void)
         p->iov = NULL;
         g_free(p->normal);
         p->normal = NULL;
+        g_free(p->zero);
+        p->zero = NULL;
         multifd_send_state->ops->send_cleanup(p, &local_err);
         if (local_err) {
             migrate_set_error(migrate_get_current(), local_err);
@@ -679,6 +691,7 @@ static void *multifd_send_thread(void *opaque)
             uint64_t packet_num = p->packet_num;
             uint32_t flags;
             p->normal_num = 0;
+            p->zero_num = 0;
 
             if (use_zero_copy_send) {
                 p->iovs_num = 0;
@@ -703,12 +716,13 @@ static void *multifd_send_thread(void *opaque)
             p->flags = 0;
             p->num_packets++;
             p->total_normal_pages += p->normal_num;
+            p->total_zero_pages += p->zero_num;
             p->pages->num = 0;
             p->pages->block = NULL;
             qemu_mutex_unlock(&p->mutex);
 
-            trace_multifd_send(p->id, packet_num, p->normal_num, flags,
-                               p->next_packet_size);
+            trace_multifd_send(p->id, packet_num, p->normal_num, p->zero_num,
+                               flags, p->next_packet_size);
 
             if (use_zero_copy_send) {
                 /* Send header first, without zerocopy */
@@ -731,6 +745,8 @@ static void *multifd_send_thread(void *opaque)
 
             stat64_add(&mig_stats.multifd_bytes,
                        p->next_packet_size + p->packet_len);
+            stat64_add(&mig_stats.normal_pages, p->normal_num);
+            stat64_add(&mig_stats.zero_pages, p->zero_num);
             p->next_packet_size = 0;
             qemu_mutex_lock(&p->mutex);
             p->pending_job--;
@@ -761,7 +777,8 @@ out:
 
     rcu_unregister_thread();
     migration_threads_remove(thread);
-    trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
+    trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages,
+                                  p->total_zero_pages);
 
     return NULL;
 }
@@ -936,6 +953,7 @@ int multifd_save_setup(Error **errp)
         /* We need one extra place for the packet header */
         p->iov = g_new0(struct iovec, page_count + 1);
         p->normal = g_new0(ram_addr_t, page_count);
+        p->zero = g_new0(ram_addr_t, page_count);
         p->page_size = qemu_target_page_size();
         p->page_count = page_count;
 
@@ -1051,6 +1069,8 @@ void multifd_load_cleanup(void)
         p->iov = NULL;
         g_free(p->normal);
         p->normal = NULL;
+        g_free(p->zero);
+        p->zero = NULL;
         multifd_recv_state->ops->recv_cleanup(p);
     }
     qemu_sem_destroy(&multifd_recv_state->sem_sync);
@@ -1119,10 +1139,11 @@ static void *multifd_recv_thread(void *opaque)
         flags = p->flags;
         /* recv methods don't know how to handle the SYNC flag */
         p->flags &= ~MULTIFD_FLAG_SYNC;
-        trace_multifd_recv(p->id, p->packet_num, p->normal_num, flags,
-                           p->next_packet_size);
+        trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num,
+                           flags, p->next_packet_size);
         p->num_packets++;
         p->total_normal_pages += p->normal_num;
+        p->total_zero_pages += p->zero_num;
         qemu_mutex_unlock(&p->mutex);
 
         if (p->normal_num) {
@@ -1147,7 +1168,8 @@ static void *multifd_recv_thread(void *opaque)
     qemu_mutex_unlock(&p->mutex);
 
     rcu_unregister_thread();
-    trace_multifd_recv_thread_end(p->id, p->num_packets, p->total_normal_pages);
+    trace_multifd_recv_thread_end(p->id, p->num_packets, p->total_normal_pages,
+                                  p->total_zero_pages);
 
     return NULL;
 }
@@ -1186,6 +1208,7 @@ int multifd_load_setup(Error **errp)
         p->name = g_strdup_printf("multifdrecv_%d", i);
         p->iov = g_new0(struct iovec, page_count);
         p->normal = g_new0(ram_addr_t, page_count);
+        p->zero = g_new0(ram_addr_t, page_count);
         p->page_count = page_count;
         p->page_size = qemu_target_page_size();
     }
diff --git a/migration/multifd.h b/migration/multifd.h
index 35d11f103c..6be9b2f6c1 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -48,7 +48,10 @@ typedef struct {
     /* size of the next packet that contains pages */
     uint32_t next_packet_size;
     uint64_t packet_num;
-    uint64_t unused[4];    /* Reserved for future use */
+    /* zero pages */
+    uint32_t zero_pages;
+    uint32_t unused32[1];    /* Reserved for future use */
+    uint64_t unused64[3];    /* Reserved for future use */
     char ramblock[256];
     uint64_t offset[];
 } __attribute__((packed)) MultiFDPacket_t;
@@ -120,6 +123,8 @@ typedef struct {
     uint64_t num_packets;
     /* non zero pages sent through this channel */
     uint64_t total_normal_pages;
+    /* zero pages sent through this channel */
+    uint64_t total_zero_pages;
     /* buffers to send */
     struct iovec *iov;
     /* number of iovs used */
@@ -128,6 +133,10 @@ typedef struct {
     ram_addr_t *normal;
     /* num of non zero pages */
     uint32_t normal_num;
+    /* Pages that are zero */
+    ram_addr_t *zero;
+    /* num of zero pages */
+    uint32_t zero_num;
     /* used for compression methods */
     void *data;
 }  MultiFDSendParams;
@@ -179,12 +188,18 @@ typedef struct {
     uint8_t *host;
     /* non zero pages recv through this channel */
     uint64_t total_normal_pages;
+    /* zero pages recv through this channel */
+    uint64_t total_zero_pages;
     /* buffers to recv */
     struct iovec *iov;
     /* Pages that are not zero */
     ram_addr_t *normal;
     /* num of non zero pages */
     uint32_t normal_num;
+    /* Pages that are zero */
+    ram_addr_t *zero;
+    /* num of zero pages */
+    uint32_t zero_num;
     /* used for de-compression methods */
     void *data;
 } MultiFDRecvParams;
diff --git a/migration/trace-events b/migration/trace-events
index de4a743c8a..c0a758db9d 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -128,21 +128,21 @@ postcopy_preempt_reset_channel(void) ""
 # multifd.c
 multifd_new_send_channel_async(uint8_t id) "channel %u"
 multifd_new_send_channel_async_error(uint8_t id, void *err) "channel=%u err=%p"
-multifd_recv(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " pages %u flags 0x%x next packet size %u"
+multifd_recv(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t zero, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %u"
 multifd_recv_new_channel(uint8_t id) "channel %u"
 multifd_recv_sync_main(long packet_num) "packet num %ld"
 multifd_recv_sync_main_signal(uint8_t id) "channel %u"
 multifd_recv_sync_main_wait(uint8_t id) "channel %u"
 multifd_recv_terminate_threads(bool error) "error %d"
-multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t pages) "channel %u packets %" PRIu64 " pages %" PRIu64
+multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages, uint64_t zero_pages) "channel %u packets %" PRIu64 " normal pages %" PRIu64 " zero pages %" PRIu64
 multifd_recv_thread_start(uint8_t id) "%u"
-multifd_send(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u flags 0x%x next packet size %u"
+multifd_send(uint8_t id, uint64_t packet_num, uint32_t normalpages, uint32_t zero_pages, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %u"
 multifd_send_error(uint8_t id) "channel %u"
 multifd_send_sync_main(long packet_num) "packet num %ld"
 multifd_send_sync_main_signal(uint8_t id) "channel %u"
 multifd_send_sync_main_wait(uint8_t id) "channel %u"
 multifd_send_terminate_threads(bool error) "error %d"
-multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages) "channel %u packets %" PRIu64 " normal pages %"  PRIu64
+multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages, uint64_t zero_pages) "channel %u packets %" PRIu64 " normal pages %"  PRIu64 " zero pages %"  PRIu64
 multifd_send_thread_start(uint8_t id) "%u"
 multifd_tls_outgoing_handshake_start(void *ioc, void *tioc, const char *hostname) "ioc=%p tioc=%p hostname=%s"
 multifd_tls_outgoing_handshake_error(void *ioc, const char *err) "ioc=%p err=%s"
-- 
2.30.2

Re: [PATCH 3/6] migration/multifd: Support for zero pages transmission in multifd format.

[Peter Xu]

On Tue, Feb 06, 2024 at 11:19:05PM +0000, Hao Xiang wrote:
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 25cbc6dc6b..a20d0ed10e 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -264,6 +264,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
>      packet->flags = cpu_to_be32(p->flags);
>      packet->pages_alloc = cpu_to_be32(p->pages->allocated);
>      packet->normal_pages = cpu_to_be32(p->normal_num);
> +    packet->zero_pages = cpu_to_be32(p->zero_num);

This doesn't look right..

If to fill up the zero accounting only, we shouldn't be touching multifd
packet at all since multifd zero page detection is not yet supported.

We should only reference mig_stats.zero_pages.

>      packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>      packet->packet_num = cpu_to_be64(p->packet_num);

-- 
Peter Xu

Re: [External] Re: [PATCH 3/6] migration/multifd: Support for zero pages transmission in multifd format.

[Hao Xiang]

On Tue, Feb 6, 2024 at 8:25 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Feb 06, 2024 at 11:19:05PM +0000, Hao Xiang wrote:
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 25cbc6dc6b..a20d0ed10e 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -264,6 +264,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
> >      packet->flags = cpu_to_be32(p->flags);
> >      packet->pages_alloc = cpu_to_be32(p->pages->allocated);
> >      packet->normal_pages = cpu_to_be32(p->normal_num);
> > +    packet->zero_pages = cpu_to_be32(p->zero_num);
>
> This doesn't look right..
>
> If to fill up the zero accounting only, we shouldn't be touching multifd
> packet at all since multifd zero page detection is not yet supported.
>
> We should only reference mig_stats.zero_pages.

p->zero_num will always be 0 because multifd zero page checking is not
yet enabled. The next commit will contain the code to calculate
p->zero_num for each packet. This is just a preparation for the next
commit that enables the feature. Main migration thread zero page goes
through a different format. We are using the same counter
mig_stats.zero_pages to track zero pages for both the legacy zero page
checking and multifd zero page checking. These two are mutually
exclusive so zero page will not be double counted.

Let me know if I am missing something.

>
> >      packet->next_packet_size = cpu_to_be32(p->next_packet_size);
> >      packet->packet_num = cpu_to_be64(p->packet_num);
>
> --
> Peter Xu
>

[PATCH 4/6] migration/multifd: Zero page transmission on the multifd thread.

[Hao Xiang]

This implements the zero page detection and handling on the multifd
threads.

Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
---
 migration/multifd.c | 62 +++++++++++++++++++++++++++++++++++++++++----
 migration/multifd.h |  5 ++++
 2 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index a20d0ed10e..c031f947c7 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "qemu/rcu.h"
 #include "exec/target_page.h"
 #include "sysemu/sysemu.h"
@@ -278,6 +279,12 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
 
         packet->offset[i] = cpu_to_be64(temp);
     }
+    for (i = 0; i < p->zero_num; i++) {
+        /* there are architectures where ram_addr_t is 32 bit */
+        uint64_t temp = p->zero[i];
+
+        packet->offset[p->normal_num + i] = cpu_to_be64(temp);
+    }
 }
 
 static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
@@ -360,6 +367,18 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
         p->normal[i] = offset;
     }
 
+    for (i = 0; i < p->zero_num; i++) {
+        uint64_t offset = be64_to_cpu(packet->offset[p->normal_num + i]);
+
+        if (offset > (p->block->used_length - p->page_size)) {
+            error_setg(errp, "multifd: offset too long %" PRIu64
+                       " (max " RAM_ADDR_FMT ")",
+                       offset, p->block->used_length);
+            return -1;
+        }
+        p->zero[i] = offset;
+    }
+
     return 0;
 }
 
@@ -658,13 +677,37 @@ int multifd_send_sync_main(void)
     return 0;
 }
 
+static void zero_page_check_send(MultiFDSendParams *p)
+{
+    /*
+     * QEMU older than 9.0 don't understand zero page
+     * on multifd channel. This switch is required to
+     * maintain backward compatibility.
+     */
+    bool use_multifd_zero_page = migrate_multifd_zero_page();
+    RAMBlock *rb = p->pages->block;
+
+    for (int i = 0; i < p->pages->num; i++) {
+        uint64_t offset = p->pages->offset[i];
+        if (use_multifd_zero_page &&
+            buffer_is_zero(rb->host + offset, p->page_size)) {
+            p->zero[p->zero_num] = offset;
+            p->zero_num++;
+            ram_release_page(rb->idstr, offset);
+        } else {
+            p->normal[p->normal_num] = offset;
+            p->normal_num++;
+        }
+    }
+}
+
 static void *multifd_send_thread(void *opaque)
 {
     MultiFDSendParams *p = opaque;
     MigrationThread *thread = NULL;
     Error *local_err = NULL;
-    int ret = 0;
     bool use_zero_copy_send = migrate_zero_copy_send();
+    int ret = 0;
 
     thread = migration_threads_add(p->name, qemu_get_thread_id());
 
@@ -699,10 +742,7 @@ static void *multifd_send_thread(void *opaque)
                 p->iovs_num = 1;
             }
 
-            for (int i = 0; i < p->pages->num; i++) {
-                p->normal[p->normal_num] = p->pages->offset[i];
-                p->normal_num++;
-            }
+            zero_page_check_send(p);
 
             if (p->normal_num) {
                 ret = multifd_send_state->ops->send_prepare(p, &local_err);
@@ -1107,6 +1147,16 @@ void multifd_recv_sync_main(void)
     trace_multifd_recv_sync_main(multifd_recv_state->packet_num);
 }
 
+static void zero_page_check_recv(MultiFDRecvParams *p)
+{
+    for (int i = 0; i < p->zero_num; i++) {
+        void *page = p->host + p->zero[i];
+        if (!buffer_is_zero(page, p->page_size)) {
+            memset(page, 0, p->page_size);
+        }
+    }
+}
+
 static void *multifd_recv_thread(void *opaque)
 {
     MultiFDRecvParams *p = opaque;
@@ -1153,6 +1203,8 @@ static void *multifd_recv_thread(void *opaque)
             }
         }
 
+        zero_page_check_recv(p);
+
         if (flags & MULTIFD_FLAG_SYNC) {
             qemu_sem_post(&multifd_recv_state->sem_sync);
             qemu_sem_wait(&p->sem_sync);
diff --git a/migration/multifd.h b/migration/multifd.h
index 6be9b2f6c1..7448cb1aa9 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -53,6 +53,11 @@ typedef struct {
     uint32_t unused32[1];    /* Reserved for future use */
     uint64_t unused64[3];    /* Reserved for future use */
     char ramblock[256];
+    /*
+     * This array contains the pointers to:
+     *  - normal pages (initial normal_pages entries)
+     *  - zero pages (following zero_pages entries)
+     */
     uint64_t offset[];
 } __attribute__((packed)) MultiFDPacket_t;
 
-- 
2.30.2

Re: [PATCH 4/6] migration/multifd: Zero page transmission on the multifd thread.

[Peter Xu]

On Tue, Feb 06, 2024 at 11:19:06PM +0000, Hao Xiang wrote:
> This implements the zero page detection and handling on the multifd
> threads.
> 
> Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
> ---
>  migration/multifd.c | 62 +++++++++++++++++++++++++++++++++++++++++----
>  migration/multifd.h |  5 ++++
>  2 files changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index a20d0ed10e..c031f947c7 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "qemu/rcu.h"
>  #include "exec/target_page.h"
>  #include "sysemu/sysemu.h"
> @@ -278,6 +279,12 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
>  
>          packet->offset[i] = cpu_to_be64(temp);
>      }
> +    for (i = 0; i < p->zero_num; i++) {
> +        /* there are architectures where ram_addr_t is 32 bit */
> +        uint64_t temp = p->zero[i];
> +
> +        packet->offset[p->normal_num + i] = cpu_to_be64(temp);
> +    }
>  }

Please be noted taht p->normal_num will be dropped very soon, see:

https://lore.kernel.org/all/20240202102857.110210-6-peterx@redhat.com/

Please use p->pages->num instead.

This patch also relies on some changes in previous patch.. IMHO we can
split the patch better in this way:

  - Patch 1: Add new parameter "zero-page-detection", support "none",
    "legacy".  You'll need to implement "none" here that we skip zero page
    by returning 0 in save_zero_page() if "none".

  - Patch 2: Add new "multifd" mode in above, implement it in the same
    patch completely.

  - Patch 3: introduce ram_save_target_page_multifd()

  - Patch 4: test case

If you want to add "zeros" accounting, that can be done as more patches on
top.

Thanks,

-- 
Peter Xu

[PATCH 5/6] migration/multifd: Enable zero page checking from multifd threads.

[Hao Xiang]

This change adds a dedicated handler for MigrationOps::ram_save_target_page in
multifd live migration. Now zero page checking can be done in the multifd threads
and this becomes the default configuration. We still provide backward compatibility
where zero page checking is done from the migration main thread.

Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
---
 migration/multifd.c |  3 ++-
 migration/ram.c     | 49 ++++++++++++++++++++++++++++++++++++---------
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index c031f947c7..c6833ccb07 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "qemu/rcu.h"
+#include "qemu/cutils.h"
 #include "exec/target_page.h"
 #include "sysemu/sysemu.h"
 #include "exec/ramblock.h"
@@ -458,7 +459,6 @@ static int multifd_send_pages(void)
     p->packet_num = multifd_send_state->packet_num++;
     multifd_send_state->pages = p->pages;
     p->pages = pages;
-
     qemu_mutex_unlock(&p->mutex);
     qemu_sem_post(&p->sem);
 
@@ -733,6 +733,7 @@ static void *multifd_send_thread(void *opaque)
         if (p->pending_job) {
             uint64_t packet_num = p->packet_num;
             uint32_t flags;
+
             p->normal_num = 0;
             p->zero_num = 0;
 
diff --git a/migration/ram.c b/migration/ram.c
index d5b7cd5ac2..e6742c9593 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1252,6 +1252,10 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
 
 static int ram_save_multifd_page(RAMBlock *block, ram_addr_t offset)
 {
+    assert(migrate_multifd());
+    assert(!migrate_compress());
+    assert(!migration_in_postcopy());
+
     if (multifd_queue_page(block, offset) < 0) {
         return -1;
     }
@@ -2043,7 +2047,6 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
  */
 static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
 {
-    RAMBlock *block = pss->block;
     ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
     int res;
 
@@ -2059,17 +2062,40 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
         return 1;
     }
 
+    return ram_save_page(rs, pss);
+}
+
+/**
+ * ram_save_target_page_multifd: save one target page
+ *
+ * Returns the number of pages written
+ *
+ * @rs: current RAM state
+ * @pss: data about the page we want to send
+ */
+static int ram_save_target_page_multifd(RAMState *rs, PageSearchStatus *pss)
+{
+    RAMBlock *block = pss->block;
+    ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
+
+    /* Multifd is not compatible with old compression. */
+    assert(!migrate_compress());
+
+    /* Multifd is not compabible with postcopy. */
+    assert(!migration_in_postcopy());
+
     /*
-     * Do not use multifd in postcopy as one whole host page should be
-     * placed.  Meanwhile postcopy requires atomic update of pages, so even
-     * if host page size == guest page size the dest guest during run may
-     * still see partially copied pages which is data corruption.
+     * Backward compatibility support. While using multifd live
+     * migration, we still need to handle zero page checking on the
+     * migration main thread.
      */
-    if (migrate_multifd() && !migration_in_postcopy()) {
-        return ram_save_multifd_page(block, offset);
+    if (!migrate_multifd_zero_page()) {
+        if (save_zero_page(rs, pss, offset)) {
+            return 1;
+        }
     }
 
-    return ram_save_page(rs, pss);
+    return ram_save_multifd_page(block, offset);
 }
 
 /* Should be called before sending a host page */
@@ -2981,7 +3007,12 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     }
 
     migration_ops = g_malloc0(sizeof(MigrationOps));
-    migration_ops->ram_save_target_page = ram_save_target_page_legacy;
+
+    if (migrate_multifd()) {
+        migration_ops->ram_save_target_page = ram_save_target_page_multifd;
+    } else {
+        migration_ops->ram_save_target_page = ram_save_target_page_legacy;
+    }
 
     bql_unlock();
     ret = multifd_send_sync_main();
-- 
2.30.2

[PATCH 6/6] migration/multifd: Add a new migration test case for legacy zero page checking.

[Hao Xiang]

Now that zero page checking is done on the multifd sender threads by
default, we still provide an option for backward compatibility. This
change adds a qtest migration test case to set the multifd-zero-page
option to false and run multifd migration with zero page checking on the
migration main thread.

Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
---
 tests/qtest/migration-test.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7675519cfa..2c13df04c3 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2621,6 +2621,15 @@ test_migrate_precopy_tcp_multifd_start(QTestState *from,
     return test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
 }
 
+static void *
+test_migrate_precopy_tcp_multifd_start_zero_page_legacy(QTestState *from,
+                                                        QTestState *to)
+{
+    test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
+    migrate_set_parameter_bool(from, "multifd-zero-page", false);
+    return NULL;
+}
+
 static void *
 test_migrate_precopy_tcp_multifd_zlib_start(QTestState *from,
                                             QTestState *to)
@@ -2652,6 +2661,21 @@ static void test_multifd_tcp_none(void)
     test_precopy_common(&args);
 }
 
+static void test_multifd_tcp_zero_page_legacy(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "defer",
+        .start_hook = test_migrate_precopy_tcp_multifd_start_zero_page_legacy,
+        /*
+         * Multifd is more complicated than most of the features, it
+         * directly takes guest page buffers when sending, make sure
+         * everything will work alright even if guest page is changing.
+         */
+        .live = true,
+    };
+    test_precopy_common(&args);
+}
+
 static void test_multifd_tcp_zlib(void)
 {
     MigrateCommon args = {
@@ -3550,6 +3574,8 @@ int main(int argc, char **argv)
     }
     migration_test_add("/migration/multifd/tcp/plain/none",
                        test_multifd_tcp_none);
+    migration_test_add("/migration/multifd/tcp/plain/zero_page_legacy",
+                       test_multifd_tcp_zero_page_legacy);
     migration_test_add("/migration/multifd/tcp/plain/cancel",
                        test_multifd_tcp_cancel);
     migration_test_add("/migration/multifd/tcp/plain/zlib",
-- 
2.30.2

Re: [PATCH 0/6] Introduce multifd zero page checking.

[Peter Xu]

On Tue, Feb 06, 2024 at 11:19:02PM +0000, Hao Xiang wrote:
> This patchset is based on Juan Quintela's old series here
> https://lore.kernel.org/all/20220802063907.18882-1-quintela@redhat.com/
> 
> In the multifd live migration model, there is a single migration main
> thread scanning the page map, queuing the pages to multiple multifd
> sender threads. The migration main thread runs zero page checking on
> every page before queuing the page to the sender threads. Zero page
> checking is a CPU intensive task and hence having a single thread doing
> all that doesn't scale well. This change introduces a new function
> to run the zero page checking on the multifd sender threads. This
> patchset also lays the ground work for future changes to offload zero
> page checking task to accelerator hardwares.
> 
> Use two Intel 4th generation Xeon servers for testing.
> 
> Architecture:        x86_64
> CPU(s):              192
> Thread(s) per core:  2
> Core(s) per socket:  48
> Socket(s):           2
> NUMA node(s):        2
> Vendor ID:           GenuineIntel
> CPU family:          6
> Model:               143
> Model name:          Intel(R) Xeon(R) Platinum 8457C
> Stepping:            8
> CPU MHz:             2538.624
> CPU max MHz:         3800.0000
> CPU min MHz:         800.0000
> 
> Perform multifd live migration with below setup:
> 1. VM has 100GB memory. All pages in the VM are zero pages.
> 2. Use tcp socket for live migratio.
> 3. Use 4 multifd channels and zero page checking on migration main thread.
> 4. Use 1/2/4 multifd channels and zero page checking on multifd sender
> threads.
> 5. Record migration total time from sender QEMU console's "info migrate"
> command.
> 6. Calculate throughput with "100GB / total time".
> 
> +------------------------------------------------------+
> |zero-page-checking | total-time(ms) | throughput(GB/s)|
> +------------------------------------------------------+
> |main-thread        | 9629           | 10.38GB/s       |
> +------------------------------------------------------+
> |multifd-1-threads  | 6182           | 16.17GB/s       |
> +------------------------------------------------------+
> |multifd-2-threads  | 4643           | 21.53GB/s       |
> +------------------------------------------------------+
> |multifd-4-threads  | 4143           | 24.13GB/s       |
> +------------------------------------------------------+

This "throughput" is slightly confusing; I was initially surprised to see a
large throughput for idle guests.  IMHO the "total-time" would explain.
Feel free to drop that column if there's a repost.

Did you check why 4 channels mostly already reached the top line?  Is it
because main thread is already spinning 100%?

Thanks,

-- 
Peter Xu

Re: [External] Re: [PATCH 0/6] Introduce multifd zero page checking.

[Hao Xiang]

On Tue, Feb 6, 2024 at 7:39 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Feb 06, 2024 at 11:19:02PM +0000, Hao Xiang wrote:
> > This patchset is based on Juan Quintela's old series here
> > https://lore.kernel.org/all/20220802063907.18882-1-quintela@redhat.com/
> >
> > In the multifd live migration model, there is a single migration main
> > thread scanning the page map, queuing the pages to multiple multifd
> > sender threads. The migration main thread runs zero page checking on
> > every page before queuing the page to the sender threads. Zero page
> > checking is a CPU intensive task and hence having a single thread doing
> > all that doesn't scale well. This change introduces a new function
> > to run the zero page checking on the multifd sender threads. This
> > patchset also lays the ground work for future changes to offload zero
> > page checking task to accelerator hardwares.
> >
> > Use two Intel 4th generation Xeon servers for testing.
> >
> > Architecture:        x86_64
> > CPU(s):              192
> > Thread(s) per core:  2
> > Core(s) per socket:  48
> > Socket(s):           2
> > NUMA node(s):        2
> > Vendor ID:           GenuineIntel
> > CPU family:          6
> > Model:               143
> > Model name:          Intel(R) Xeon(R) Platinum 8457C
> > Stepping:            8
> > CPU MHz:             2538.624
> > CPU max MHz:         3800.0000
> > CPU min MHz:         800.0000
> >
> > Perform multifd live migration with below setup:
> > 1. VM has 100GB memory. All pages in the VM are zero pages.
> > 2. Use tcp socket for live migratio.
> > 3. Use 4 multifd channels and zero page checking on migration main thread.
> > 4. Use 1/2/4 multifd channels and zero page checking on multifd sender
> > threads.
> > 5. Record migration total time from sender QEMU console's "info migrate"
> > command.
> > 6. Calculate throughput with "100GB / total time".
> >
> > +------------------------------------------------------+
> > |zero-page-checking | total-time(ms) | throughput(GB/s)|
> > +------------------------------------------------------+
> > |main-thread        | 9629           | 10.38GB/s       |
> > +------------------------------------------------------+
> > |multifd-1-threads  | 6182           | 16.17GB/s       |
> > +------------------------------------------------------+
> > |multifd-2-threads  | 4643           | 21.53GB/s       |
> > +------------------------------------------------------+
> > |multifd-4-threads  | 4143           | 24.13GB/s       |
> > +------------------------------------------------------+
>
> This "throughput" is slightly confusing; I was initially surprised to see a
> large throughput for idle guests.  IMHO the "total-time" would explain.
> Feel free to drop that column if there's a repost.
>
> Did you check why 4 channels mostly already reached the top line?  Is it
> because main thread is already spinning 100%?
>
> Thanks,
>
> --
> Peter Xu

Sure I will drop "throughput" to avoid confusion. In my testing, 1
multifd channel already makes the main thread spin at 100%. So the
total-time is the same across 1/2/4 multifd channels as long as zero
page is run on the main migration thread. Of course, this is based on
the fact that the network is not the bottleneck. One interesting
finding is that multifd 1 channel with multifd zero page has better
performance than multifd 1 channel with main migration thread.
>

Re: [External] Re: [PATCH 0/6] Introduce multifd zero page checking.

[Peter Xu]

On Wed, Feb 07, 2024 at 04:47:27PM -0800, Hao Xiang wrote:
> Sure I will drop "throughput" to avoid confusion. In my testing, 1
> multifd channel already makes the main thread spin at 100%. So the
> total-time is the same across 1/2/4 multifd channels as long as zero
> page is run on the main migration thread. Of course, this is based on
> the fact that the network is not the bottleneck. One interesting
> finding is that multifd 1 channel with multifd zero page has better
> performance than multifd 1 channel with main migration thread.

It's probably because the main thread has even more works to do than
"detecting zero page" alone.

When zero detection is done in main thread and when the guest is fully
idle, it'll consume a major portion of main thread cpu resource scanning
those pages already.  Consider all pages zero, multifd threads should be
fully idle, so n_channels may not matter here.

When 1 multifd thread created with zero-page offloading, zero page is fully
offloaded from main -> multifd thread even if only one.  It's kind of a
similar effect of forking the main thread into two threads, so the main
thread can be more efficient on other tasks (fetching/scanning dirty bits,
etc.).

Thanks,

-- 
Peter Xu