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 |  1 +
 migration/options.c |  2 +-
 migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index fbb40ea10b..ef5dad1019 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"
diff --git a/migration/options.c b/migration/options.c
index 3c603391b0..3c79b6ccd4 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -181,7 +181,7 @@ Property migration_properties[] = {
                       MIG_MODE_NORMAL),
     DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
                        parameters.zero_page_detection,
-                       ZERO_PAGE_DETECTION_LEGACY),
+                       ZERO_PAGE_DETECTION_MULTIFD),
 
     /* Migration capabilities */
     DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
diff --git a/migration/ram.c b/migration/ram.c
index 5ece9f042e..b088c5a98c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
     QEMUFile *file = pss->pss_channel;
     int len = 0;
 
-    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
-        return 0;
-    }
-
     if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
         return 0;
     }
@@ -1256,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)) {
         return -1;
     }
@@ -2046,7 +2046,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;
 
@@ -2062,17 +2061,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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
+        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 */
@@ -2984,7 +3006,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

Hao Xiang <hao.xiang@bytedance.com> writes:

> This change adds a dedicated handler for MigrationOps::ram_save_target_page in

nit: Add a dedicated handler...

Usually "this patch/change" is used only when necessary to avoid
ambiguity.

> 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 |  1 +
>  migration/options.c |  2 +-
>  migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
>  3 files changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index fbb40ea10b..ef5dad1019 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/cutils.h"

This include...

>  #include "qemu/rcu.h"
> +#include "qemu/cutils.h"

is there already.

>  #include "exec/target_page.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/ramblock.h"
> diff --git a/migration/options.c b/migration/options.c
> index 3c603391b0..3c79b6ccd4 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -181,7 +181,7 @@ Property migration_properties[] = {
>                        MIG_MODE_NORMAL),
>      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
>                         parameters.zero_page_detection,
> -                       ZERO_PAGE_DETECTION_LEGACY),
> +                       ZERO_PAGE_DETECTION_MULTIFD),

I think we'll need something to avoid a 9.0 -> 8.2 migration with this
enabled. Otherwise it will go along happily until we get data corruption
because the new QEMU didn't send any zero pages on the migration thread
and the old QEMU did not look for them in the multifd packet.

Perhaps bumping the MULTIFD_VERSION when ZERO_PAGE_DETECTION_MULTIFD is
in use. We'd just need to fix the test in the new QEMU to check
(msg.version > MULTIFD_VERSION) instead of (msg.version != MULTIFD_VERSION).

>  
>      /* Migration capabilities */
>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> diff --git a/migration/ram.c b/migration/ram.c
> index 5ece9f042e..b088c5a98c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
>      QEMUFile *file = pss->pss_channel;
>      int len = 0;
>  
> -    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
> -        return 0;
> -    }

How does 'none' work now?

> -
>      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>          return 0;
>      }
> @@ -1256,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());

Drop these, please. Keep only the asserts that are likely to trigger
during development, such as the existing ones at multifd_send_pages.

> +
>      if (!multifd_queue_page(block, offset)) {
>          return -1;
>      }
> @@ -2046,7 +2046,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;
>  
> @@ -2062,17 +2061,40 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>          return 1;
>      }
>  
> +    return ram_save_page(rs, pss);

Look at where git put this! Are you using the default diff algorithm? If
not try using --patience to see if it improves the diff.

> +}
> +
> +/**
> + * ram_save_target_page_multifd: save one target page
> + *
> + * Returns the number of pages written

We could be more precise here:

 ram_save_target_page_multifd: send one target page to multifd workers
 
 Returns 1 if the page was queued, -1 otherwise.

> + *
> + * @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());

This should already be enforced at options.c.

> +
> +    /* Multifd is not compabible with postcopy. */
> +    assert(!migration_in_postcopy());

Same here.

> +
>      /*
> -     * 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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
> +        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 */
> @@ -2984,7 +3006,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();

