[PATCH v2 0/4] tests/qtest/migration-test: Improve shared

[PATCH v2 0/4] tests/qtest/migration-test: Improve shared

[Nicholas Piggin]

These apply on top of the previous ppc series. They are similar
to what was discussed except the end result does not attempt to
run /dev/shm tests on Gitlab CI because of concerns about
consistency of test results if shm becomes exhausted.

Thanks,
Nick

Nicholas Piggin (4):
  tests/qtest/migration-test: Use regular file file for shared-memory
    tests
  tests/qtest/migration-test: Enable test_mode_reboot
  tests/qtest/migration-test: Fix and enable test_ignore_shared
  tests/qtest/migration-test: Add a postcopy memfile test

 tests/qtest/migration-test.c | 128 ++++++++++++++++++++++++-----------
 1 file changed, 88 insertions(+), 40 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] tests/qtest/migration-test: Use regular file file for shared-memory tests

[Nicholas Piggin]

There is no need to use /dev/shm for file-backed memory devices, and
on Gitlab CI the tmpfs mount is too small to be usable for migration
tests. Switch to using a regular file in /tmp/ which will usually have
more space available.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/migration-test.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d6f5ceed80..8bbf45313d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -552,7 +552,7 @@ typedef struct {
      * unconditionally, because it means the user would like to be verbose.
      */
     bool hide_stderr;
-    bool use_shmem;
+    bool use_memfile;
     /* only launch the target process */
     bool only_target;
     /* Use dirty ring if true; dirty logging otherwise */
@@ -672,21 +672,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     g_autofree gchar *cmd_source = NULL;
     g_autofree gchar *cmd_target = NULL;
     const gchar *ignore_stderr;
-    g_autofree char *shmem_opts = NULL;
-    g_autofree char *shmem_path = NULL;
+    g_autofree char *memfile_opts = NULL;
+    g_autofree char *memfile_path = NULL;
     const char *kvm_opts = NULL;
     const char *arch = qtest_get_arch();
     const char *memory_size;
     const char *machine_alias, *machine_opts = "";
     g_autofree char *machine = NULL;
 
-    if (args->use_shmem) {
-        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
-            g_test_skip("/dev/shm is not supported");
-            return -1;
-        }
-    }
-
     dst_state = (QTestMigrationState) { };
     src_state = (QTestMigrationState) { };
     bootfile_create(tmpfs, args->suspend_me);
@@ -746,12 +739,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         ignore_stderr = "";
     }
 
