All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] migration/postcopy: enable compress during postcopy
@ 2019-10-18  0:48 Wei Yang
  2019-10-18  0:48 ` [PATCH 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size Wei Yang
                   ` (7 more replies)
  0 siblings, 8 replies; 22+ messages in thread
From: Wei Yang @ 2019-10-18  0:48 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

This patch set tries enable compress during postcopy.

postcopy requires to place a whole host page, while migration thread migrate
memory in target page size. This makes postcopy need to collect all target
pages in one host page before placing via userfaultfd.

To enable compress during postcopy, there are two problems to solve:

    1. Random order for target page arrival
    2. Target pages in one host page arrives without interrupt by target
       page from other host page

The first one is handled by counting the number of target pages arrived
instead of the last target page arrived.

The second one is handled by:

    1. Flush compress thread for each host page
    2. Wait for decompress thread for before placing host page

With the combination of these two changes, compress is enabled during
postcopy.

Wei Yang (6):
  migration/postcopy: reduce memset when it is zero page and
    matches_target_page_size
  migration/postcopy: wait for decompress thread in precopy
  migration/postcopy: count target page number to decide the
    place_needed
  migration/postcopy: set all_zero to true on the first target page
  migration/postcopy: enable random order target page arrival
  migration/postcopy: enable compress during postcopy

 migration/migration.c | 11 --------
 migration/ram.c       | 65 ++++++++++++++++++++++++++++++-------------
 2 files changed, 45 insertions(+), 31 deletions(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size
  2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
@ 2019-10-18  0:48 ` Wei Yang
  2019-11-06 18:18   ` Dr. David Alan Gilbert
  2019-10-18  0:48 ` [PATCH 2/6] migration/postcopy: wait for decompress thread in precopy Wei Yang
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-10-18  0:48 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

In this case, page_buffer content would not be used.

Skip this to save some time.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 99a98b2da4..7938a643d9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4091,7 +4091,13 @@ static int ram_load_postcopy(QEMUFile *f)
         switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
         case RAM_SAVE_FLAG_ZERO:
             ch = qemu_get_byte(f);
-            memset(page_buffer, ch, TARGET_PAGE_SIZE);
+            /*
+             * Can skip to set page_buffer when
+             * this is a zero page and (block->page_size == TARGET_PAGE_SIZE).
+             */
+            if (ch || !matches_target_page_size) {
+                memset(page_buffer, ch, TARGET_PAGE_SIZE);
+            }
             if (ch) {
                 all_zero = false;
             }
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/6] migration/postcopy: wait for decompress thread in precopy
  2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
  2019-10-18  0:48 ` [PATCH 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size Wei Yang
@ 2019-10-18  0:48 ` Wei Yang
  2019-11-06 19:57   ` Dr. David Alan Gilbert
  2019-10-18  0:48 ` [PATCH 3/6] migration/postcopy: count target page number to decide the place_needed Wei Yang
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-10-18  0:48 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

Compress is not supported with postcopy, it is safe to wait for
decompress thread just in precopy.

This is a preparation for later patch.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 7938a643d9..f59e3fe197 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4375,6 +4375,7 @@ static int ram_load_precopy(QEMUFile *f)
         }
     }
 
+    ret |= wait_for_decompress_done();
     return ret;
 }
 
@@ -4406,8 +4407,6 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
         } else {
             ret = ram_load_precopy(f);
         }
-
-        ret |= wait_for_decompress_done();
     }
     trace_ram_load_complete(ret, seq_iter);
 
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 3/6] migration/postcopy: count target page number to decide the place_needed
  2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
  2019-10-18  0:48 ` [PATCH 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size Wei Yang
  2019-10-18  0:48 ` [PATCH 2/6] migration/postcopy: wait for decompress thread in precopy Wei Yang
@ 2019-10-18  0:48 ` Wei Yang
  2019-11-06 19:59   ` Dr. David Alan Gilbert
  2019-10-18  0:48 ` [PATCH 4/6] migration/postcopy: set all_zero to true on the first target page Wei Yang
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-10-18  0:48 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

In postcopy, it requires to place whole host page instead of target
page.

Currently, it relies on the page offset to decide whether this is the
last target page. We also can count the target page number during the
iteration. When the number of target page equals
(host page size / target page size), this means it is the last target
page in the host page.

This is a preparation for non-ordered target page transmission.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index f59e3fe197..5c05376d8d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4017,6 +4017,7 @@ static int ram_load_postcopy(QEMUFile *f)
     void *postcopy_host_page = mis->postcopy_tmp_page;
     void *last_host = NULL;
     bool all_zero = false;
+    int target_pages = 0;
 
     while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
         ram_addr_t addr;
@@ -4051,6 +4052,7 @@ static int ram_load_postcopy(QEMUFile *f)
                 ret = -EINVAL;
                 break;
             }
+            target_pages++;
             matches_target_page_size = block->page_size == TARGET_PAGE_SIZE;
             /*
              * Postcopy requires that we place whole host pages atomically;
@@ -4082,8 +4084,10 @@ static int ram_load_postcopy(QEMUFile *f)
              * If it's the last part of a host page then we place the host
              * page
              */
-            place_needed = (((uintptr_t)host + TARGET_PAGE_SIZE) &
-                                     (block->page_size - 1)) == 0;
+            if (target_pages == (block->page_size / TARGET_PAGE_SIZE)) {
+                place_needed = true;
+                target_pages = 0;
+            }
             place_source = postcopy_host_page;
         }
         last_host = host;
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 4/6] migration/postcopy: set all_zero to true on the first target page
  2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
                   ` (2 preceding siblings ...)
  2019-10-18  0:48 ` [PATCH 3/6] migration/postcopy: count target page number to decide the place_needed Wei Yang