On Wed, Feb 21, 2024 at 06:06:19PM -0300, Fabiano Rosas wrote:
> Hao Xiang <hao.xiang@bytedance.com> writes:
> 
> > This change adds a dedicated handler for MigrationOps::ram_save_target_page in
> 
> nit: Add a dedicated handler...
> 
> Usually "this patch/change" is used only when necessary to avoid
> ambiguity.
> 
> > 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 |  1 +
> >  migration/options.c |  2 +-
> >  migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
> >  3 files changed, 42 insertions(+), 14 deletions(-)
> >
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index fbb40ea10b..ef5dad1019 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -13,6 +13,7 @@
> >  #include "qemu/osdep.h"
> >  #include "qemu/cutils.h"
> 
> This include...
> 
> >  #include "qemu/rcu.h"
> > +#include "qemu/cutils.h"
> 
> is there already.
> 
> >  #include "exec/target_page.h"
> >  #include "sysemu/sysemu.h"
> >  #include "exec/ramblock.h"
> > diff --git a/migration/options.c b/migration/options.c
> > index 3c603391b0..3c79b6ccd4 100644
> > --- a/migration/options.c
> > +++ b/migration/options.c
> > @@ -181,7 +181,7 @@ Property migration_properties[] = {
> >                        MIG_MODE_NORMAL),
> >      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
> >                         parameters.zero_page_detection,
> > -                       ZERO_PAGE_DETECTION_LEGACY),
> > +                       ZERO_PAGE_DETECTION_MULTIFD),
> 
> I think we'll need something to avoid a 9.0 -> 8.2 migration with this
> enabled. Otherwise it will go along happily until we get data corruption
> because the new QEMU didn't send any zero pages on the migration thread
> and the old QEMU did not look for them in the multifd packet.

It could be even worse, as the new QEMU will only attach "normal" pages
after the multifd packet, the old QEMU could read more than it could,
expecting all pages..

> 
> Perhaps bumping the MULTIFD_VERSION when ZERO_PAGE_DETECTION_MULTIFD is
> in use. We'd just need to fix the test in the new QEMU to check
> (msg.version > MULTIFD_VERSION) instead of (msg.version != MULTIFD_VERSION).

IMHO we don't need yet to change MULTIFD_VERSION, what we need is perhaps a
compat entry in hw_compat_8_2 setting "zero-page-detection" to "legacy".
We should make sure when "legacy" is set, multifd ran the old protocol
(zero_num will always be 0, and will be ignored by old QEMUs, IIUC).

One more comment is, when repost please consider split this patch into two;
The new ram_save_target_page_multifd() hook can be done in another patch,
AFAIU.

> 
> >  
> >      /* Migration capabilities */
> >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 5ece9f042e..b088c5a98c 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
> >      QEMUFile *file = pss->pss_channel;
> >      int len = 0;
> >  
> > -    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
> > -        return 0;
> > -    }
> 
> How does 'none' work now?
> 
> > -
> >      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> >          return 0;
> >      }
> > @@ -1256,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());
> 
> Drop these, please. Keep only the asserts that are likely to trigger
> during development, such as the existing ones at multifd_send_pages.
> 
> > +
> >      if (!multifd_queue_page(block, offset)) {
> >          return -1;
> >      }
> > @@ -2046,7 +2046,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;
> >  
> > @@ -2062,17 +2061,40 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
> >          return 1;
> >      }
> >  
> > +    return ram_save_page(rs, pss);
> 
> Look at where git put this! Are you using the default diff algorithm? If
> not try using --patience to see if it improves the diff.
> 
> > +}
> > +
> > +/**
> > + * ram_save_target_page_multifd: save one target page
> > + *
> > + * Returns the number of pages written
> 
> We could be more precise here:
> 
>  ram_save_target_page_multifd: send one target page to multifd workers
>  
>  Returns 1 if the page was queued, -1 otherwise.
> 
> > + *
> > + * @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());
> 
> This should already be enforced at options.c.
> 
> > +
> > +    /* Multifd is not compabible with postcopy. */
> > +    assert(!migration_in_postcopy());
> 
> Same here.
> 
> > +
> >      /*
> > -     * 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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
> > +        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 */
> > @@ -2984,7 +3006,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();
> 

-- 
Peter Xu