-    if (args->use_shmem) {
-        shmem_path = g_strdup_printf("/dev/shm/qemu-%d", getpid());
-        shmem_opts = g_strdup_printf(
+    if (args->use_memfile) {
+        memfile_path = g_strdup_printf("/%s/qemu-%d", tmpfs, getpid());
+        memfile_opts = g_strdup_printf(
             "-object memory-backend-file,id=mem0,size=%s"
             ",mem-path=%s,share=on -numa node,memdev=mem0",
-            memory_size, shmem_path);
+            memory_size, memfile_path);
     }
 
     if (args->use_dirty_ring) {
@@ -780,7 +773,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  memory_size, tmpfs,
                                  arch_opts ? arch_opts : "",
                                  arch_source ? arch_source : "",
-                                 shmem_opts ? shmem_opts : "",
+                                 memfile_opts ? memfile_opts : "",
                                  args->opts_source ? args->opts_source : "",
                                  ignore_stderr);
     if (!args->only_target) {
@@ -802,7 +795,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  memory_size, tmpfs, uri,
                                  arch_opts ? arch_opts : "",
                                  arch_target ? arch_target : "",
-                                 shmem_opts ? shmem_opts : "",
+                                 memfile_opts ? memfile_opts : "",
                                  args->opts_target ? args->opts_target : "",
                                  ignore_stderr);
     *to = qtest_init_with_env(QEMU_ENV_DST, cmd_target);
@@ -814,8 +807,8 @@ static int test_migrate_start(QTestState **from, QTestState **to,
      * Remove shmem file immediately to avoid memory leak in test failed case.
      * It's valid because QEMU has already opened this file
      */
-    if (args->use_shmem) {
-        unlink(shmem_path);
+    if (args->use_memfile) {
+        unlink(memfile_path);
     }
 
     return 0;
@@ -1868,6 +1861,9 @@ static void test_ignore_shared(void)
 {
     g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *from, *to;
+    MigrateStart args = {
+        .use_memfile = true,
+    };
 
     if (test_migrate_start(&from, &to, uri, false, true, NULL, NULL)) {
         return;
@@ -2024,7 +2020,7 @@ static void test_mode_reboot(void)
     g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
                                            FILE_TEST_FILENAME);
     MigrateCommon args = {
-        .start.use_shmem = true,
+        .start.use_memfile = true,
         .connect_uri = uri,
         .listen_uri = "defer",
         .start_hook = test_mode_reboot_start
-- 
2.43.0

Re: [PATCH v2 1/4] tests/qtest/migration-test: Use regular file file for shared-memory tests

[Prasad Pandit]

On Thu, 30 May 2024 at 15:25, Nicholas Piggin <npiggin@gmail.com> wrote:
> There is no need to use /dev/shm for file-backed memory devices, and
> on Gitlab CI the tmpfs mount is too small to be usable for migration
> tests. Switch to using a regular file in /tmp/ which will usually have
> more space available.
...
> -    if (args->use_shmem) {
> -        shmem_path = g_strdup_printf("/dev/shm/qemu-%d", getpid());
> -        shmem_opts = g_strdup_printf(
> +    if (args->use_memfile) {
> +        memfile_path = g_strdup_printf("/%s/qemu-%d", tmpfs, getpid());
> +        memfile_opts = g_strdup_printf(
...
>      g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
>      QTestState *from, *to;
...
>      g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME);

* Maybe the 'tmpfs' variable needs to be renamed to indicate that it
uses '/tmp/' or '/var/tmp' directory to create temporary files. And it
is not in memory tmpfs(5) used for shared memory '/dev/shm'. Commit
message above says 'tmpfs mount is too small' and above calls continue
to use 'tmpfs' variable to create temporary files. It's a little
confusing.

Otherwise patch looks okay.
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thank you.
---
  - Prasad

[PATCH v2 2/4] tests/qtest/migration-test: Enable test_mode_reboot

[Nicholas Piggin]

Fabiano pointed out this test probably is not flaky, just that it could
not run under Gitlab CI due to very small shm filesystem size in that
environment.

Now that it has moved to use /tmp instead of /dev/shm files, enable it.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/migration-test.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 8bbf45313d..de380757be 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -3469,15 +3469,7 @@ int main(int argc, char **argv)
                        test_precopy_file_offset);
     migration_test_add("/migration/precopy/file/offset/bad",
                        test_precopy_file_offset_bad);
-
-    /*
-     * Our CI system has problems with shared memory.
-     * Don't run this test until we find a workaround.
-     */
-    if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-        migration_test_add("/migration/mode/reboot", test_mode_reboot);
-    }
-
+    migration_test_add("/migration/mode/reboot", test_mode_reboot);
     migration_test_add("/migration/precopy/file/mapped-ram",
                        test_precopy_file_mapped_ram);
     migration_test_add("/migration/precopy/file/mapped-ram/live",
-- 
2.43.0

Re: [PATCH v2 2/4] tests/qtest/migration-test: Enable test_mode_reboot

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> Fabiano pointed out this test probably is not flaky, just that it could
> not run under Gitlab CI due to very small shm filesystem size in that
> environment.
>
> Now that it has moved to use /tmp instead of /dev/shm files, enable it.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 3/4] tests/qtest/migration-test: Fix and enable test_ignore_shared

[Nicholas Piggin]

This test is already starting to bitrot, so first remove it from ifdef
and fix compile issues. ppc64 transfers about 2MB, so bump the size
threshold too.

It was said to be broken on aarch64 but it may have been due to the
limited shm size under Gitlab CI. Now that it uses /tmp, enable it.

Cc: Yury Kotov <yury-kotov@yandex-team.ru>
Cc: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/migration-test.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index de380757be..86eace354e 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1855,8 +1855,6 @@ static void test_precopy_unix_tls_x509_override_host(void)
 #endif /* CONFIG_TASN1 */
 #endif /* CONFIG_GNUTLS */
 
-#if 0
-/* Currently upset on aarch64 TCG */
 static void test_ignore_shared(void)
 {
     g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
@@ -1865,7 +1863,7 @@ static void test_ignore_shared(void)
         .use_memfile = true,
     };
 
-    if (test_migrate_start(&from, &to, uri, false, true, NULL, NULL)) {
+    if (test_migrate_start(&from, &to, uri, &args)) {
         return;
     }
 
@@ -1890,11 +1888,11 @@ static void test_ignore_shared(void)
     wait_for_migration_complete(from);
 
     /* Check whether shared RAM has been really skipped */
-    g_assert_cmpint(read_ram_property_int(from, "transferred"), <, 1024 * 1024);
+    g_assert_cmpint(read_ram_property_int(from, "transferred"), <,
+                                                   4 * 1024 * 1024);
 
     test_migrate_end(from, to, true);
 }
-#endif
 
 static void *
 test_migrate_xbzrle_start(QTestState *from,
@@ -3535,7 +3533,8 @@ int main(int argc, char **argv)
 #endif /* CONFIG_TASN1 */
 #endif /* CONFIG_GNUTLS */
 
-    /* migration_test_add("/migration/ignore_shared", test_ignore_shared); */
+    migration_test_add("/migration/ignore_shared", test_ignore_shared);
+
 #ifndef _WIN32
     migration_test_add("/migration/precopy/fd/tcp",
                        test_migrate_precopy_fd_socket);
-- 
2.43.0

Re: [PATCH v2 3/4] tests/qtest/migration-test: Fix and enable test_ignore_shared

[Dr. David Alan Gilbert]

* Nicholas Piggin (npiggin@gmail.com) wrote:
> This test is already starting to bitrot, so first remove it from ifdef
> and fix compile issues. ppc64 transfers about 2MB, so bump the size
> threshold too.
> 
> It was said to be broken on aarch64 but it may have been due to the
> limited shm size under Gitlab CI. Now that it uses /tmp, enable it.

If it does fail, lets see if we can figure out how, i.e. whether it's the
shm size or something else.

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> Cc: Yury Kotov <yury-kotov@yandex-team.ru>
> Cc: Dr. David Alan Gilbert <dave@treblig.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  tests/qtest/migration-test.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index de380757be..86eace354e 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1855,8 +1855,6 @@ static void test_precopy_unix_tls_x509_override_host(void)
>  #endif /* CONFIG_TASN1 */
>  #endif /* CONFIG_GNUTLS */
>  
> -#if 0
> -/* Currently upset on aarch64 TCG */
>  static void test_ignore_shared(void)
>  {
>      g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> @@ -1865,7 +1863,7 @@ static void test_ignore_shared(void)
>          .use_memfile = true,
>      };
>  
> -    if (test_migrate_start(&from, &to, uri, false, true, NULL, NULL)) {
> +    if (test_migrate_start(&from, &to, uri, &args)) {
>          return;
>      }
>  
> @@ -1890,11 +1888,11 @@ static void test_ignore_shared(void)
>      wait_for_migration_complete(from);
>  
>      /* Check whether shared RAM has been really skipped */
> -    g_assert_cmpint(read_ram_property_int(from, "transferred"), <, 1024 * 1024);
> +    g_assert_cmpint(read_ram_property_int(from, "transferred"), <,
> +                                                   4 * 1024 * 1024);
>  
>      test_migrate_end(from, to, true);
>  }
> -#endif
>  
>  static void *
>  test_migrate_xbzrle_start(QTestState *from,
> @@ -3535,7 +3533,8 @@ int main(int argc, char **argv)
>  #endif /* CONFIG_TASN1 */
>  #endif /* CONFIG_GNUTLS */
>  
> -    /* migration_test_add("/migration/ignore_shared", test_ignore_shared); */
> +    migration_test_add("/migration/ignore_shared", test_ignore_shared);
> +
>  #ifndef _WIN32
>      migration_test_add("/migration/precopy/fd/tcp",
>                         test_migrate_precopy_fd_socket);
> -- 
> 2.43.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH v2 3/4] tests/qtest/migration-test: Fix and enable test_ignore_shared

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> This test is already starting to bitrot, so first remove it from ifdef
> and fix compile issues. ppc64 transfers about 2MB, so bump the size
> threshold too.
>
> It was said to be broken on aarch64 but it may have been due to the
> limited shm size under Gitlab CI. Now that it uses /tmp, enable it.
>
> Cc: Yury Kotov <yury-kotov@yandex-team.ru>
> Cc: Dr. David Alan Gilbert <dave@treblig.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  tests/qtest/migration-test.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index de380757be..86eace354e 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1855,8 +1855,6 @@ static void test_precopy_unix_tls_x509_override_host(void)
>  #endif /* CONFIG_TASN1 */
>  #endif /* CONFIG_GNUTLS */
>  
> -#if 0
> -/* Currently upset on aarch64 TCG */
>  static void test_ignore_shared(void)
>  {
>      g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> @@ -1865,7 +1863,7 @@ static void test_ignore_shared(void)
>          .use_memfile = true,
>      };
>  
> -    if (test_migrate_start(&from, &to, uri, false, true, NULL, NULL)) {
> +    if (test_migrate_start(&from, &to, uri, &args)) {
>          return;
>      }
>  
> @@ -1890,11 +1888,11 @@ static void test_ignore_shared(void)
>      wait_for_migration_complete(from);
>  
>      /* Check whether shared RAM has been really skipped */
> -    g_assert_cmpint(read_ram_property_int(from, "transferred"), <, 1024 * 1024);
> +    g_assert_cmpint(read_ram_property_int(from, "transferred"), <,
> +                                                   4 * 1024 * 1024);
>  
>      test_migrate_end(from, to, true);
>  }
> -#endif
>  
>  static void *
>  test_migrate_xbzrle_start(QTestState *from,
> @@ -3535,7 +3533,8 @@ int main(int argc, char **argv)
>  #endif /* CONFIG_TASN1 */
>  #endif /* CONFIG_GNUTLS */
>  
> -    /* migration_test_add("/migration/ignore_shared", test_ignore_shared); */
> +    migration_test_add("/migration/ignore_shared", test_ignore_shared);
> +
>  #ifndef _WIN32
>      migration_test_add("/migration/precopy/fd/tcp",
>                         test_migrate_precopy_fd_socket);

I'm queuing this patch to qtest-next, but moving it under
QEMU_TEST_FLAKY_TESTS. Otherwise the move of this function to another
file (from the refactoring series) breaks checkpatch due to the #if 0.

[PATCH v2 4/4] tests/qtest/migration-test: Add a postcopy memfile test

[Nicholas Piggin]

Postcopy requires userfaultfd support, which requires tmpfs if a memory
file is used.

This adds back support for /dev/shm memory files, but adds preallocation
to skip environments where that mount is limited in size.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/migration-test.c | 77 ++++++++++++++++++++++++++++++++----
 1 file changed, 69 insertions(+), 8 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 86eace354e..5078033ded 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 
 #include "libqtest.h"
 #include "qapi/qmp/qdict.h"
@@ -553,6 +554,7 @@ typedef struct {
      */
     bool hide_stderr;
     bool use_memfile;
+    bool use_shm_memfile;
     /* only launch the target process */
     bool only_target;
     /* Use dirty ring if true; dirty logging otherwise */
@@ -739,7 +741,62 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         ignore_stderr = "";
     }
 
-    if (args->use_memfile) {
+    if (!qtest_has_machine(machine_alias)) {
+        g_autofree char *msg = g_strdup_printf("machine %s not supported",
+                                               machine_alias);
+        g_test_skip(msg);
+        return -1;
+    }
+
+    if (args->use_shm_memfile) {
+#if defined(__NR_userfaultfd) && defined(__linux__)
+        int fd;
+        uint64_t size;
+
+        if (getenv("GITLAB_CI")) {
+            /*
+             * Gitlab runners are limited to 64MB shm size and despite
+             * pre-allocation there is concern that concurrent tests
+             * could result in nondeterministic failures. Until all shm
+             * usage in all CI tests is found to fail gracefully on
+             * ENOSPC, it is safer to avoid large allocations for now.
+             *
+             * https://lore.kernel.org/qemu-devel/875xuwg4mx.fsf@suse.de/
+             */
+            g_test_skip("shm tests are not supported in Gitlab CI environment");
+            return -1;
+        }
+
+        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
+            g_test_skip("/dev/shm does not exist or is not a directory");
+            return -1;
+        }
+
+        /*
+         * Pre-create and allocate the file here, because /dev/shm/
+         * is known to be limited in size in some places (e.g., Gitlab CI).
+         */
+        memfile_path = g_strdup_printf("/dev/shm/qemu-%d", getpid());
+        fd = open(memfile_path, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
+        if (fd == -1) {
+            g_test_skip("/dev/shm file could not be created");
+            return -1;
+        }
+
+        g_assert(qemu_strtosz(memory_size, NULL, &size) == 0);
+        size += 64*1024; /* QEMU may map a bit more memory for a guard page */
+
+        if (fallocate(fd, 0, 0, size) == -1) {
+            unlink(memfile_path);
+            perror("could not alloc"); exit(1);
+            g_test_skip("Could not allocate machine memory in /dev/shm");
+            return -1;
+        }
+        close(fd);
+#else
+        g_test_skip("userfaultfd is not supported");
+#endif
+    } else if (args->use_memfile) {
         memfile_path = g_strdup_printf("/%s/qemu-%d", tmpfs, getpid());
         memfile_opts = g_strdup_printf(
             "-object memory-backend-file,id=mem0,size=%s"
@@ -751,12 +808,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         kvm_opts = ",dirty-ring-size=4096";
     }
 
-    if (!qtest_has_machine(machine_alias)) {
-        g_autofree char *msg = g_strdup_printf("machine %s not supported", machine_alias);
-        g_test_skip(msg);
-        return -1;
-    }
-
     machine = resolve_machine_version(machine_alias, QEMU_ENV_SRC,
                                       QEMU_ENV_DST);
 
@@ -807,7 +858,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
      * Remove shmem file immediately to avoid memory leak in test failed case.
      * It's valid because QEMU has already opened this file
      */
-    if (args->use_memfile) {
+    if (args->use_memfile || args->use_shm_memfile) {
         unlink(memfile_path);
     }
 
@@ -1275,6 +1326,15 @@ static void test_postcopy(void)
     test_postcopy_common(&args);
 }
 
+static void test_postcopy_memfile(void)
+{
+    MigrateCommon args = {
+        .start.use_shm_memfile = true,
+    };
+
+    test_postcopy_common(&args);
+}
+
 static void test_postcopy_suspend(void)
 {
     MigrateCommon args = {
@@ -3441,6 +3501,7 @@ int main(int argc, char **argv)
 
     if (has_uffd) {
         migration_test_add("/migration/postcopy/plain", test_postcopy);
+        migration_test_add("/migration/postcopy/memfile", test_postcopy_memfile);
         migration_test_add("/migration/postcopy/recovery/plain",
                            test_postcopy_recovery);
         migration_test_add("/migration/postcopy/preempt/plain",
-- 
2.43.0

Re: [PATCH v2 4/4] tests/qtest/migration-test: Add a postcopy memfile test

[Peter Xu]

On Thu, May 30, 2024 at 07:54:07PM +1000, Nicholas Piggin wrote:
> Postcopy requires userfaultfd support, which requires tmpfs if a memory
> file is used.
> 
> This adds back support for /dev/shm memory files, but adds preallocation
> to skip environments where that mount is limited in size.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Thanks for doing this regardless.

> ---
>  tests/qtest/migration-test.c | 77 ++++++++++++++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 86eace354e..5078033ded 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  
>  #include "libqtest.h"
>  #include "qapi/qmp/qdict.h"
> @@ -553,6 +554,7 @@ typedef struct {
>       */
>      bool hide_stderr;
>      bool use_memfile;
> +    bool use_shm_memfile;

Nitpick: when having both, it's slightly confusing on the name, e.g. not
clear whether use_memfile needs to be set to true too if use_shm_memfile=true.

Maybe "use_tmpfs_memfile" and "use_shm_memfile"?

>      /* only launch the target process */
>      bool only_target;
>      /* Use dirty ring if true; dirty logging otherwise */
> @@ -739,7 +741,62 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>          ignore_stderr = "";
>      }
>  
> -    if (args->use_memfile) {
> +    if (!qtest_has_machine(machine_alias)) {
> +        g_autofree char *msg = g_strdup_printf("machine %s not supported",
> +                                               machine_alias);
> +        g_test_skip(msg);
> +        return -1;
> +    }
> +
> +    if (args->use_shm_memfile) {
> +#if defined(__NR_userfaultfd) && defined(__linux__)

IIUC we only need defined(__linux__) as there's nothing to do with uffd yet?

> +        int fd;
> +        uint64_t size;
> +
> +        if (getenv("GITLAB_CI")) {
> +            /*
> +             * Gitlab runners are limited to 64MB shm size and despite
> +             * pre-allocation there is concern that concurrent tests
> +             * could result in nondeterministic failures. Until all shm
> +             * usage in all CI tests is found to fail gracefully on
> +             * ENOSPC, it is safer to avoid large allocations for now.
> +             *
> +             * https://lore.kernel.org/qemu-devel/875xuwg4mx.fsf@suse.de/
> +             */
> +            g_test_skip("shm tests are not supported in Gitlab CI environment");
> +            return -1;
> +        }

I'm not sure whether this is Fabiano's intention.  I'm wondering whether we
can drop this and just let it still run there.

Other tests not detecting avaiablility of shmem looks like a separate issue
to be fixed to me, regardless of this.

My wild guess is since we're doing memory_size+64K below then if test won't
fail others won't either, as normally the shmem quota should normally be
power of 2 anyway.. then it should always fit another few MBs if this one.
While this test is ready to fail gracefully now.

> +
> +        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> +            g_test_skip("/dev/shm does not exist or is not a directory");
> +            return -1;
> +        }
> +
> +        /*
> +         * Pre-create and allocate the file here, because /dev/shm/
> +         * is known to be limited in size in some places (e.g., Gitlab CI).
> +         */
> +        memfile_path = g_strdup_printf("/dev/shm/qemu-%d", getpid());
> +        fd = open(memfile_path, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
> +        if (fd == -1) {
> +            g_test_skip("/dev/shm file could not be created");
> +            return -1;
> +        }
> +
> +        g_assert(qemu_strtosz(memory_size, NULL, &size) == 0);
> +        size += 64*1024; /* QEMU may map a bit more memory for a guard page */
> +
> +        if (fallocate(fd, 0, 0, size) == -1) {
> +            unlink(memfile_path);
> +            perror("could not alloc"); exit(1);
> +            g_test_skip("Could not allocate machine memory in /dev/shm");
> +            return -1;
> +        }
> +        close(fd);
> +#else
> +        g_test_skip("userfaultfd is not supported");

"/dev/shm not available" instead?

> +#endif
> +    } else if (args->use_memfile) {
>          memfile_path = g_strdup_printf("/%s/qemu-%d", tmpfs, getpid());
>          memfile_opts = g_strdup_printf(
>              "-object memory-backend-file,id=mem0,size=%s"
> @@ -751,12 +808,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>          kvm_opts = ",dirty-ring-size=4096";
>      }
>  
> -    if (!qtest_has_machine(machine_alias)) {
> -        g_autofree char *msg = g_strdup_printf("machine %s not supported", machine_alias);
> -        g_test_skip(msg);
> -        return -1;
> -    }
> -
>      machine = resolve_machine_version(machine_alias, QEMU_ENV_SRC,
>                                        QEMU_ENV_DST);
>  
> @@ -807,7 +858,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       * Remove shmem file immediately to avoid memory leak in test failed case.
>       * It's valid because QEMU has already opened this file
>       */
> -    if (args->use_memfile) {
> +    if (args->use_memfile || args->use_shm_memfile) {
>          unlink(memfile_path);
>      }
>  
> @@ -1275,6 +1326,15 @@ static void test_postcopy(void)
>      test_postcopy_common(&args);
>  }
>  
> +static void test_postcopy_memfile(void)
> +{

IMHO the defined(__NR_userfaultfd) should be here to guard if needed.

Or rather, we don't need to care about uffd yet? As what we already do with
test_postcopy().

I'm guessing the test just doesn't run on !linux, while compilation always
works with/without that.

Thanks,

> +    MigrateCommon args = {
> +        .start.use_shm_memfile = true,
> +    };
> +
> +    test_postcopy_common(&args);
> +}
> +
>  static void test_postcopy_suspend(void)
>  {
>      MigrateCommon args = {
> @@ -3441,6 +3501,7 @@ int main(int argc, char **argv)
>  
>      if (has_uffd) {
>          migration_test_add("/migration/postcopy/plain", test_postcopy);
> +        migration_test_add("/migration/postcopy/memfile", test_postcopy_memfile);
>          migration_test_add("/migration/postcopy/recovery/plain",
>                             test_postcopy_recovery);
>          migration_test_add("/migration/postcopy/preempt/plain",
> -- 
> 2.43.0
> 

-- 
Peter Xu

Re: [PATCH v2 4/4] tests/qtest/migration-test: Add a postcopy memfile test

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 30, 2024 at 07:54:07PM +1000, Nicholas Piggin wrote:
>> Postcopy requires userfaultfd support, which requires tmpfs if a memory
>> file is used.
>> 
>> This adds back support for /dev/shm memory files, but adds preallocation
>> to skip environments where that mount is limited in size.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
> Thanks for doing this regardless.
>
>> ---
>>  tests/qtest/migration-test.c | 77 ++++++++++++++++++++++++++++++++----
>>  1 file changed, 69 insertions(+), 8 deletions(-)
>> 
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 86eace354e..5078033ded 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -11,6 +11,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>>  
>>  #include "libqtest.h"
>>  #include "qapi/qmp/qdict.h"
>> @@ -553,6 +554,7 @@ typedef struct {
>>       */
>>      bool hide_stderr;
>>      bool use_memfile;
>> +    bool use_shm_memfile;
>
> Nitpick: when having both, it's slightly confusing on the name, e.g. not
> clear whether use_memfile needs to be set to true too if use_shm_memfile=true.
>
> Maybe "use_tmpfs_memfile" and "use_shm_memfile"?
>
>>      /* only launch the target process */
>>      bool only_target;
>>      /* Use dirty ring if true; dirty logging otherwise */
>> @@ -739,7 +741,62 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>          ignore_stderr = "";
>>      }
>>  
>> -    if (args->use_memfile) {
>> +    if (!qtest_has_machine(machine_alias)) {
>> +        g_autofree char *msg = g_strdup_printf("machine %s not supported",
>> +                                               machine_alias);
>> +        g_test_skip(msg);
>> +        return -1;
>> +    }
>> +
>> +    if (args->use_shm_memfile) {
>> +#if defined(__NR_userfaultfd) && defined(__linux__)
>
> IIUC we only need defined(__linux__) as there's nothing to do with uffd yet?
>
>> +        int fd;
>> +        uint64_t size;
>> +
>> +        if (getenv("GITLAB_CI")) {
>> +            /*
>> +             * Gitlab runners are limited to 64MB shm size and despite
>> +             * pre-allocation there is concern that concurrent tests
>> +             * could result in nondeterministic failures. Until all shm
>> +             * usage in all CI tests is found to fail gracefully on
>> +             * ENOSPC, it is safer to avoid large allocations for now.
>> +             *
>> +             * https://lore.kernel.org/qemu-devel/875xuwg4mx.fsf@suse.de/
>> +             */
>> +            g_test_skip("shm tests are not supported in Gitlab CI environment");
>> +            return -1;
>> +        }
>
> I'm not sure whether this is Fabiano's intention.  I'm wondering whether we
> can drop this and just let it still run there.
>

It was my intention. But I overlooked the fact that the current shm
cannot even run one migration test already.

> Other tests not detecting avaiablility of shmem looks like a separate issue
> to be fixed to me, regardless of this.
>
> My wild guess is since we're doing memory_size+64K below then if test won't
> fail others won't either, as normally the shmem quota should normally be
> power of 2 anyway.. then it should always fit another few MBs if this one.
> While this test is ready to fail gracefully now.
>

I agree. Let's drop this part then.

Re: [PATCH v2 4/4] tests/qtest/migration-test: Add a postcopy memfile test

[Nicholas Piggin]

On Fri May 31, 2024 at 11:34 PM AEST, Peter Xu wrote:
> On Thu, May 30, 2024 at 07:54:07PM +1000, Nicholas Piggin wrote:
> > Postcopy requires userfaultfd support, which requires tmpfs if a memory
> > file is used.
> > 
> > This adds back support for /dev/shm memory files, but adds preallocation
> > to skip environments where that mount is limited in size.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
> Thanks for doing this regardless.
>
> > ---
> >  tests/qtest/migration-test.c | 77 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 69 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 86eace354e..5078033ded 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -11,6 +11,7 @@
> >   */
> >  
> >  #include "qemu/osdep.h"
> > +#include "qemu/cutils.h"
> >  
> >  #include "libqtest.h"
> >  #include "qapi/qmp/qdict.h"
> > @@ -553,6 +554,7 @@ typedef struct {
> >       */
> >      bool hide_stderr;
> >      bool use_memfile;
> > +    bool use_shm_memfile;
>
> Nitpick: when having both, it's slightly confusing on the name, e.g. not
> clear whether use_memfile needs to be set to true too if use_shm_memfile=true.
>
> Maybe "use_tmpfs_memfile" and "use_shm_memfile"?

Could be easy to confuse. It's not actually "tmpfs", it is the fs that
is mounted on /tmp :) tmpfs *is* shmfs in Linux. The intention was just
that if you don't specify then it's because you don't have a particular
requirement other than enough space.

> >      /* only launch the target process */
> >      bool only_target;
> >      /* Use dirty ring if true; dirty logging otherwise */
> > @@ -739,7 +741,62 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >          ignore_stderr = "";
> >      }
> >  
> > -    if (args->use_memfile) {
> > +    if (!qtest_has_machine(machine_alias)) {
> > +        g_autofree char *msg = g_strdup_printf("machine %s not supported",
> > +                                               machine_alias);
> > +        g_test_skip(msg);
> > +        return -1;
> > +    }
> > +
> > +    if (args->use_shm_memfile) {
> > +#if defined(__NR_userfaultfd) && defined(__linux__)
>
> IIUC we only need defined(__linux__) as there's nothing to do with uffd yet?

I thought it was polite since it uses a few other Linux (or at least
POSIX) calls directly rather than go via the abstraction layer. Probably
it would never happen that something defines __NR_userfaultfd that does
not also have open and fallocate, but no harm?

> > +        int fd;
> > +        uint64_t size;
> > +
> > +        if (getenv("GITLAB_CI")) {
> > +            /*
> > +             * Gitlab runners are limited to 64MB shm size and despite
> > +             * pre-allocation there is concern that concurrent tests
> > +             * could result in nondeterministic failures. Until all shm
> > +             * usage in all CI tests is found to fail gracefully on
> > +             * ENOSPC, it is safer to avoid large allocations for now.
> > +             *
> > +             * https://lore.kernel.org/qemu-devel/875xuwg4mx.fsf@suse.de/
> > +             */
> > +            g_test_skip("shm tests are not supported in Gitlab CI environment");
> > +            return -1;
> > +        }
>
> I'm not sure whether this is Fabiano's intention.  I'm wondering whether we
> can drop this and just let it still run there.
>
> Other tests not detecting avaiablility of shmem looks like a separate issue
> to be fixed to me, regardless of this.
>
> My wild guess is since we're doing memory_size+64K below then if test won't
> fail others won't either, as normally the shmem quota should normally be
> power of 2 anyway.. then it should always fit another few MBs if this one.
> While this test is ready to fail gracefully now.

Well if CI runners got upgraded to 256MB shm space under us, we might
start to succeed? I _think_ fallocate on tmpfs should be relatively
atomic in that it doesn't try to allocate space if the fs was too small,
but at least naive implementations can do it too, so I didn't want to
rely on it.

I'm fine to do what you and Fabiano prefer. If we commit this then later
remove this hunk, it would be easy to revert if it started to show up
fails. OTOH not too hard to add it in later either.

> > +
> > +        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> > +            g_test_skip("/dev/shm does not exist or is not a directory");
> > +            return -1;
> > +        }
> > +
> > +        /*
> > +         * Pre-create and allocate the file here, because /dev/shm/
> > +         * is known to be limited in size in some places (e.g., Gitlab CI).
> > +         */
> > +        memfile_path = g_strdup_printf("/dev/shm/qemu-%d", getpid());
> > +        fd = open(memfile_path, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
> > +        if (fd == -1) {
> > +            g_test_skip("/dev/shm file could not be created");
> > +            return -1;
> > +        }
> > +
> > +        g_assert(qemu_strtosz(memory_size, NULL, &size) == 0);
> > +        size += 64*1024; /* QEMU may map a bit more memory for a guard page */
> > +
> > +        if (fallocate(fd, 0, 0, size) == -1) {
> > +            unlink(memfile_path);
> > +            perror("could not alloc"); exit(1);
> > +            g_test_skip("Could not allocate machine memory in /dev/shm");
> > +            return -1;
> > +        }
> > +        close(fd);
> > +#else
> > +        g_test_skip("userfaultfd is not supported");
>
> "/dev/shm not available" instead?

Ah yes, that was for the userfaultfd not supported case, but indeed
since renaming it from uffd to shm, it is better to move the ifdef
guard to where you say.

> > +#endif
> > +    } else if (args->use_memfile) {
> >          memfile_path = g_strdup_printf("/%s/qemu-%d", tmpfs, getpid());
> >          memfile_opts = g_strdup_printf(
> >              "-object memory-backend-file,id=mem0,size=%s"
> > @@ -751,12 +808,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >          kvm_opts = ",dirty-ring-size=4096";
> >      }
> >  
> > -    if (!qtest_has_machine(machine_alias)) {
> > -        g_autofree char *msg = g_strdup_printf("machine %s not supported", machine_alias);
> > -        g_test_skip(msg);
> > -        return -1;
> > -    }
> > -
> >      machine = resolve_machine_version(machine_alias, QEMU_ENV_SRC,
> >                                        QEMU_ENV_DST);
> >  
> > @@ -807,7 +858,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >       * Remove shmem file immediately to avoid memory leak in test failed case.
> >       * It's valid because QEMU has already opened this file
> >       */
> > -    if (args->use_memfile) {
> > +    if (args->use_memfile || args->use_shm_memfile) {
> >          unlink(memfile_path);
> >      }
> >  
> > @@ -1275,6 +1326,15 @@ static void test_postcopy(void)
> >      test_postcopy_common(&args);
> >  }
> >  
> > +static void test_postcopy_memfile(void)
> > +{
>
> IMHO the defined(__NR_userfaultfd) should be here to guard if needed.
>
> Or rather, we don't need to care about uffd yet? As what we already do with
> test_postcopy().
>
> I'm guessing the test just doesn't run on !linux, while compilation always
> works with/without that.

Yeah they fall under has_uffd, so no ifdef required.

Thanks,
Nick

Re: [PATCH v2 4/4] tests/qtest/migration-test: Add a postcopy memfile test

[Peter Xu]

On Mon, Jun 03, 2024 at 04:02:42PM +1000, Nicholas Piggin wrote:
> On Fri May 31, 2024 at 11:34 PM AEST, Peter Xu wrote:
> > On Thu, May 30, 2024 at 07:54:07PM +1000, Nicholas Piggin wrote:
> > > Postcopy requires userfaultfd support, which requires tmpfs if a memory
> > > file is used.
> > > 
> > > This adds back support for /dev/shm memory files, but adds preallocation
> > > to skip environments where that mount is limited in size.
> > > 
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >
> > Thanks for doing this regardless.
> >
> > > ---
> > >  tests/qtest/migration-test.c | 77 ++++++++++++++++++++++++++++++++----
> > >  1 file changed, 69 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > > index 86eace354e..5078033ded 100644
> > > --- a/tests/qtest/migration-test.c
> > > +++ b/tests/qtest/migration-test.c
> > > @@ -11,6 +11,7 @@
> > >   */
> > >  
> > >  #include "qemu/osdep.h"
> > > +#include "qemu/cutils.h"
> > >  
> > >  #include "libqtest.h"
> > >  #include "qapi/qmp/qdict.h"
> > > @@ -553,6 +554,7 @@ typedef struct {
> > >       */
> > >      bool hide_stderr;
> > >      bool use_memfile;
> > > +    bool use_shm_memfile;
> >
> > Nitpick: when having both, it's slightly confusing on the name, e.g. not
> > clear whether use_memfile needs to be set to true too if use_shm_memfile=true.
> >
> > Maybe "use_tmpfs_memfile" and "use_shm_memfile"?
> 
> Could be easy to confuse. It's not actually "tmpfs", it is the fs that
> is mounted on /tmp :) tmpfs *is* shmfs in Linux. The intention was just
> that if you don't specify then it's because you don't have a particular
> requirement other than enough space.

Ah sorry, yeah I meant use_tmp_memfile..

> 
> > >      /* only launch the target process */
> > >      bool only_target;
> > >      /* Use dirty ring if true; dirty logging otherwise */
> > > @@ -739,7 +741,62 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> > >          ignore_stderr = "";
> > >      }
> > >  
> > > -    if (args->use_memfile) {
> > > +    if (!qtest_has_machine(machine_alias)) {
> > > +        g_autofree char *msg = g_strdup_printf("machine %s not supported",
> > > +                                               machine_alias);
> > > +        g_test_skip(msg);
> > > +        return -1;
> > > +    }
> > > +
> > > +    if (args->use_shm_memfile) {
> > > +#if defined(__NR_userfaultfd) && defined(__linux__)
> >
> > IIUC we only need defined(__linux__) as there's nothing to do with uffd yet?
> 
> I thought it was polite since it uses a few other Linux (or at least
> POSIX) calls directly rather than go via the abstraction layer. Probably
> it would never happen that something defines __NR_userfaultfd that does
> not also have open and fallocate, but no harm?

It's about when there're shmem tests added without postcopy, we may want
the host to run these tests even if that host doesn't support userfault
syscall.

$ grep -iE "(SHMEM|USERFAULTFD)=" .config
CONFIG_SHMEM=y
CONFIG_USERFAULTFD=y

So I want to make sure the test runs the right thing always, irrelevant of
which arch it ran on, or kernel config.

I agree that's not a huge deal, but still I wanted to remove the
collreation that userfault and shmem is closely related - they're just
totally irrelevant to me, e.g., we can have shmem test/hostconfig without
userfault, we can also have userfault test/hostconfig without shmem.

> 
> > > +        int fd;
> > > +        uint64_t size;
> > > +
> > > +        if (getenv("GITLAB_CI")) {
> > > +            /*
> > > +             * Gitlab runners are limited to 64MB shm size and despite
> > > +             * pre-allocation there is concern that concurrent tests
> > > +             * could result in nondeterministic failures. Until all shm
> > > +             * usage in all CI tests is found to fail gracefully on
> > > +             * ENOSPC, it is safer to avoid large allocations for now.
> > > +             *
> > > +             * https://lore.kernel.org/qemu-devel/875xuwg4mx.fsf@suse.de/
> > > +             */
> > > +            g_test_skip("shm tests are not supported in Gitlab CI environment");
> > > +            return -1;
> > > +        }
> >
> > I'm not sure whether this is Fabiano's intention.  I'm wondering whether we
> > can drop this and just let it still run there.
> >
> > Other tests not detecting avaiablility of shmem looks like a separate issue
> > to be fixed to me, regardless of this.
> >
> > My wild guess is since we're doing memory_size+64K below then if test won't
> > fail others won't either, as normally the shmem quota should normally be
> > power of 2 anyway.. then it should always fit another few MBs if this one.
> > While this test is ready to fail gracefully now.
> 
> Well if CI runners got upgraded to 256MB shm space under us, we might
> start to succeed? I _think_ fallocate on tmpfs should be relatively

Yep, and IMHO that's also why I think we can start trying this even on
GITLAB_CI if it can support it.

> atomic in that it doesn't try to allocate space if the fs was too small,
> but at least naive implementations can do it too, so I didn't want to
> rely on it.
> 
> I'm fine to do what you and Fabiano prefer. If we commit this then later
> remove this hunk, it would be easy to revert if it started to show up
> fails. OTOH not too hard to add it in later either.

The question is why it can fail even with fallocate() around.  I just don't
see how it can fail besides either succeed or skip.

Maybe I missed something alone the lines.. though.

The other test (e.g. ivshmem) can fail, but those can fail with/without
the change here.  That's also why I think we need to fix them separately,
rather than trying to detect GITLAB_CI only because we know they _might_ be
running concurrently - if all tests have protection over shmem allocation
then IIUC we shouldn't fail anymore, but only success/skip.

I agree this doesn't look good at all, as it's not predictable on what test
will be covered in a CI run.. but again IMHO that's yet another separate
issue on how we can make CI support enough shmem usages.

Thanks,

-- 
Peter Xu