@ 2019-10-18  0:48 ` Wei Yang
  2019-11-06 20:04   ` Dr. David Alan Gilbert
  2019-10-18  0:48 ` [PATCH 5/6] migration/postcopy: enable random order target page arrival Wei Yang
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-10-18  0:48 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

For the first target page, all_zero is set to true for this round check.

After target_pages introduced, we could leverage this variable instead
of checking the address offset.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 5c05376d8d..b5759793a9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4067,7 +4067,7 @@ static int ram_load_postcopy(QEMUFile *f)
             page_buffer = postcopy_host_page +
                           ((uintptr_t)host & (block->page_size - 1));
             /* If all TP are zero then we can optimise the place */
-            if (!((uintptr_t)host & (block->page_size - 1))) {
+            if (target_pages == 1) {
                 all_zero = true;
             } else {
                 /* not the 1st TP within the HP */
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 5/6] migration/postcopy: enable random order target page arrival
  2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
                   ` (3 preceding siblings ...)
  2019-10-18  0:48 ` [PATCH 4/6] migration/postcopy: set all_zero to true on the first target page Wei Yang
@ 2019-10-18  0:48 ` Wei Yang
  2019-11-06 20:08   ` Dr. David Alan Gilbert
  2019-10-18  0:48 ` [PATCH 6/6] migration/postcopy: enable compress during postcopy Wei Yang
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-10-18  0:48 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

After using number of target page received to track one host page, we
could have the capability to handle random order target page arrival in
one host page.

This is a preparation for enabling compress during postcopy.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index b5759793a9..da0596411c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4015,7 +4015,6 @@ static int ram_load_postcopy(QEMUFile *f)
     MigrationIncomingState *mis = migration_incoming_get_current();
     /* Temporary page that is later 'placed' */
     void *postcopy_host_page = mis->postcopy_tmp_page;
-    void *last_host = NULL;
     bool all_zero = false;
     int target_pages = 0;
 
@@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
              * that's moved into place later.
              * The migration protocol uses,  possibly smaller, target-pages
              * however the source ensures it always sends all the components
-             * of a host page in order.
+             * of a host page in one chunk.
              */
             page_buffer = postcopy_host_page +
                           ((uintptr_t)host & (block->page_size - 1));
             /* If all TP are zero then we can optimise the place */
             if (target_pages == 1) {
                 all_zero = true;
-            } else {
-                /* not the 1st TP within the HP */
-                if (host != (last_host + TARGET_PAGE_SIZE)) {
-                    error_report("Non-sequential target page %p/%p",
-                                  host, last_host);
-                    ret = -EINVAL;
-                    break;
-                }
             }
 
-
             /*
              * If it's the last part of a host page then we place the host
              * page
@@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
             }
             place_source = postcopy_host_page;
         }
-        last_host = host;
 
         switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
         case RAM_SAVE_FLAG_ZERO:
@@ -4143,7 +4132,8 @@ static int ram_load_postcopy(QEMUFile *f)
 
         if (!ret && place_needed) {
             /* This gets called at the last target page in the host page */
-            void *place_dest = host + TARGET_PAGE_SIZE - block->page_size;
+            void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
+                                                       block->page_size);
 
             if (all_zero) {
                 ret = postcopy_place_page_zero(mis, place_dest,
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 6/6] migration/postcopy: enable compress during postcopy
  2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
                   ` (4 preceding siblings ...)
  2019-10-18  0:48 ` [PATCH 5/6] migration/postcopy: enable random order target page arrival Wei Yang
@ 2019-10-18  0:48 ` Wei Yang
  2019-11-06 19:55   ` Dr. David Alan Gilbert
  2019-10-18 16:50 ` [PATCH 0/6] " no-reply
  2019-11-06 20:11 ` Dr. David Alan Gilbert
  7 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-10-18  0:48 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

postcopy requires to place a whole host page, while migration thread
migrate memory in target page size. This makes postcopy need to collect
all target pages in one host page before placing via userfaultfd.

To enable compress during postcopy, there are two problems to solve:

    1. Random order for target page arrival
    2. Target pages in one host page arrives without interrupt by target
       page from other host page

The first one is handled by previous cleanup patch.

This patch handles the second one by:

    1. Flush compress thread for each host page
    2. Wait for decompress thread for before placing host page

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/migration.c | 11 -----------
 migration/ram.c       | 28 +++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3febd0f8f3..72e53e2249 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1000,17 +1000,6 @@ static bool migrate_caps_check(bool *cap_list,
 #endif
 
     if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
-        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
-            /* The decompression threads asynchronously write into RAM
-             * rather than use the atomic copies needed to avoid
-             * userfaulting.  It should be possible to fix the decompression
-             * threads for compatibility in future.
-             */
-            error_setg(errp, "Postcopy is not currently compatible "
-                       "with compression");
-            return false;
-        }
-
         /* This check is reasonably expensive, so only when it's being
          * set the first time, also it's only the destination that needs
          * special support.
diff --git a/migration/ram.c b/migration/ram.c
index da0596411c..1403978d75 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3449,6 +3449,14 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 
             rs->target_page_count += pages;
 
+            /*
+             * During postcopy, it is necessary to make sure one whole host
+             * page is sent in one chunk.
+             */
+            if (migrate_postcopy_ram()) {
+                flush_compressed_data(rs);
+            }
+
             /*
              * we want to check in the 1st loop, just in case it was the 1st
              * time and we had to sync the dirty bitmap.
@@ -4025,6 +4033,7 @@ static int ram_load_postcopy(QEMUFile *f)
         void *place_source = NULL;
         RAMBlock *block = NULL;
         uint8_t ch;
+        int len;
 
         addr = qemu_get_be64(f);
 
@@ -4042,7 +4051,8 @@ static int ram_load_postcopy(QEMUFile *f)
 
         trace_ram_load_postcopy_loop((uint64_t)addr, flags);
         place_needed = false;
-        if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE)) {
+        if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
+                     RAM_SAVE_FLAG_COMPRESS_PAGE)) {
             block = ram_block_from_stream(f, flags);
 
             host = host_from_ram_block_offset(block, addr);
@@ -4114,6 +4124,17 @@ static int ram_load_postcopy(QEMUFile *f)
                                          TARGET_PAGE_SIZE);
             }
             break;
+        case RAM_SAVE_FLAG_COMPRESS_PAGE:
+            all_zero = false;
+            len = qemu_get_be32(f);
+            if (len < 0 || len > compressBound(TARGET_PAGE_SIZE)) {
+                error_report("Invalid compressed data length: %d", len);
+                ret = -EINVAL;
+                break;
+            }
+            decompress_data_with_multi_threads(f, page_buffer, len);
+            break;
+
         case RAM_SAVE_FLAG_EOS:
             /* normal exit */
             multifd_recv_sync_main();
@@ -4125,6 +4146,11 @@ static int ram_load_postcopy(QEMUFile *f)
             break;
         }
 