On Thu, Feb 22, 2024 at 6:33 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Feb 21, 2024 at 06:06:19PM -0300, Fabiano Rosas wrote:
> > Hao Xiang <hao.xiang@bytedance.com> writes:
> >
> > > This change adds a dedicated handler for MigrationOps::ram_save_target_page in
> >
> > nit: Add a dedicated handler...
> >
> > Usually "this patch/change" is used only when necessary to avoid
> > ambiguity.
> >
> > > 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 |  1 +
> > >  migration/options.c |  2 +-
> > >  migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
> > >  3 files changed, 42 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/migration/multifd.c b/migration/multifd.c
> > > index fbb40ea10b..ef5dad1019 100644
> > > --- a/migration/multifd.c
> > > +++ b/migration/multifd.c
> > > @@ -13,6 +13,7 @@
> > >  #include "qemu/osdep.h"
> > >  #include "qemu/cutils.h"
> >
> > This include...
> >
> > >  #include "qemu/rcu.h"
> > > +#include "qemu/cutils.h"
> >
> > is there already.
> >
> > >  #include "exec/target_page.h"
> > >  #include "sysemu/sysemu.h"
> > >  #include "exec/ramblock.h"
> > > diff --git a/migration/options.c b/migration/options.c
> > > index 3c603391b0..3c79b6ccd4 100644
> > > --- a/migration/options.c
> > > +++ b/migration/options.c
> > > @@ -181,7 +181,7 @@ Property migration_properties[] = {
> > >                        MIG_MODE_NORMAL),
> > >      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
> > >                         parameters.zero_page_detection,
> > > -                       ZERO_PAGE_DETECTION_LEGACY),
> > > +                       ZERO_PAGE_DETECTION_MULTIFD),
> >
> > I think we'll need something to avoid a 9.0 -> 8.2 migration with this
> > enabled. Otherwise it will go along happily until we get data corruption
> > because the new QEMU didn't send any zero pages on the migration thread
> > and the old QEMU did not look for them in the multifd packet.
>
> It could be even worse, as the new QEMU will only attach "normal" pages
> after the multifd packet, the old QEMU could read more than it could,
> expecting all pages..
>
> >
> > Perhaps bumping the MULTIFD_VERSION when ZERO_PAGE_DETECTION_MULTIFD is
> > in use. We'd just need to fix the test in the new QEMU to check
> > (msg.version > MULTIFD_VERSION) instead of (msg.version != MULTIFD_VERSION).
>
> IMHO we don't need yet to change MULTIFD_VERSION, what we need is perhaps a
> compat entry in hw_compat_8_2 setting "zero-page-detection" to "legacy".
> We should make sure when "legacy" is set, multifd ran the old protocol
> (zero_num will always be 0, and will be ignored by old QEMUs, IIUC).
>
> One more comment is, when repost please consider split this patch into two;
> The new ram_save_target_page_multifd() hook can be done in another patch,
> AFAIU.

Sorry, I kept missing this. I will keep telling myself, compatibility
is king. I will set the hw_compat_8_2 setting and make sure to test
migration 9.0 -> 8.2 fails with "multifd" option set.
Will split patches.

>
> >
> > >
> > >      /* Migration capabilities */
> > >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 5ece9f042e..b088c5a98c 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
> > >      QEMUFile *file = pss->pss_channel;
> > >      int len = 0;
> > >
> > > -    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
> > > -        return 0;
> > > -    }
> >
> > How does 'none' work now?
> >
> > > -
> > >      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> > >          return 0;
> > >      }
> > > @@ -1256,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());
> >
> > Drop these, please. Keep only the asserts that are likely to trigger
> > during development, such as the existing ones at multifd_send_pages.
> >
> > > +
> > >      if (!multifd_queue_page(block, offset)) {
> > >          return -1;
> > >      }
> > > @@ -2046,7 +2046,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;
> > >
> > > @@ -2062,17 +2061,40 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
> > >          return 1;
> > >      }
> > >
> > > +    return ram_save_page(rs, pss);
> >
> > Look at where git put this! Are you using the default diff algorithm? If
> > not try using --patience to see if it improves the diff.
> >
> > > +}
> > > +
> > > +/**
> > > + * ram_save_target_page_multifd: save one target page
> > > + *
> > > + * Returns the number of pages written
> >
> > We could be more precise here:
> >
> >  ram_save_target_page_multifd: send one target page to multifd workers
> >
> >  Returns 1 if the page was queued, -1 otherwise.
> >
> > > + *
> > > + * @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());
> >
> > This should already be enforced at options.c.
> >
> > > +
> > > +    /* Multifd is not compabible with postcopy. */
> > > +    assert(!migration_in_postcopy());
> >
> > Same here.
> >
> > > +
> > >      /*
> > > -     * 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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
> > > +        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 */
> > > @@ -2984,7 +3006,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();
> >
>
> --
> Peter Xu
>

