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

[Fabiano Rosas <farosas@suse.de>]

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.