+        /* Got the whole host page, wait for decompress before placing. */
+        if (place_needed) {
+            ret |= wait_for_decompress_done();
+        }
+
         /* Detect for any possible file errors */
         if (!ret && qemu_file_get_error(f)) {
             ret = qemu_file_get_error(f);
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
  2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
                   ` (5 preceding siblings ...)
  2019-10-18  0:48 ` [PATCH 6/6] migration/postcopy: enable compress during postcopy Wei Yang
@ 2019-10-18 16:50 ` no-reply
  2019-10-19  0:15   ` Wei Yang
  2019-11-06 20:11 ` Dr. David Alan Gilbert
  7 siblings, 1 reply; 22+ messages in thread
From: no-reply @ 2019-10-18 16:50 UTC (permalink / raw)
  To: richardw.yang; +Cc: qemu-devel, richardw.yang, dgilbert, quintela

Patchew URL: https://patchew.org/QEMU/20191018004850.9888-1-richardw.yang@linux.intel.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/hw/timer/allwinner-a10-pit.o
In file included from /tmp/qemu-test/src/migration/ram.c:29:
/tmp/qemu-test/src/migration/ram.c: In function 'ram_load_postcopy':
/tmp/qemu-test/src/migration/ram.c:4177:56: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
             void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
                                                        ^
/tmp/qemu-test/src/include/qemu/osdep.h:268:33: note: in definition of macro 'QEMU_ALIGN_DOWN'
 #define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
                                 ^
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1
make[1]: *** Waiting for unfinished jobs....
  CC      x86_64-softmmu/target/i386/arch_dump.o
  CC      aarch64-softmmu/hw/usb/tusb6010.o
---
  CC      aarch64-softmmu/hw/arm/xlnx-zynqmp.o
In file included from /tmp/qemu-test/src/migration/ram.c:29:
/tmp/qemu-test/src/migration/ram.c: In function 'ram_load_postcopy':
/tmp/qemu-test/src/migration/ram.c:4177:56: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
             void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
                                                        ^
/tmp/qemu-test/src/include/qemu/osdep.h:268:33: note: in definition of macro 'QEMU_ALIGN_DOWN'
 #define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
                                 ^
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:482: aarch64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
make: *** [Makefile:482: x86_64-softmmu/all] Error 2
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=90570434880344249cff701baa188163', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-dh8p6f27/src/docker-src.2019-10-18-12.47.19.4164:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=90570434880344249cff701baa188163
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-dh8p6f27/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m45.691s
user    0m8.390s


The full log is available at
http://patchew.org/logs/20191018004850.9888-1-richardw.yang@linux.intel.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
  2019-10-18 16:50 ` [PATCH 0/6] " no-reply
@ 2019-10-19  0:15   ` Wei Yang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Yang @ 2019-10-19  0:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: richardw.yang, dgilbert, quintela

On Fri, Oct 18, 2019 at 09:50:05AM -0700, no-reply@patchew.org wrote:
>Patchew URL: https://patchew.org/QEMU/20191018004850.9888-1-richardw.yang@linux.intel.com/
>
>
>
>Hi,
>
>This series failed the docker-mingw@fedora build test. Please find the testing commands and
>their output below. If you have Docker installed, you can probably reproduce it
>locally.
>
>=== TEST SCRIPT BEGIN ===
>#! /bin/bash
>export ARCH=x86_64
>make docker-image-fedora V=1 NETWORK=1
>time make docker-test-mingw@fedora J=14 NETWORK=1
>=== TEST SCRIPT END ===
>
>  CC      aarch64-softmmu/hw/timer/allwinner-a10-pit.o
>In file included from /tmp/qemu-test/src/migration/ram.c:29:
>/tmp/qemu-test/src/migration/ram.c: In function 'ram_load_postcopy':
>/tmp/qemu-test/src/migration/ram.c:4177:56: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>             void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
>                                                        ^

Sounds should use uintptr_t.

Would change it in next version.

>/tmp/qemu-test/src/include/qemu/osdep.h:268:33: note: in definition of macro 'QEMU_ALIGN_DOWN'
> #define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
>                                 ^
>cc1: all warnings being treated as errors
>make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1
>make[1]: *** Waiting for unfinished jobs....
>  CC      x86_64-softmmu/target/i386/arch_dump.o
>  CC      aarch64-softmmu/hw/usb/tusb6010.o
>---
>  CC      aarch64-softmmu/hw/arm/xlnx-zynqmp.o
>In file included from /tmp/qemu-test/src/migration/ram.c:29:
>/tmp/qemu-test/src/migration/ram.c: In function 'ram_load_postcopy':
>/tmp/qemu-test/src/migration/ram.c:4177:56: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>             void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
>                                                        ^
>/tmp/qemu-test/src/include/qemu/osdep.h:268:33: note: in definition of macro 'QEMU_ALIGN_DOWN'
> #define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
>                                 ^
>cc1: all warnings being treated as errors
>make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1
>make[1]: *** Waiting for unfinished jobs....
>make: *** [Makefile:482: aarch64-softmmu/all] Error 2
>make: *** Waiting for unfinished jobs....
>make: *** [Makefile:482: x86_64-softmmu/all] Error 2
>Traceback (most recent call last):
>  File "./tests/docker/docker.py", line 662, in <module>
>    sys.exit(main())
>---
>    raise CalledProcessError(retcode, cmd)
>subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=90570434880344249cff701baa188163', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-dh8p6f27/src/docker-src.2019-10-18-12.47.19.4164:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
>filter=--filter=label=com.qemu.instance.uuid=90570434880344249cff701baa188163
>make[1]: *** [docker-run] Error 1
>make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-dh8p6f27/src'
>make: *** [docker-run-test-mingw@fedora] Error 2
>
>real    2m45.691s
>user    0m8.390s
>
>
>The full log is available at
>http://patchew.org/logs/20191018004850.9888-1-richardw.yang@linux.intel.com/testing.docker-mingw@fedora/?type=message.
>---
>Email generated automatically by Patchew [https://patchew.org/].
>Please send your feedback to patchew-devel@redhat.com

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size
  2019-10-18  0:48 ` [PATCH 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size Wei Yang
@ 2019-11-06 18:18   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-06 18:18 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> In this case, page_buffer content would not be used.
> 
> Skip this to save some time.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/ram.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 99a98b2da4..7938a643d9 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4091,7 +4091,13 @@ static int ram_load_postcopy(QEMUFile *f)
>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>          case RAM_SAVE_FLAG_ZERO:
>              ch = qemu_get_byte(f);
> -            memset(page_buffer, ch, TARGET_PAGE_SIZE);
> +            /*
> +             * Can skip to set page_buffer when
> +             * this is a zero page and (block->page_size == TARGET_PAGE_SIZE).
> +             */

When it's zero we will use place_page_zero which doesn't need
to page_buffer; so yes that's OK; and you're right, the gotcha is with 
mismatched page sizes where one subpage might be zero but not all of
them; so yes it's right that we need that check.

SO yes,


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> +            if (ch || !matches_target_page_size) {
> +                memset(page_buffer, ch, TARGET_PAGE_SIZE);
> +            }
>              if (ch) {
>                  all_zero = false;
>              }
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 6/6] migration/postcopy: enable compress during postcopy
  2019-10-18  0:48 ` [PATCH 6/6] migration/postcopy: enable compress during postcopy Wei Yang
@ 2019-11-06 19:55   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-06 19:55 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> postcopy requires to place a whole host page, while migration thread
> migrate memory in target page size. This makes postcopy need to collect
> all target pages in one host page before placing via userfaultfd.
> 
> To enable compress during postcopy, there are two problems to solve:
> 
>     1. Random order for target page arrival
>     2. Target pages in one host page arrives without interrupt by target
>        page from other host page
> 
> The first one is handled by previous cleanup patch.
> 
> This patch handles the second one by:
> 
>     1. Flush compress thread for each host page
>     2. Wait for decompress thread for before placing host page
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/migration.c | 11 -----------
>  migration/ram.c       | 28 +++++++++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3febd0f8f3..72e53e2249 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1000,17 +1000,6 @@ static bool migrate_caps_check(bool *cap_list,
>  #endif
>  
>      if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> -        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
> -            /* The decompression threads asynchronously write into RAM
> -             * rather than use the atomic copies needed to avoid
> -             * userfaulting.  It should be possible to fix the decompression
> -             * threads for compatibility in future.
> -             */
> -            error_setg(errp, "Postcopy is not currently compatible "
> -                       "with compression");
> -            return false;
> -        }
> -

Yes, I think that's safe - as long as the 'compress' gets set on both
sides you should never get a combination of one side trying it and the
other not being capable.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>          /* This check is reasonably expensive, so only when it's being
>           * set the first time, also it's only the destination that needs
>           * special support.
> diff --git a/migration/ram.c b/migration/ram.c
> index da0596411c..1403978d75 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3449,6 +3449,14 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>  
>              rs->target_page_count += pages;
>  
> +            /*
> +             * During postcopy, it is necessary to make sure one whole host
> +             * page is sent in one chunk.
> +             */
> +            if (migrate_postcopy_ram()) {
> +                flush_compressed_data(rs);
> +            }
> +
>              /*
>               * we want to check in the 1st loop, just in case it was the 1st
>               * time and we had to sync the dirty bitmap.
> @@ -4025,6 +4033,7 @@ static int ram_load_postcopy(QEMUFile *f)
>          void *place_source = NULL;
>          RAMBlock *block = NULL;
>          uint8_t ch;
> +        int len;
>  
>          addr = qemu_get_be64(f);
>  
> @@ -4042,7 +4051,8 @@ static int ram_load_postcopy(QEMUFile *f)
>  
>          trace_ram_load_postcopy_loop((uint64_t)addr, flags);
>          place_needed = false;
> -        if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE)) {
> +        if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
> +                     RAM_SAVE_FLAG_COMPRESS_PAGE)) {
>              block = ram_block_from_stream(f, flags);
>  
>              host = host_from_ram_block_offset(block, addr);
> @@ -4114,6 +4124,17 @@ static int ram_load_postcopy(QEMUFile *f)
>                                           TARGET_PAGE_SIZE);
>              }
>              break;
> +        case RAM_SAVE_FLAG_COMPRESS_PAGE:
> +            all_zero = false;
> +            len = qemu_get_be32(f);
> +            if (len < 0 || len > compressBound(TARGET_PAGE_SIZE)) {
> +                error_report("Invalid compressed data length: %d", len);
> +                ret = -EINVAL;
> +                break;
> +            }
> +            decompress_data_with_multi_threads(f, page_buffer, len);
> +            break;
> +
>          case RAM_SAVE_FLAG_EOS:
>              /* normal exit */
>              multifd_recv_sync_main();
> @@ -4125,6 +4146,11 @@ static int ram_load_postcopy(QEMUFile *f)
>              break;
>          }
>  
> +        /* Got the whole host page, wait for decompress before placing. */
> +        if (place_needed) {
> +            ret |= wait_for_decompress_done();
> +        }
> +
>          /* Detect for any possible file errors */
>          if (!ret && qemu_file_get_error(f)) {
>              ret = qemu_file_get_error(f);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/6] migration/postcopy: wait for decompress thread in precopy
  2019-10-18  0:48 ` [PATCH 2/6] migration/postcopy: wait for decompress thread in precopy Wei Yang
@ 2019-11-06 19:57   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-06 19:57 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Compress is not supported with postcopy, it is safe to wait for
> decompress thread just in precopy.
> 
> This is a preparation for later patch.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

OK, since your last patch does this for postcopy.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/ram.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 7938a643d9..f59e3fe197 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4375,6 +4375,7 @@ static int ram_load_precopy(QEMUFile *f)
>          }
>      }
>  
> +    ret |= wait_for_decompress_done();
>      return ret;
>  }
>  
> @@ -4406,8 +4407,6 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          } else {
>              ret = ram_load_precopy(f);
>          }
> -
> -        ret |= wait_for_decompress_done();
>      }
>      trace_ram_load_complete(ret, seq_iter);
>  
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] migration/postcopy: count target page number to decide the place_needed
  2019-10-18  0:48 ` [PATCH 3/6] migration/postcopy: count target page number to decide the place_needed Wei Yang
@ 2019-11-06 19:59   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-06 19:59 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> In postcopy, it requires to place whole host page instead of target
> page.
> 
> Currently, it relies on the page offset to decide whether this is the
> last target page. We also can count the target page number during the
> iteration. When the number of target page equals
> (host page size / target page size), this means it is the last target
> page in the host page.
> 
> This is a preparation for non-ordered target page transmission.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/ram.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f59e3fe197..5c05376d8d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4017,6 +4017,7 @@ static int ram_load_postcopy(QEMUFile *f)
>      void *postcopy_host_page = mis->postcopy_tmp_page;
>      void *last_host = NULL;
>      bool all_zero = false;
> +    int target_pages = 0;
>  
>      while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
>          ram_addr_t addr;
> @@ -4051,6 +4052,7 @@ static int ram_load_postcopy(QEMUFile *f)
>                  ret = -EINVAL;
>                  break;
>              }
> +            target_pages++;
>              matches_target_page_size = block->page_size == TARGET_PAGE_SIZE;
>              /*
>               * Postcopy requires that we place whole host pages atomically;
> @@ -4082,8 +4084,10 @@ static int ram_load_postcopy(QEMUFile *f)
>               * If it's the last part of a host page then we place the host
>               * page
>               */
> -            place_needed = (((uintptr_t)host + TARGET_PAGE_SIZE) &
> -                                     (block->page_size - 1)) == 0;
> +            if (target_pages == (block->page_size / TARGET_PAGE_SIZE)) {
> +                place_needed = true;
> +                target_pages = 0;
> +            }
>              place_source = postcopy_host_page;
>          }
>          last_host = host;
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/6] migration/postcopy: set all_zero to true on the first target page
  2019-10-18  0:48 ` [PATCH 4/6] migration/postcopy: set all_zero to true on the first target page Wei Yang