On Thu, Feb 22, 2024 at 10:02 PM Hao Xiang <hao.xiang@bytedance.com> wrote:
>
> On Thu, Feb 22, 2024 at 6:33 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Wed, Feb 21, 2024 at 06:06:19PM -0300, Fabiano Rosas wrote:
> > > Hao Xiang <hao.xiang@bytedance.com> writes:
> > >
> > > > This change adds a dedicated handler for MigrationOps::ram_save_target_page in
> > >
> > > nit: Add a dedicated handler...
> > >
> > > Usually "this patch/change" is used only when necessary to avoid
> > > ambiguity.
> > >
> > > > 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 |  1 +
> > > >  migration/options.c |  2 +-
> > > >  migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
> > > >  3 files changed, 42 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/migration/multifd.c b/migration/multifd.c
> > > > index fbb40ea10b..ef5dad1019 100644
> > > > --- a/migration/multifd.c
> > > > +++ b/migration/multifd.c
> > > > @@ -13,6 +13,7 @@
> > > >  #include "qemu/osdep.h"
> > > >  #include "qemu/cutils.h"
> > >
> > > This include...
> > >
> > > >  #include "qemu/rcu.h"
> > > > +#include "qemu/cutils.h"
> > >
> > > is there already.
> > >
> > > >  #include "exec/target_page.h"
> > > >  #include "sysemu/sysemu.h"
> > > >  #include "exec/ramblock.h"
> > > > diff --git a/migration/options.c b/migration/options.c
> > > > index 3c603391b0..3c79b6ccd4 100644
> > > > --- a/migration/options.c
> > > > +++ b/migration/options.c
> > > > @@ -181,7 +181,7 @@ Property migration_properties[] = {
> > > >                        MIG_MODE_NORMAL),
> > > >      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
> > > >                         parameters.zero_page_detection,
> > > > -                       ZERO_PAGE_DETECTION_LEGACY),
> > > > +                       ZERO_PAGE_DETECTION_MULTIFD),
> > >
> > > I think we'll need something to avoid a 9.0 -> 8.2 migration with this
> > > enabled. Otherwise it will go along happily until we get data corruption
> > > because the new QEMU didn't send any zero pages on the migration thread
> > > and the old QEMU did not look for them in the multifd packet.
> >
> > It could be even worse, as the new QEMU will only attach "normal" pages
> > after the multifd packet, the old QEMU could read more than it could,
> > expecting all pages..
> >
> > >
> > > Perhaps bumping the MULTIFD_VERSION when ZERO_PAGE_DETECTION_MULTIFD is
> > > in use. We'd just need to fix the test in the new QEMU to check
> > > (msg.version > MULTIFD_VERSION) instead of (msg.version != MULTIFD_VERSION).
> >
> > IMHO we don't need yet to change MULTIFD_VERSION, what we need is perhaps a
> > compat entry in hw_compat_8_2 setting "zero-page-detection" to "legacy".
> > We should make sure when "legacy" is set, multifd ran the old protocol
> > (zero_num will always be 0, and will be ignored by old QEMUs, IIUC).
> >
> > One more comment is, when repost please consider split this patch into two;
> > The new ram_save_target_page_multifd() hook can be done in another patch,
> > AFAIU.
>
> Sorry, I kept missing this. I will keep telling myself, compatibility
> is king. I will set the hw_compat_8_2 setting and make sure to test
> migration 9.0 -> 8.2 fails with "multifd" option set.
> Will split patches.

