| Message ID | 20240528042758.621589-1-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest/migrate-test: Use regular file file for shared-memory tests | expand |
On Tue, May 28, 2024 at 02:27:57PM +1000, Nicholas Piggin wrote: > There is no need to use /dev/shm for file-backed memory devices, and > it is too small to be usable in gitlab CI. Switch to using a regular > file in /tmp/ which will usually have more space available. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > Am I missing something? AFAIKS there is not even any point using > /dev/shm aka tmpfs anyway, there is not much special about it as a > filesystem. This applies on top of the series just sent, and passes > gitlab CI qtests including aarch64. I think it's just that /dev/shm guarantees shmem usage, while the var "tmpfs" implies g_dir_make_tmp() which may be another non-ram based file system, while that'll be slightly different comparing to what a real user would use - we don't suggest user to put guest RAM on things like btrfs. One real implication is if we add a postcopy test it'll fail with g_dir_make_tmp() when it is not pointing to a shmem mount, as UFFDIO_REGISTER will fail there. But that test doesn't yet exist as the QEMU paths should be the same even if Linux will trigger different paths when different types of mem is used (anonymous v.s. shmem). If the goal here is to properly handle the case where tmpfs doesn't have enough space, how about what I suggested in the other email? https://lore.kernel.org/r/ZlSppKDE6wzjCF--@x1n IOW, try populate the shmem region before starting the guest, skip if population failed. Would that work? Thanks, > > Thanks, > Nick > > tests/qtest/migration-test.c | 41 ++++++++++++------------------------ > 1 file changed, 13 insertions(+), 28 deletions(-) > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 45830eb213..86eace354e 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,29 +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; > - } > - if (getenv("GITLAB_CI")) { > - /* > - * Gitlab runners are limited to 64MB shm size. See: > - * https://lore.kernel.org/all/87ttq5fvh7.fsf@suse.de/ > - */ > - g_test_skip("/dev/shm is not supported in Gitlab CI environment"); > - return -1; > - } > - } > - > dst_state = (QTestMigrationState) { }; > src_state = (QTestMigrationState) { }; > bootfile_create(tmpfs, args->suspend_me); > @@ -754,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) { > @@ -788,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) { > @@ -810,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); > @@ -822,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; > @@ -1875,7 +1860,7 @@ static void test_ignore_shared(void) > g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); > QTestState *from, *to; > MigrateStart args = { > - .use_shmem = true, > + .use_memfile = true, > }; > > if (test_migrate_start(&from, &to, uri, &args)) { > @@ -2033,7 +2018,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 >
Nicholas Piggin <npiggin@gmail.com> writes: > There is no need to use /dev/shm for file-backed memory devices, and > it is too small to be usable in gitlab CI. Switch to using a regular > file in /tmp/ which will usually have more space available. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > Am I missing something? AFAIKS there is not even any point using > /dev/shm aka tmpfs anyway, there is not much special about it as a > filesystem. This applies on top of the series just sent, and passes > gitlab CI qtests including aarch64. /dev/shm however will be mounted on tmpfs while /tmp may not. I don't know if this has any implication to this test. Probably not. > > Thanks, > Nick > > tests/qtest/migration-test.c | 41 ++++++++++++------------------------ > 1 file changed, 13 insertions(+), 28 deletions(-) > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 45830eb213..86eace354e 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,29 +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; > - } > - if (getenv("GITLAB_CI")) { > - /* > - * Gitlab runners are limited to 64MB shm size. See: > - * https://lore.kernel.org/all/87ttq5fvh7.fsf@suse.de/ > - */ > - g_test_skip("/dev/shm is not supported in Gitlab CI environment"); > - return -1; > - } > - } > - > dst_state = (QTestMigrationState) { }; > src_state = (QTestMigrationState) { }; > bootfile_create(tmpfs, args->suspend_me); > @@ -754,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()); The variable tmpfs already contains the leading slash. Strictly speaking we don't need the pid because 'tmpfs' is unique for each migration-test run. If you use a fixed string such as qemu-mem, you can then clean it up at test_migrate_end() along with the others. > + 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) { > @@ -788,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) { > @@ -810,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); > @@ -822,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 > */ I'm not sure what memory leak this referred to. When a test fails anywhere outside test_migrate_end(), the /tmp/migration-test-XXXX directory will stay behind with all the files used during the test. We probably don't need the special case for this one file. $ ls /tmp/migration-test-*/ /tmp/migration-test-GFO6N2/: tlscredsx5090 /tmp/migration-test-QH2MO2/: bootsect dest_serial src_serial > - if (args->use_shmem) { > - unlink(shmem_path); > + if (args->use_memfile) { > + unlink(memfile_path); > } > > return 0; > @@ -1875,7 +1860,7 @@ static void test_ignore_shared(void) > g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); > QTestState *from, *to; > MigrateStart args = { > - .use_shmem = true, > + .use_memfile = true, > }; > > if (test_migrate_start(&from, &to, uri, &args)) { > @@ -2033,7 +2018,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
On Tue, May 28, 2024 at 09:35:22AM -0400, Peter Xu wrote: > On Tue, May 28, 2024 at 02:27:57PM +1000, Nicholas Piggin wrote: > > There is no need to use /dev/shm for file-backed memory devices, and > > it is too small to be usable in gitlab CI. Switch to using a regular > > file in /tmp/ which will usually have more space available. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > Am I missing something? AFAIKS there is not even any point using > > /dev/shm aka tmpfs anyway, there is not much special about it as a > > filesystem. This applies on top of the series just sent, and passes > > gitlab CI qtests including aarch64. > > I think it's just that /dev/shm guarantees shmem usage, while the var > "tmpfs" implies g_dir_make_tmp() which may be another non-ram based file > system, while that'll be slightly different comparing to what a real user > would use - we don't suggest user to put guest RAM on things like btrfs. > > One real implication is if we add a postcopy test it'll fail with > g_dir_make_tmp() when it is not pointing to a shmem mount, as > UFFDIO_REGISTER will fail there. But that test doesn't yet exist as the > QEMU paths should be the same even if Linux will trigger different paths > when different types of mem is used (anonymous v.s. shmem). > > If the goal here is to properly handle the case where tmpfs doesn't have > enough space, how about what I suggested in the other email? > > https://lore.kernel.org/r/ZlSppKDE6wzjCF--@x1n > > IOW, try populate the shmem region before starting the guest, skip if > population failed. Would that work? Let me append some more info here.. I think madvise() isn't needed as fallocate() should do the population work already, afaiu, then it means we pass the shmem path to QEMU and QEMU should notice this memory-backend-file existed, open() directly. I quicked walk the QEMU memory code and so far it looks all applicable, so that QEMU should just start the guest with the pre-populated shmem page caches. There's one trick where qemu_ram_mmap() will map some extra pages, on x86 4k, and I don't yet know why we did that.. /* * Note: this always allocates at least one extra page of virtual address * space, even if size is already aligned. */ total = size + align; But that was only used in mmap_reserve() not mmap_activate(), so the real mmap() should still be exactly what we fallocate()ed. Thanks,
Peter Xu <peterx@redhat.com> writes: > On Tue, May 28, 2024 at 09:35:22AM -0400, Peter Xu wrote: >> On Tue, May 28, 2024 at 02:27:57PM +1000, Nicholas Piggin wrote: >> > There is no need to use /dev/shm for file-backed memory devices, and >> > it is too small to be usable in gitlab CI. Switch to using a regular >> > file in /tmp/ which will usually have more space available. >> > >> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> > --- >> > Am I missing something? AFAIKS there is not even any point using >> > /dev/shm aka tmpfs anyway, there is not much special about it as a >> > filesystem. This applies on top of the series just sent, and passes >> > gitlab CI qtests including aarch64. >> >> I think it's just that /dev/shm guarantees shmem usage, while the var >> "tmpfs" implies g_dir_make_tmp() which may be another non-ram based file >> system, while that'll be slightly different comparing to what a real user >> would use - we don't suggest user to put guest RAM on things like btrfs. >> >> One real implication is if we add a postcopy test it'll fail with >> g_dir_make_tmp() when it is not pointing to a shmem mount, as >> UFFDIO_REGISTER will fail there. But that test doesn't yet exist as the >> QEMU paths should be the same even if Linux will trigger different paths >> when different types of mem is used (anonymous v.s. shmem). >> >> If the goal here is to properly handle the case where tmpfs doesn't have >> enough space, how about what I suggested in the other email? >> >> https://lore.kernel.org/r/ZlSppKDE6wzjCF--@x1n >> >> IOW, try populate the shmem region before starting the guest, skip if >> population failed. Would that work? > > Let me append some more info here.. > > I think madvise() isn't needed as fallocate() should do the population work > already, afaiu, then it means we pass the shmem path to QEMU and QEMU > should notice this memory-backend-file existed, open() directly. > > I quicked walk the QEMU memory code and so far it looks all applicable, so > that QEMU should just start the guest with the pre-populated shmem page > caches. > > There's one trick where qemu_ram_mmap() will map some extra pages, on x86 > 4k, and I don't yet know why we did that.. > > /* > * Note: this always allocates at least one extra page of virtual address > * space, even if size is already aligned. > */ > total = size + align; At the end of the function: /* * Leave a single PROT_NONE page allocated after the RAM block, to serve as * a guard page guarding against potential buffer overflows. */
On Wed May 29, 2024 at 2:05 AM AEST, Peter Xu wrote: > On Tue, May 28, 2024 at 09:35:22AM -0400, Peter Xu wrote: > > On Tue, May 28, 2024 at 02:27:57PM +1000, Nicholas Piggin wrote: > > > There is no need to use /dev/shm for file-backed memory devices, and > > > it is too small to be usable in gitlab CI. Switch to using a regular > > > file in /tmp/ which will usually have more space available. > > > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > > --- > > > Am I missing something? AFAIKS there is not even any point using > > > /dev/shm aka tmpfs anyway, there is not much special about it as a > > > filesystem. This applies on top of the series just sent, and passes > > > gitlab CI qtests including aarch64. > > > > I think it's just that /dev/shm guarantees shmem usage, while the var > > "tmpfs" implies g_dir_make_tmp() which may be another non-ram based file > > system, while that'll be slightly different comparing to what a real user > > would use - we don't suggest user to put guest RAM on things like btrfs. Right, these days I think /tmp usually is not tmpfs but just a regular filesystem. For these tests that's okay though. And it gets us working with gitlab CI. The ignore-shared test works and is verified to skip the copy (according to counters and some tracing I did) so I think it's a good step. > > > > One real implication is if we add a postcopy test it'll fail with > > g_dir_make_tmp() when it is not pointing to a shmem mount, as > > UFFDIO_REGISTER will fail there. But that test doesn't yet exist as the > > QEMU paths should be the same even if Linux will trigger different paths > > when different types of mem is used (anonymous v.s. shmem). Ah okay userfault. I guess that would require real tmpfs. We could just add a new option to the harness for require_uffd when it comes up? > > If the goal here is to properly handle the case where tmpfs doesn't have > > enough space, how about what I suggested in the other email? > > > > https://lore.kernel.org/r/ZlSppKDE6wzjCF--@x1n > > > > IOW, try populate the shmem region before starting the guest, skip if > > population failed. Would that work? I think that's good if you _need_ shm (e.g., for a uffd test), but we should permit tests that only require a memory file. Thanks, Nick
On Wed, May 29, 2024 at 10:05:32AM +1000, Nicholas Piggin wrote: > I think that's good if you _need_ shm (e.g., for a uffd test), but > we should permit tests that only require a memory file. Yes there's no reason to forbid that, it's just that we're not adding new tests but we can potentially change any future use_shmem to not test shmem anymore.. instead we test something we don't suggest users to use.. The only concern is a small /dev/shm mount, am I right? Would it work if switch to memory-backend-memfd,shared=on? Thanks,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 45830eb213..86eace354e 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,29 +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; - } - if (getenv("GITLAB_CI")) { - /* - * Gitlab runners are limited to 64MB shm size. See: - * https://lore.kernel.org/all/87ttq5fvh7.fsf@suse.de/ - */ - g_test_skip("/dev/shm is not supported in Gitlab CI environment"); - return -1; - } - } - dst_state = (QTestMigrationState) { }; src_state = (QTestMigrationState) { }; bootfile_create(tmpfs, args->suspend_me); @@ -754,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) { @@ -788,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) { @@ -810,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); @@ -822,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; @@ -1875,7 +1860,7 @@ static void test_ignore_shared(void) g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); QTestState *from, *to; MigrateStart args = { - .use_shmem = true, + .use_memfile = true, }; if (test_migrate_start(&from, &to, uri, &args)) { @@ -2033,7 +2018,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
There is no need to use /dev/shm for file-backed memory devices, and it is too small to be usable in gitlab CI. Switch to using a regular file in /tmp/ which will usually have more space available. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Am I missing something? AFAIKS there is not even any point using /dev/shm aka tmpfs anyway, there is not much special about it as a filesystem. This applies on top of the series just sent, and passes gitlab CI qtests including aarch64. Thanks, Nick tests/qtest/migration-test.c | 41 ++++++++++++------------------------ 1 file changed, 13 insertions(+), 28 deletions(-)