@ 2019-11-06 20:04   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-06 20:04 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> For the first target page, all_zero is set to true for this round check.
> 
> After target_pages introduced, we could leverage this variable instead
> of checking the address offset.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Yes, OK - I find target_pages being incremented before
this point a bit confusing, I think of '0' as the first one.

Still, it's OK:


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/ram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 5c05376d8d..b5759793a9 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4067,7 +4067,7 @@ static int ram_load_postcopy(QEMUFile *f)
>              page_buffer = postcopy_host_page +
>                            ((uintptr_t)host & (block->page_size - 1));
>              /* If all TP are zero then we can optimise the place */
> -            if (!((uintptr_t)host & (block->page_size - 1))) {
> +            if (target_pages == 1) {
>                  all_zero = true;
>              } else {
>                  /* not the 1st TP within the HP */
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/6] migration/postcopy: enable random order target page arrival
  2019-10-18  0:48 ` [PATCH 5/6] migration/postcopy: enable random order target page arrival Wei Yang
@ 2019-11-06 20:08   ` Dr. David Alan Gilbert
  2019-11-07  6:00     ` Wei Yang
  0 siblings, 1 reply; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-06 20:08 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> After using number of target page received to track one host page, we
> could have the capability to handle random order target page arrival in
> one host page.
> 
> This is a preparation for enabling compress during postcopy.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/ram.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index b5759793a9..da0596411c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4015,7 +4015,6 @@ static int ram_load_postcopy(QEMUFile *f)
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      /* Temporary page that is later 'placed' */
>      void *postcopy_host_page = mis->postcopy_tmp_page;
> -    void *last_host = NULL;
>      bool all_zero = false;
>      int target_pages = 0;
>  
> @@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
>               * that's moved into place later.
>               * The migration protocol uses,  possibly smaller, target-pages
>               * however the source ensures it always sends all the components
> -             * of a host page in order.
> +             * of a host page in one chunk.
>               */
>              page_buffer = postcopy_host_page +
>                            ((uintptr_t)host & (block->page_size - 1));
>              /* If all TP are zero then we can optimise the place */
>              if (target_pages == 1) {
>                  all_zero = true;
> -            } else {
> -                /* not the 1st TP within the HP */
> -                if (host != (last_host + TARGET_PAGE_SIZE)) {
> -                    error_report("Non-sequential target page %p/%p",
> -                                  host, last_host);
> -                    ret = -EINVAL;
> -                    break;
> -                }

I think this is losing more protection than needed.
I think you can still protect against a page from a different host-page
arriving until we've placed the current host-page.
So something like:

    if (((uintptr_t)host & ~(block->page_size - 1)) !=
        last_host)

and then set last_host to the start of the host page.

Then you'll check if that flush is really working.

Dave

>              }
>  
> -
>              /*
>               * If it's the last part of a host page then we place the host
>               * page
> @@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
>              }
>              place_source = postcopy_host_page;
>          }
> -        last_host = host;
>  
>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>          case RAM_SAVE_FLAG_ZERO:
> @@ -4143,7 +4132,8 @@ static int ram_load_postcopy(QEMUFile *f)
>  
>          if (!ret && place_needed) {
>              /* This gets called at the last target page in the host page */
> -            void *place_dest = host + TARGET_PAGE_SIZE - block->page_size;
> +            void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
> +                                                       block->page_size);
>  
>              if (all_zero) {
>                  ret = postcopy_place_page_zero(mis, place_dest,
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
  2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
                   ` (6 preceding siblings ...)
  2019-10-18 16:50 ` [PATCH 0/6] " no-reply
@ 2019-11-06 20:11 ` Dr. David Alan Gilbert
  2019-11-07  6:02   ` Wei Yang
  7 siblings, 1 reply; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-06 20:11 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> This patch set tries enable compress during postcopy.
> 
> postcopy requires to place a whole host page, while migration thread migrate
> memory in target page size. This makes postcopy need to collect all target
> pages in one host page before placing via userfaultfd.
> 
> To enable compress during postcopy, there are two problems to solve:
> 
>     1. Random order for target page arrival
>     2. Target pages in one host page arrives without interrupt by target
>        page from other host page
> 
> The first one is handled by counting the number of target pages arrived
> instead of the last target page arrived.
> 
> The second one is handled by:
> 
>     1. Flush compress thread for each host page
>     2. Wait for decompress thread for before placing host page
> 
> With the combination of these two changes, compress is enabled during
> postcopy.

What have you tested this with? 2MB huge pages I guess?

Dave

> Wei Yang (6):
>   migration/postcopy: reduce memset when it is zero page and
>     matches_target_page_size
>   migration/postcopy: wait for decompress thread in precopy
>   migration/postcopy: count target page number to decide the
>     place_needed
>   migration/postcopy: set all_zero to true on the first target page
>   migration/postcopy: enable random order target page arrival
>   migration/postcopy: enable compress during postcopy
> 
>  migration/migration.c | 11 --------
>  migration/ram.c       | 65 ++++++++++++++++++++++++++++++-------------
>  2 files changed, 45 insertions(+), 31 deletions(-)
> 
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/6] migration/postcopy: enable random order target page arrival
  2019-11-06 20:08   ` Dr. David Alan Gilbert
@ 2019-11-07  6:00     ` Wei Yang
  2019-11-07  9:14       ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-11-07  6:00 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, Wei Yang, quintela

On Wed, Nov 06, 2019 at 08:08:28PM +0000, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> After using number of target page received to track one host page, we
>> could have the capability to handle random order target page arrival in
>> one host page.
>> 
>> This is a preparation for enabling compress during postcopy.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/ram.c | 16 +++-------------
>>  1 file changed, 3 insertions(+), 13 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b5759793a9..da0596411c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -4015,7 +4015,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>>      /* Temporary page that is later 'placed' */
>>      void *postcopy_host_page = mis->postcopy_tmp_page;
>> -    void *last_host = NULL;
>>      bool all_zero = false;
>>      int target_pages = 0;
>>  
>> @@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
>>               * that's moved into place later.
>>               * The migration protocol uses,  possibly smaller, target-pages
>>               * however the source ensures it always sends all the components
>> -             * of a host page in order.
>> +             * of a host page in one chunk.
>>               */
>>              page_buffer = postcopy_host_page +
>>                            ((uintptr_t)host & (block->page_size - 1));
>>              /* If all TP are zero then we can optimise the place */
>>              if (target_pages == 1) {
>>                  all_zero = true;
>> -            } else {
>> -                /* not the 1st TP within the HP */
>> -                if (host != (last_host + TARGET_PAGE_SIZE)) {
>> -                    error_report("Non-sequential target page %p/%p",
>> -                                  host, last_host);
>> -                    ret = -EINVAL;
>> -                    break;
>> -                }
>
>I think this is losing more protection than needed.
>I think you can still protect against a page from a different host-page
>arriving until we've placed the current host-page.
>So something like:
>
>    if (((uintptr_t)host & ~(block->page_size - 1)) !=
>        last_host)
>

OK, looks reasonable.

>and then set last_host to the start of the host page.
>

I think it is not necessary to update the last_host on each target page. We
can just set it at the first target page.

>Then you'll check if that flush is really working.
>
>Dave
>
>>              }
>>  
>> -
>>              /*
>>               * If it's the last part of a host page then we place the host
>>               * page
>> @@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>              }
>>              place_source = postcopy_host_page;
>>          }
>> -        last_host = host;
>>  
>>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>>          case RAM_SAVE_FLAG_ZERO:
>> @@ -4143,7 +4132,8 @@ static int ram_load_postcopy(QEMUFile *f)
>>  
>>          if (!ret && place_needed) {
>>              /* This gets called at the last target page in the host page */
>> -            void *place_dest = host + TARGET_PAGE_SIZE - block->page_size;
>> +            void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
>> +                                                       block->page_size);
>>  
>>              if (all_zero) {
>>                  ret = postcopy_place_page_zero(mis, place_dest,
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
  2019-11-06 20:11 ` Dr. David Alan Gilbert
@ 2019-11-07  6:02   ` Wei Yang
  2019-11-07  9:15     ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-11-07  6:02 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, Wei Yang, quintela

On Wed, Nov 06, 2019 at 08:11:44PM +0000, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> This patch set tries enable compress during postcopy.
>> 
>> postcopy requires to place a whole host page, while migration thread migrate
>> memory in target page size. This makes postcopy need to collect all target
>> pages in one host page before placing via userfaultfd.
>> 
>> To enable compress during postcopy, there are two problems to solve:
>> 
>>     1. Random order for target page arrival
>>     2. Target pages in one host page arrives without interrupt by target
>>        page from other host page
>> 
>> The first one is handled by counting the number of target pages arrived
>> instead of the last target page arrived.
>> 
>> The second one is handled by:
>> 
>>     1. Flush compress thread for each host page
>>     2. Wait for decompress thread for before placing host page
>> 
>> With the combination of these two changes, compress is enabled during
>> postcopy.
>
>What have you tested this with? 2MB huge pages I guess?
>

I tried with this qemu option:

   -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/guest2,size=4G \
   -device pc-dimm,id=dimm1,memdev=mem1

/dev/hugepages/guest2 is a file under hugetlbfs

   hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime,pagesize=2M)

>Dave
>
>> Wei Yang (6):
>>   migration/postcopy: reduce memset when it is zero page and
>>     matches_target_page_size
>>   migration/postcopy: wait for decompress thread in precopy
>>   migration/postcopy: count target page number to decide the
>>     place_needed
>>   migration/postcopy: set all_zero to true on the first target page
>>   migration/postcopy: enable random order target page arrival
>>   migration/postcopy: enable compress during postcopy
>> 
>>  migration/migration.c | 11 --------
>>  migration/ram.c       | 65 ++++++++++++++++++++++++++++++-------------
>>  2 files changed, 45 insertions(+), 31 deletions(-)
>> 
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/6] migration/postcopy: enable random order target page arrival
  2019-11-07  6:00     ` Wei Yang
@ 2019-11-07  9:14       ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-07  9:14 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Wed, Nov 06, 2019 at 08:08:28PM +0000, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> >> After using number of target page received to track one host page, we
> >> could have the capability to handle random order target page arrival in
> >> one host page.
> >> 
> >> This is a preparation for enabling compress during postcopy.
> >> 
> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >> ---
> >>  migration/ram.c | 16 +++-------------
> >>  1 file changed, 3 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index b5759793a9..da0596411c 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -4015,7 +4015,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >>      MigrationIncomingState *mis = migration_incoming_get_current();
> >>      /* Temporary page that is later 'placed' */
> >>      void *postcopy_host_page = mis->postcopy_tmp_page;
> >> -    void *last_host = NULL;
> >>      bool all_zero = false;
> >>      int target_pages = 0;
> >>  
> >> @@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
> >>               * that's moved into place later.
> >>               * The migration protocol uses,  possibly smaller, target-pages
> >>               * however the source ensures it always sends all the components
> >> -             * of a host page in order.
> >> +             * of a host page in one chunk.
> >>               */
> >>              page_buffer = postcopy_host_page +
> >>                            ((uintptr_t)host & (block->page_size - 1));
> >>              /* If all TP are zero then we can optimise the place */
> >>              if (target_pages == 1) {
> >>                  all_zero = true;
> >> -            } else {
> >> -                /* not the 1st TP within the HP */
> >> -                if (host != (last_host + TARGET_PAGE_SIZE)) {
> >> -                    error_report("Non-sequential target page %p/%p",
> >> -                                  host, last_host);
> >> -                    ret = -EINVAL;
> >> -                    break;
> >> -                }
> >
> >I think this is losing more protection than needed.
> >I think you can still protect against a page from a different host-page
> >arriving until we've placed the current host-page.
> >So something like:
> >
> >    if (((uintptr_t)host & ~(block->page_size - 1)) !=
> >        last_host)
> >
> 
> OK, looks reasonable.
> 
> >and then set last_host to the start of the host page.
> >
> 
> I think it is not necessary to update the last_host on each target page. We
> can just set it at the first target page.

Yes, that would be fine.

Dave

> >Then you'll check if that flush is really working.
> >
> >Dave
> >
> >>              }
> >>  
> >> -
> >>              /*
> >>               * If it's the last part of a host page then we place the host
> >>               * page
> >> @@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >>              }
> >>              place_source = postcopy_host_page;
> >>          }
> >> -        last_host = host;
> >>  
> >>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> >>          case RAM_SAVE_FLAG_ZERO:
> >> @@ -4143,7 +4132,8 @@ static int ram_load_postcopy(QEMUFile *f)
> >>  
> >>          if (!ret && place_needed) {
> >>              /* This gets called at the last target page in the host page */
> >> -            void *place_dest = host + TARGET_PAGE_SIZE - block->page_size;
> >> +            void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
> >> +                                                       block->page_size);
> >>  
> >>              if (all_zero) {
> >>                  ret = postcopy_place_page_zero(mis, place_dest,
> >> -- 
> >> 2.17.1
> >> 
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
  2019-11-07  6:02   ` Wei Yang
@ 2019-11-07  9:15     ` Dr. David Alan Gilbert
  2019-11-07 12:03       ` Wei Yang
  0 siblings, 1 reply; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-07  9:15 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Wed, Nov 06, 2019 at 08:11:44PM +0000, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> >> This patch set tries enable compress during postcopy.
> >> 
> >> postcopy requires to place a whole host page, while migration thread migrate
> >> memory in target page size. This makes postcopy need to collect all target
> >> pages in one host page before placing via userfaultfd.
> >> 
> >> To enable compress during postcopy, there are two problems to solve:
> >> 
> >>     1. Random order for target page arrival
> >>     2. Target pages in one host page arrives without interrupt by target
> >>        page from other host page
> >> 
> >> The first one is handled by counting the number of target pages arrived
> >> instead of the last target page arrived.
> >> 
> >> The second one is handled by:
> >> 
> >>     1. Flush compress thread for each host page
> >>     2. Wait for decompress thread for before placing host page
> >> 
> >> With the combination of these two changes, compress is enabled during
> >> postcopy.
> >
> >What have you tested this with? 2MB huge pages I guess?
> >
> 
> I tried with this qemu option:
> 
>    -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/guest2,size=4G \
>    -device pc-dimm,id=dimm1,memdev=mem1
> 
> /dev/hugepages/guest2 is a file under hugetlbfs
> 
>    hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime,pagesize=2M)

OK, yes that should be fine.
I suspect on Power/ARM where they have normal memory with 16/64k pages,
the cost of the flush will mean compression is more expensive in
postcopy mode; but still makes it possible.

Dave

> >Dave
> >
> >> Wei Yang (6):
> >>   migration/postcopy: reduce memset when it is zero page and
> >>     matches_target_page_size
> >>   migration/postcopy: wait for decompress thread in precopy
> >>   migration/postcopy: count target page number to decide the
> >>     place_needed
> >>   migration/postcopy: set all_zero to true on the first target page
> >>   migration/postcopy: enable random order target page arrival
> >>   migration/postcopy: enable compress during postcopy
> >> 
> >>  migration/migration.c | 11 --------
> >>  migration/ram.c       | 65 ++++++++++++++++++++++++++++++-------------
> >>  2 files changed, 45 insertions(+), 31 deletions(-)
> >> 
> >> -- 
> >> 2.17.1
> >> 
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
  2019-11-07  9:15     ` Dr. David Alan Gilbert
@ 2019-11-07 12:03       ` Wei Yang
  2019-11-07 12:06         ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 22+ messages in thread
From: Wei Yang @ 2019-11-07 12:03 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, Wei Yang, quintela

On Thu, Nov 07, 2019 at 09:15:44AM +0000, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> On Wed, Nov 06, 2019 at 08:11:44PM +0000, Dr. David Alan Gilbert wrote:
>> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> >> This patch set tries enable compress during postcopy.
>> >> 
>> >> postcopy requires to place a whole host page, while migration thread migrate
>> >> memory in target page size. This makes postcopy need to collect all target
>> >> pages in one host page before placing via userfaultfd.
>> >> 
>> >> To enable compress during postcopy, there are two problems to solve:
>> >> 
>> >>     1. Random order for target page arrival
>> >>     2. Target pages in one host page arrives without interrupt by target
>> >>        page from other host page
>> >> 
>> >> The first one is handled by counting the number of target pages arrived
>> >> instead of the last target page arrived.
>> >> 
>> >> The second one is handled by:
>> >> 
>> >>     1. Flush compress thread for each host page
>> >>     2. Wait for decompress thread for before placing host page
>> >> 
>> >> With the combination of these two changes, compress is enabled during
>> >> postcopy.
>> >
>> >What have you tested this with? 2MB huge pages I guess?
>> >
>> 
>> I tried with this qemu option:
>> 
>>    -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/guest2,size=4G \
>>    -device pc-dimm,id=dimm1,memdev=mem1
>> 
>> /dev/hugepages/guest2 is a file under hugetlbfs
>> 
>>    hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime,pagesize=2M)
>
>OK, yes that should be fine.
>I suspect on Power/ARM where they have normal memory with 16/64k pages,
>the cost of the flush will mean compression is more expensive in
>postcopy mode; but still makes it possible.
>

Not get your point clearly about more expensive. You mean more expensive on
ARM/Power?

If the solution looks good to you, I would prepare v2.

>Dave
>
>> >Dave
>> >
>> >> Wei Yang (6):
>> >>   migration/postcopy: reduce memset when it is zero page and
>> >>     matches_target_page_size
>> >>   migration/postcopy: wait for decompress thread in precopy
>> >>   migration/postcopy: count target page number to decide the
>> >>     place_needed
>> >>   migration/postcopy: set all_zero to true on the first target page
>> >>   migration/postcopy: enable random order target page arrival
>> >>   migration/postcopy: enable compress during postcopy
>> >> 
>> >>  migration/migration.c | 11 --------
>> >>  migration/ram.c       | 65 ++++++++++++++++++++++++++++++-------------
>> >>  2 files changed, 45 insertions(+), 31 deletions(-)
>> >> 
>> >> -- 
>> >> 2.17.1
>> >> 
>> >--
>> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>> 
>> -- 
>> Wei Yang
>> Help you, Help me
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
  2019-11-07 12:03       ` Wei Yang
@ 2019-11-07 12:06         ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-07 12:06 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Thu, Nov 07, 2019 at 09:15:44AM +0000, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> >> On Wed, Nov 06, 2019 at 08:11:44PM +0000, Dr. David Alan Gilbert wrote:
> >> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> >> >> This patch set tries enable compress during postcopy.
> >> >> 
> >> >> postcopy requires to place a whole host page, while migration thread migrate
> >> >> memory in target page size. This makes postcopy need to collect all target
> >> >> pages in one host page before placing via userfaultfd.
> >> >> 
> >> >> To enable compress during postcopy, there are two problems to solve:
> >> >> 
> >> >>     1. Random order for target page arrival
> >> >>     2. Target pages in one host page arrives without interrupt by target
> >> >>        page from other host page
> >> >> 
> >> >> The first one is handled by counting the number of target pages arrived
> >> >> instead of the last target page arrived.
> >> >> 
> >> >> The second one is handled by:
> >> >> 
> >> >>     1. Flush compress thread for each host page
> >> >>     2. Wait for decompress thread for before placing host page
> >> >> 
> >> >> With the combination of these two changes, compress is enabled during
> >> >> postcopy.
> >> >
> >> >What have you tested this with? 2MB huge pages I guess?
> >> >
> >> 
> >> I tried with this qemu option:
> >> 
> >>    -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/guest2,size=4G \
> >>    -device pc-dimm,id=dimm1,memdev=mem1
> >> 
> >> /dev/hugepages/guest2 is a file under hugetlbfs
> >> 
> >>    hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime,pagesize=2M)
> >
> >OK, yes that should be fine.
> >I suspect on Power/ARM where they have normal memory with 16/64k pages,
> >the cost of the flush will mean compression is more expensive in
> >postcopy mode; but still makes it possible.
> >
> 
> Not get your point clearly about more expensive. You mean more expensive on
> ARM/Power?

Yes;  you're doing a flush at the end of each host page;  on x86 without
hugepages you don't do anything, on arm/power you'll need to do a flush
at the end of each of their normal pages - so that's a bit more
expensive.

> If the solution looks good to you, I would prepare v2.

Yes; I think it is OK.

Dave

> >Dave
> >
> >> >Dave
> >> >
> >> >> Wei Yang (6):
> >> >>   migration/postcopy: reduce memset when it is zero page and
> >> >>     matches_target_page_size
> >> >>   migration/postcopy: wait for decompress thread in precopy
> >> >>   migration/postcopy: count target page number to decide the
> >> >>     place_needed
> >> >>   migration/postcopy: set all_zero to true on the first target page
> >> >>   migration/postcopy: enable random order target page arrival
> >> >>   migration/postcopy: enable compress during postcopy
> >> >> 
> >> >>  migration/migration.c | 11 --------
> >> >>  migration/ram.c       | 65 ++++++++++++++++++++++++++++++-------------
> >> >>  2 files changed, 45 insertions(+), 31 deletions(-)
> >> >> 
> >> >> -- 
> >> >> 2.17.1
> >> >> 
> >> >--
> >> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >> 
> >> -- 
> >> Wei Yang
> >> Help you, Help me
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2019-11-07 12:07 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18  0:48 [PATCH 0/6] migration/postcopy: enable compress during postcopy Wei Yang
2019-10-18  0:48 ` [PATCH 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size Wei Yang
2019-11-06 18:18   ` Dr. David Alan Gilbert
2019-10-18  0:48 ` [PATCH 2/6] migration/postcopy: wait for decompress thread in precopy Wei Yang
2019-11-06 19:57   ` Dr. David Alan Gilbert
2019-10-18  0:48 ` [PATCH 3/6] migration/postcopy: count target page number to decide the place_needed Wei Yang
2019-11-06 19:59   ` Dr. David Alan Gilbert
2019-10-18  0:48 ` [PATCH 4/6] migration/postcopy: set all_zero to true on the first target page Wei Yang
2019-11-06 20:04   ` Dr. David Alan Gilbert
2019-10-18  0:48 ` [PATCH 5/6] migration/postcopy: enable random order target page arrival Wei Yang
2019-11-06 20:08   ` Dr. David Alan Gilbert
2019-11-07  6:00     ` Wei Yang
2019-11-07  9:14       ` Dr. David Alan Gilbert
2019-10-18  0:48 ` [PATCH 6/6] migration/postcopy: enable compress during postcopy Wei Yang
2019-11-06 19:55   ` Dr. David Alan Gilbert
2019-10-18 16:50 ` [PATCH 0/6] " no-reply
2019-10-19  0:15   ` Wei Yang
2019-11-06 20:11 ` Dr. David Alan Gilbert
2019-11-07  6:02   ` Wei Yang
2019-11-07  9:15     ` Dr. David Alan Gilbert
2019-11-07 12:03       ` Wei Yang
2019-11-07 12:06         ` Dr. David Alan Gilbert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.