So I just want to make sure I am coding the right solution. I added
setting "zero-page-detection" to "legacy" in hw_compat_8_2 and tested
it. The behavior is that if I set machine type to pc-q35-8.2,
zero-page-detection will automatically be set to "legacy". But if I
set the machine type to pc-q35-9.0, zero-page-detection will be the
default value "multifd". However, this doesn't seem to be a hard
requirement because I can still override zero-page-detection to
multifd on machine type pc-q35-8.2. Is this OK?

>
> >
> > >
> > > >
> > > >      /* Migration capabilities */
> > > >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > index 5ece9f042e..b088c5a98c 100644
> > > > --- a/migration/ram.c
> > > > +++ b/migration/ram.c
> > > > @@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
> > > >      QEMUFile *file = pss->pss_channel;
> > > >      int len = 0;
> > > >
> > > > -    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
> > > > -        return 0;
> > > > -    }
> > >
> > > How does 'none' work now?
> > >
> > > > -
> > > >      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> > > >          return 0;
> > > >      }
> > > > @@ -1256,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());
> > >
> > > Drop these, please. Keep only the asserts that are likely to trigger
> > > during development, such as the existing ones at multifd_send_pages.
> > >
> > > > +
> > > >      if (!multifd_queue_page(block, offset)) {
> > > >          return -1;
> > > >      }
> > > > @@ -2046,7 +2046,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;
> > > >
> > > > @@ -2062,17 +2061,40 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
> > > >          return 1;
> > > >      }
> > > >
> > > > +    return ram_save_page(rs, pss);
> > >
> > > Look at where git put this! Are you using the default diff algorithm? If
> > > not try using --patience to see if it improves the diff.
> > >
> > > > +}
> > > > +
> > > > +/**
> > > > + * ram_save_target_page_multifd: save one target page
> > > > + *
> > > > + * Returns the number of pages written
> > >
> > > We could be more precise here:
> > >
> > >  ram_save_target_page_multifd: send one target page to multifd workers
> > >
> > >  Returns 1 if the page was queued, -1 otherwise.
> > >
> > > > + *
> > > > + * @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());
> > >
> > > This should already be enforced at options.c.
> > >
> > > > +
> > > > +    /* Multifd is not compabible with postcopy. */
> > > > +    assert(!migration_in_postcopy());
> > >
> > > Same here.
> > >
> > > > +
> > > >      /*
> > > > -     * 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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
> > > > +        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 */
> > > > @@ -2984,7 +3006,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();
> > >
> >
> > --
> > Peter Xu
> >

On Sat, Feb 24, 2024 at 03:03:15PM -0800, Hao Xiang wrote:
> So I just want to make sure I am coding the right solution. I added
> setting "zero-page-detection" to "legacy" in hw_compat_8_2 and tested
> it. The behavior is that if I set machine type to pc-q35-8.2,
> zero-page-detection will automatically be set to "legacy". But if I
> set the machine type to pc-q35-9.0, zero-page-detection will be the
> default value "multifd". However, this doesn't seem to be a hard
> requirement because I can still override zero-page-detection to
> multifd on machine type pc-q35-8.2. Is this OK?

What we want to guarantee is old 8.2 users can smoothly migrate to the new
qemus, and existing 8.2 (or prior) users definitely don't have any override
over the new parameter zero-page-detection simply because 8.2 (or prior)
binary doesn't support it yet.

Then, if someone is using new binary with 8.2 machine types, meanwhile
override this default value, it means it's the user's choice of doing this,
and the user should guarantee all the qemus he/she manages also keeps this
parameter override to make sure migration will work between these qemu
processes.

So in short, that's all fine.

Thanks,

-- 
Peter Xu

On Wed, Feb 21, 2024 at 1:06 PM Fabiano Rosas <farosas@suse.de> wrote:
>
> Hao Xiang <hao.xiang@bytedance.com> writes:
>
> > This change adds a dedicated handler for MigrationOps::ram_save_target_page in
>
> nit: Add a dedicated handler...
>
> Usually "this patch/change" is used only when necessary to avoid
> ambiguity.

Will do.

