Re: [PATCH] tests/qtest/migrate-test: Add a postcopy memfile test
On Thu May 30, 2024 at 3:35 AM AEST, Fabiano Rosas wrote: > Peter Xu writes: > > > On Wed, May 29, 2024 at 09:54:30AM -0300, Fabiano Rosas wrote: > >> Nicholas Piggin writes: > >> > >> > 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 > >> > --- > >> > > >> > How about this? This goes on top of the reset of the patches > >> > (I'll re-send them all as a series if we can get to some agreement). > >> > > >> > This adds back the /dev/shm option with preallocation and adds a test > >> > case that requires tmpfs. > >> > >> Peter has stronger opinions on this than I do. I'll leave it to him to > >> decide. > > > > Sorry if I gave that feeling; it's more of a stronger willingness to at > > some point enable shmem for QEMU migration, rather than wanting to push > > back what Nicholas was trying to do. > > Of course, I didn't mean to imply that. I just saying that using /tmp > would have been fine with me and I don't want to get in the way. > > > Enabling more arch for migration > > tests is definitely worthwhile on its own. > > > > Shmem is just some blank spot that IMHO we should start to think about > > better coverarge. E.g. it is the only sane way to boot the VM that is able > > to do fast qemu upgrades using ignore-shared, that was true even before > > Steve's cpr-exec work, which would be much easier than anonymous. And it's > > also possible shmem can be (in the next 3-5 years) the 1G page provider to > > replace hugetlb for postcopy's sake - this one is far beyond our current > > discussion so I won't extend.. > > Interesting, good to know. > > > > > IMHO shmem should just be a major backend just like anonymous, and the only > > possible file backend we can test in CI - as hugetlb is harder to manage > > there. > > > >> > >> Just note that now we're making the CI less deterministic in relation to > >> the migration tests. When a test that uses shmem fails, we'll not be > >> able to consistently reproduce because the test might not even run > >> depending on what has consumed the shmem first. > >> > >> Let's also take care that the other consumers of shmem (I think just > >> ivshmem-test) are able to cope with the migration-test taking all the > >> space, otherwise the CI will still break. > > > > Looks like ivshmem-test only uses 1MB shmem constantly so probably that > > will succeed if the migration test will, but true they face the same > > challenge and they interfere with each other.. that test sidently pass > > (instead of skip) if mktempshm() fails. I guess we don't have a way to > > solidly test shmem as shmem simply may not be around. > > Here we have each of the 5 migration archs taking up some amount of > memory + each of the 3 supported arches for ivshmem. They all could be > running in parallel through make check. In practice there's maybe less > overlap due to timing and not all CI jobs building all archs, but still. I could just add back the GITLAB_CI gate for shm tests for now then. > > > > > For this patch alone personally I'd avoid using "use_uffd_memfile" as the > > name, as that's definitely confusing, since shmem can be tested in other > > setups too without uffd. Nicolas, please feel free to move ahead with your I was just thinking uffd could be used with another memfile (hugetlbfs) but on second thoughts that's a bit silly. So use_shm_memfile would be better. > > arch enablement series with /tmp if you want to separate the shmem issue. > > Or just leave ignore_shared untouched for the ppc series. Good idea, I think everybody is happy enough with ppc series so I will send that first. Thanks, Nick
Re: [PATCH] tests/qtest/migrate-test: Add a postcopy memfile test
Peter Xu writes: > On Wed, May 29, 2024 at 09:54:30AM -0300, Fabiano Rosas wrote: >> Nicholas Piggin writes: >> >> > 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 >> > --- >> > >> > How about this? This goes on top of the reset of the patches >> > (I'll re-send them all as a series if we can get to some agreement). >> > >> > This adds back the /dev/shm option with preallocation and adds a test >> > case that requires tmpfs. >> >> Peter has stronger opinions on this than I do. I'll leave it to him to >> decide. > > Sorry if I gave that feeling; it's more of a stronger willingness to at > some point enable shmem for QEMU migration, rather than wanting to push > back what Nicholas was trying to do. Of course, I didn't mean to imply that. I just saying that using /tmp would have been fine with me and I don't want to get in the way. > Enabling more arch for migration > tests is definitely worthwhile on its own. > > Shmem is just some blank spot that IMHO we should start to think about > better coverarge. E.g. it is the only sane way to boot the VM that is able > to do fast qemu upgrades using ignore-shared, that was true even before > Steve's cpr-exec work, which would be much easier than anonymous. And it's > also possible shmem can be (in the next 3-5 years) the 1G page provider to > replace hugetlb for postcopy's sake - this one is far beyond our current > discussion so I won't extend.. Interesting, good to know. > > IMHO shmem should just be a major backend just like anonymous, and the only > possible file backend we can test in CI - as hugetlb is harder to manage > there. > >> >> Just note that now we're making the CI less deterministic in relation to >> the migration tests. When a test that uses shmem fails, we'll not be >> able to consistently reproduce because the test might not even run >> depending on what has consumed the shmem first. >> >> Let's also take care that the other consumers of shmem (I think just >> ivshmem-test) are able to cope with the migration-test taking all the >> space, otherwise the CI will still break. > > Looks like ivshmem-test only uses 1MB shmem constantly so probably that > will succeed if the migration test will, but true they face the same > challenge and they interfere with each other.. that test sidently pass > (instead of skip) if mktempshm() fails. I guess we don't have a way to > solidly test shmem as shmem simply may not be around. Here we have each of the 5 migration archs taking up some amount of memory + each of the 3 supported arches for ivshmem. They all could be running in parallel through make check. In practice there's maybe less overlap due to timing and not all CI jobs building all archs, but still. > > For this patch alone personally I'd avoid using "use_uffd_memfile" as the > name, as that's definitely confusing, since shmem can be tested in other > setups too without uffd. Nicolas, please feel free to move ahead with your > arch enablement series with /tmp if you want to separate the shmem issue. Or just leave ignore_shared untouched for the ppc series. > > Thanks,
Re: [PATCH] tests/qtest/migrate-test: Add a postcopy memfile test
On Wed, May 29, 2024 at 09:54:30AM -0300, Fabiano Rosas wrote: > Nicholas Piggin writes: > > > 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 > > --- > > > > How about this? This goes on top of the reset of the patches > > (I'll re-send them all as a series if we can get to some agreement). > > > > This adds back the /dev/shm option with preallocation and adds a test > > case that requires tmpfs. > > Peter has stronger opinions on this than I do. I'll leave it to him to > decide. Sorry if I gave that feeling; it's more of a stronger willingness to at some point enable shmem for QEMU migration, rather than wanting to push back what Nicholas was trying to do. Enabling more arch for migration tests is definitely worthwhile on its own. Shmem is just some blank spot that IMHO we should start to think about better coverarge. E.g. it is the only sane way to boot the VM that is able to do fast qemu upgrades using ignore-shared, that was true even before Steve's cpr-exec work, which would be much easier than anonymous. And it's also possible shmem can be (in the next 3-5 years) the 1G page provider to replace hugetlb for postcopy's sake - this one is far beyond our current discussion so I won't extend.. IMHO shmem should just be a major backend just like anonymous, and the only possible file backend we can test in CI - as hugetlb is harder to manage there. > > Just note that now we're making the CI less deterministic in relation to > the migration tests. When a test that uses shmem fails, we'll not be > able to consistently reproduce because the test might not even run > depending on what has consumed the shmem first. > > Let's also take care that the other consumers of shmem (I think just > ivshmem-test) are able to cope with the migration-test taking all the > space, otherwise the CI will still break. Looks like ivshmem-test only uses 1MB shmem constantly so probably that will succeed if the migration test will, but true they face the same challenge and they interfere with each other.. that test sidently pass (instead of skip) if mktempshm() fails. I guess we don't have a way to solidly test shmem as shmem simply may not be around. For this patch alone personally I'd avoid using "use_uffd_memfile" as the name, as that's definitely confusing, since shmem can be tested in other setups too without uffd. Nicolas, please feel free to move ahead with your arch enablement series with /tmp if you want to separate the shmem issue. Thanks, -- Peter Xu
Re: [PATCH] tests/qtest/migrate-test: Add a postcopy memfile test
Nicholas Piggin writes:
> 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
> ---
>
> How about this? This goes on top of the reset of the patches
> (I'll re-send them all as a series if we can get to some agreement).
>
> This adds back the /dev/shm option with preallocation and adds a test
> case that requires tmpfs.
Peter has stronger opinions on this than I do. I'll leave it to him to
decide.
Just note that now we're making the CI less deterministic in relation to
the migration tests. When a test that uses shmem fails, we'll not be
able to consistently reproduce because the test might not even run
depending on what has consumed the shmem first.
Let's also take care that the other consumers of shmem (I think just
ivshmem-test) are able to cope with the migration-test taking all the
space, otherwise the CI will still break.
>
> Thanks,
> Nick
>
> tests/qtest/migration-test.c | 63 +++-
> 1 file changed, 55 insertions(+), 8 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 86eace354e..7fd9bbdc18 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_uffd_memfile;
> /* only launch the target process */
> bool only_target;
> /* Use dirty ring if true; dirty logging otherwise */
> @@ -739,7 +741,48 @@ 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_uffd_memfile) {
> +#if defined(__NR_userfaultfd) && defined(__linux__)
> +int fd;
> +uint64_t size;
> +
> +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, ) == 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 +794,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 +844,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_uffd_memfile) {
> unlink(memfile_path);
> }
>
> @@ -1275,6 +1312,15 @@ static void test_postcopy(void)
> test_postcopy_common();
> }
>
> +static void test_postcopy_memfile(void)
> +{
> +MigrateCommon args = {
> +.start.use_uffd_memfile = true,
> +};
> +
> +test_postcopy_common();
> +}
> +
> static void test_postcopy_suspend(void)
> {
> MigrateCommon args = {
> @@ -3441,6 +3487,7 @@ int
[PATCH] tests/qtest/migrate-test: Add a postcopy memfile test
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
---
How about this? This goes on top of the reset of the patches
(I'll re-send them all as a series if we can get to some agreement).
This adds back the /dev/shm option with preallocation and adds a test
case that requires tmpfs.
Thanks,
Nick
tests/qtest/migration-test.c | 63 +++-
1 file changed, 55 insertions(+), 8 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 86eace354e..7fd9bbdc18 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_uffd_memfile;
/* only launch the target process */
bool only_target;
/* Use dirty ring if true; dirty logging otherwise */
@@ -739,7 +741,48 @@ 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_uffd_memfile) {
+#if defined(__NR_userfaultfd) && defined(__linux__)
+int fd;
+uint64_t size;
+
+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, ) == 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 +794,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 +844,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_uffd_memfile) {
unlink(memfile_path);
}
@@ -1275,6 +1312,15 @@ static void test_postcopy(void)
test_postcopy_common();
}
+static void test_postcopy_memfile(void)
+{
+MigrateCommon args = {
+.start.use_uffd_memfile = true,
+};
+
+test_postcopy_common();
+}
+
static void test_postcopy_suspend(void)
{
MigrateCommon args = {
@@ -3441,6 +3487,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