>
> > 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 |  1 +
> >  migration/options.c |  2 +-
> >  migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
> >  3 files changed, 42 insertions(+), 14 deletions(-)
> >
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index fbb40ea10b..ef5dad1019 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -13,6 +13,7 @@
> >  #include "qemu/osdep.h"
> >  #include "qemu/cutils.h"
>
> This include...
>
> >  #include "qemu/rcu.h"
> > +#include "qemu/cutils.h"
>
> is there already.
>
> >  #include "exec/target_page.h"
> >  #include "sysemu/sysemu.h"
> >  #include "exec/ramblock.h"
> > diff --git a/migration/options.c b/migration/options.c
> > index 3c603391b0..3c79b6ccd4 100644
> > --- a/migration/options.c
> > +++ b/migration/options.c
> > @@ -181,7 +181,7 @@ Property migration_properties[] = {
> >                        MIG_MODE_NORMAL),
> >      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
> >                         parameters.zero_page_detection,
> > -                       ZERO_PAGE_DETECTION_LEGACY),
> > +                       ZERO_PAGE_DETECTION_MULTIFD),
>
> I think we'll need something to avoid a 9.0 -> 8.2 migration with this
> enabled. Otherwise it will go along happily until we get data corruption
> because the new QEMU didn't send any zero pages on the migration thread
> and the old QEMU did not look for them in the multifd packet.
>
> Perhaps bumping the MULTIFD_VERSION when ZERO_PAGE_DETECTION_MULTIFD is
> in use. We'd just need to fix the test in the new QEMU to check
> (msg.version > MULTIFD_VERSION) instead of (msg.version != MULTIFD_VERSION).
>
> >
> >      /* Migration capabilities */
> >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 5ece9f042e..b088c5a98c 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
> >      QEMUFile *file = pss->pss_channel;
> >      int len = 0;
> >
> > -    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
> > -        return 0;
> > -    }
>
> How does 'none' work now?

I tested it and all pages are transferred with payload (including the
zero pages).

>
> > -
> >      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> >          return 0;
> >      }
> > @@ -1256,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());
>
> Drop these, please. Keep only the asserts that are likely to trigger
> during development, such as the existing ones at multifd_send_pages.

I think I have got enough feedback regarding too many asserts. I will
drop these. assert is not compiled into the free build, correct?

>
> > +
> >      if (!multifd_queue_page(block, offset)) {
> >          return -1;
> >      }
> > @@ -2046,7 +2046,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;
> >
> > @@ -2062,17 +2061,40 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
> >          return 1;
> >      }
> >
> > +    return ram_save_page(rs, pss);
>
> Look at where git put this! Are you using the default diff algorithm? If
> not try using --patience to see if it improves the diff.

I used the default diff algorithm.

>
> > +}
> > +
> > +/**
> > + * ram_save_target_page_multifd: save one target page
> > + *
> > + * Returns the number of pages written
>
> We could be more precise here:
>
>  ram_save_target_page_multifd: send one target page to multifd workers
>
>  Returns 1 if the page was queued, -1 otherwise.

Will do.

>
> > + *
> > + * @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());
>
> This should already be enforced at options.c.
>
> > +
> > +    /* Multifd is not compabible with postcopy. */
> > +    assert(!migration_in_postcopy());
>
> Same here.
>
> > +
> >      /*
> > -     * 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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
> > +        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 */
> > @@ -2984,7 +3006,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();

Hao Xiang <hao.xiang@bytedance.com> writes:

> On Wed, Feb 21, 2024 at 1:06 PM Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Hao Xiang <hao.xiang@bytedance.com> writes:
>>
>> > This change adds a dedicated handler for MigrationOps::ram_save_target_page in
>>
>> nit: Add a dedicated handler...
>>
>> Usually "this patch/change" is used only when necessary to avoid
>> ambiguity.
>
> Will do.
>
>>
>> > 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 |  1 +
>> >  migration/options.c |  2 +-
>> >  migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
>> >  3 files changed, 42 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/migration/multifd.c b/migration/multifd.c
>> > index fbb40ea10b..ef5dad1019 100644
>> > --- a/migration/multifd.c
>> > +++ b/migration/multifd.c
>> > @@ -13,6 +13,7 @@
>> >  #include "qemu/osdep.h"
>> >  #include "qemu/cutils.h"
>>
>> This include...
>>
>> >  #include "qemu/rcu.h"
>> > +#include "qemu/cutils.h"
>>
>> is there already.
>>
>> >  #include "exec/target_page.h"
>> >  #include "sysemu/sysemu.h"
>> >  #include "exec/ramblock.h"
>> > diff --git a/migration/options.c b/migration/options.c
>> > index 3c603391b0..3c79b6ccd4 100644
>> > --- a/migration/options.c
>> > +++ b/migration/options.c
>> > @@ -181,7 +181,7 @@ Property migration_properties[] = {
>> >                        MIG_MODE_NORMAL),
>> >      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
>> >                         parameters.zero_page_detection,
>> > -                       ZERO_PAGE_DETECTION_LEGACY),
>> > +                       ZERO_PAGE_DETECTION_MULTIFD),
>>
>> I think we'll need something to avoid a 9.0 -> 8.2 migration with this
>> enabled. Otherwise it will go along happily until we get data corruption
>> because the new QEMU didn't send any zero pages on the migration thread
>> and the old QEMU did not look for them in the multifd packet.
>>
>> Perhaps bumping the MULTIFD_VERSION when ZERO_PAGE_DETECTION_MULTIFD is
>> in use. We'd just need to fix the test in the new QEMU to check
>> (msg.version > MULTIFD_VERSION) instead of (msg.version != MULTIFD_VERSION).
>>
>> >
>> >      /* Migration capabilities */
>> >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>> > diff --git a/migration/ram.c b/migration/ram.c
>> > index 5ece9f042e..b088c5a98c 100644
>> > --- a/migration/ram.c
>> > +++ b/migration/ram.c
>> > @@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
>> >      QEMUFile *file = pss->pss_channel;
>> >      int len = 0;
>> >
>> > -    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
>> > -        return 0;
>> > -    }
>>
>> How does 'none' work now?
>
> I tested it and all pages are transferred with payload (including the
> zero pages).
>
>>
>> > -
>> >      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>> >          return 0;
>> >      }
>> > @@ -1256,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());
>>
>> Drop these, please. Keep only the asserts that are likely to trigger
>> during development, such as the existing ones at multifd_send_pages.
>
> I think I have got enough feedback regarding too many asserts. I will
> drop these. assert is not compiled into the free build, correct?
>

From include/qemu/osdep.h:

  /*
   * We have a lot of unaudited code that may fail in strange ways, or
   * even be a security risk during migration, if you disable assertions
   * at compile-time.  You may comment out these safety checks if you
   * absolutely want to disable assertion overhead, but it is not
   * supported upstream so the risk is all yours.  Meanwhile, please
   * submit patches to remove any side-effects inside an assertion, or
   * fixing error handling that should use Error instead of assert.
   */
  #ifdef NDEBUG
  #error building with NDEBUG is not supported
  #endif
  #ifdef G_DISABLE_ASSERT
  #error building with G_DISABLE_ASSERT is not supported
  #endif

>>
>> > +
>> >      if (!multifd_queue_page(block, offset)) {
>> >          return -1;
>> >      }
>> > @@ -2046,7 +2046,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;
>> >
>> > @@ -2062,17 +2061,40 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>> >          return 1;
>> >      }
>> >
>> > +    return ram_save_page(rs, pss);
>>
>> Look at where git put this! Are you using the default diff algorithm? If
>> not try using --patience to see if it improves the diff.
>
> I used the default diff algorithm.
>
>>
>> > +}
>> > +
>> > +/**
>> > + * ram_save_target_page_multifd: save one target page
>> > + *
>> > + * Returns the number of pages written
>>
>> We could be more precise here:
>>
>>  ram_save_target_page_multifd: send one target page to multifd workers
>>
>>  Returns 1 if the page was queued, -1 otherwise.
>
> Will do.
>
>>
>> > + *
>> > + * @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());
>>
>> This should already be enforced at options.c.
>>
>> > +
>> > +    /* Multifd is not compabible with postcopy. */
>> > +    assert(!migration_in_postcopy());
>>
>> Same here.
>>
>> > +
>> >      /*
>> > -     * 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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
>> > +        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 */
>> > @@ -2984,7 +3006,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();

On Fri, Feb 16, 2024 at 2:42 PM Hao Xiang <hao.xiang@bytedance.com> wrote:

> 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 |  1 +
>  migration/options.c |  2 +-
>  migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
>  3 files changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index fbb40ea10b..ef5dad1019 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"
> diff --git a/migration/options.c b/migration/options.c
> index 3c603391b0..3c79b6ccd4 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -181,7 +181,7 @@ Property migration_properties[] = {
>                        MIG_MODE_NORMAL),
>      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
>                         parameters.zero_page_detection,
> -                       ZERO_PAGE_DETECTION_LEGACY),
> +                       ZERO_PAGE_DETECTION_MULTIFD),
>
>      /* Migration capabilities */
>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> diff --git a/migration/ram.c b/migration/ram.c
> index 5ece9f042e..b088c5a98c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs,
> PageSearchStatus *pss,
>      QEMUFile *file = pss->pss_channel;
>      int len = 0;
>
> -    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
> -        return 0;
> -    }
> -
>      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>          return 0;
>      }
> @@ -1256,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());
>
We only call ram_save_multifd_page() if:
 if (migrate_multifd()) {
        migration_ops->ram_save_target_page = ram_save_target_page_multifd;
So this assert is not needed.

+    assert(!migrate_compress());
>
+    assert(!migration_in_postcopy());
>
These two are redundant and done before we call in here.

+
>      if (!multifd_queue_page(block, offset)) {
>          return -1;
>      }
> @@ -2046,7 +2046,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;
>
> @@ -2062,17 +2061,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());
>
Do we need to check this for every page?


> +    /* 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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
> +        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 */
> @@ -2984,7 +3006,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
>
>
>

-- 
Elena

On Wed, Feb 21, 2024 at 8:11 AM Elena Ufimtseva <ufimtseva@gmail.com> wrote:
>
>
>
> On Fri, Feb 16, 2024 at 2:42 PM Hao Xiang <hao.xiang@bytedance.com> wrote:
>>
>> 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 |  1 +
>>  migration/options.c |  2 +-
>>  migration/ram.c     | 53 ++++++++++++++++++++++++++++++++++-----------
>>  3 files changed, 42 insertions(+), 14 deletions(-)
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index fbb40ea10b..ef5dad1019 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"
>> diff --git a/migration/options.c b/migration/options.c
>> index 3c603391b0..3c79b6ccd4 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -181,7 +181,7 @@ Property migration_properties[] = {
>>                        MIG_MODE_NORMAL),
>>      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
>>                         parameters.zero_page_detection,
>> -                       ZERO_PAGE_DETECTION_LEGACY),
>> +                       ZERO_PAGE_DETECTION_MULTIFD),
>>
>>      /* Migration capabilities */
>>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 5ece9f042e..b088c5a98c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1123,10 +1123,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
>>      QEMUFile *file = pss->pss_channel;
>>      int len = 0;
>>
>> -    if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
>> -        return 0;
>> -    }
>> -
>>      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>>          return 0;
>>      }
>> @@ -1256,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());
>
> We only call ram_save_multifd_page() if:
>  if (migrate_multifd()) {
>         migration_ops->ram_save_target_page = ram_save_target_page_multifd;
> So this assert is not needed.

The point of an assert is to ensure the current function is called
with the correct assumptions. In the future, if someone moves this
function to a different place, we can catch the potential issues.

>
>> +    assert(!migrate_compress());
>>
>> +    assert(!migration_in_postcopy());
>
> These two are redundant and done before we call in here.
>
>> +
>>      if (!multifd_queue_page(block, offset)) {
>>          return -1;
>>      }
>> @@ -2046,7 +2046,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;
>>
>> @@ -2062,17 +2061,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());
>
> Do we need to check this for every page?
>
>>
>> +    /* 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_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
>> +        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 */
>> @@ -2984,7 +3006,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
>>
>>
>
>
> --
> Elena