Re: [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec

[Jinjiang Tu <tujinjiang@huawei.com>]

在 2024/3/22 19:43, David Hildenbrand 写道:
> On 22.03.24 07:09, Jinjiang Tu wrote:
>> This extends test_prctl_fork() and test_prctl_fork_exec() to make sure
>> that deduplication really happens, instead of only test the
>> MMF_VM_MERGE_ANY flag is set.
>>
>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>> ---
>>   .../selftests/mm/ksm_functional_tests.c       | 79 +++++++++++++++++--
>>   1 file changed, 73 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c 
>> b/tools/testing/selftests/mm/ksm_functional_tests.c
>> index d615767e396b..01999aab2e37 100644
>> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
>> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
>> @@ -146,6 +146,54 @@ static int ksm_unmerge(void)
>>       return 0;
>>   }
>>   +static int child_test_merge(void)
>> +{
>> +    const unsigned int size = 2 * MiB;
>> +    char *map;
>> +    int ret = -1;
>> +
>> +    /* Stabilize accounting by disabling KSM completely. */
>> +    if (ksm_unmerge()) {
>> +        ksft_print_msg("Disabling (unmerging) KSM failed\n");
>> +        return ret;
>> +    }
>> +
>> +    if (get_my_merging_pages() > 0) {
>> +        ksft_print_msg("Still pages merged\n");
>> +        return ret;
>> +    }
>> +
>> +    map = mmap(NULL, size, PROT_READ|PROT_WRITE,
>> +           MAP_PRIVATE|MAP_ANON, -1, 0);
>> +    if (map == MAP_FAILED) {
>> +        ksft_print_msg("mmap() failed\n");
>> +        return ret;
>> +    }
>> +
>> +    /* Don't use THP. Ignore if THP are not around on a kernel. */
>> +    if (madvise(map, size, MADV_NOHUGEPAGE) && errno != EINVAL) {
>> +        ksft_print_msg("MADV_NOHUGEPAGE failed\n");
>> +        goto unmap;
>> +    }
>> +
>> +    memset(map, 0x1c, size);
>> +
>> +    if (ksm_merge()) {
>> +        ksft_print_msg("Running KSM failed\n");
>> +        goto unmap;
>> +    }
>> +
>> +    if (get_my_merging_pages() <= 0) {
>> +        ksft_print_msg("Fail to merge\n");
>> +        goto unmap;
>> +    }
>
> Looks like all you want is use mmap_and_merge_range(), but neither 
> setting the prctl nor madvise().
>
> Two alternatives:
>
> 1) switching from "bool use_prctl" to an enum like
>
> enum ksm_merge_mode {
>     KSM_MERGE_PRCTL
>     KSM_MERGE_MADVISE,
>     KSM_MERGE_NONE, /* PRCTL already set */
> };
>
> Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB, 
> PROT_READ|PROT_WRITE, KSM_MERGE_NONE);
I have considered this before. But, mmap_and_merge_range() calls 
ksft_test_result_fail() when error occurs, ksft_test_result_fail()
prints prefixed with ksft_fail count. When mmap_and_merge_range() is 
called in the child process, the ksft_fail isn't consisent with the
parent process due to the global variable ksft_fail is CoWed. As a 
result, ksft_print_msg() is intended to be called in child process.

Maybe, We could introduce a macro ksm_print() to control which function 
is called according to ksm_merge_mode :

#define ksm_print(mode, fmt, ...) do {    \
     if ((mode) == KSM_MERGE_NONE)                \
         ksft_print_msg(fmt, ##__VA_ARGS__);\
     else                        \
         ksft_test_result_fail(fmt, ##__VA_ARGS__);\
     } while (0)


>
> 2) With "bool use_prctl", before doing the prctl(PR_SET_MEMORY_MERGE, 
> ...), check if it is already enabled.
>
> As we do that already in ksm_fork_exec_child(), and fail if it isn't 
> set, that should work.
>
> Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB, 
> PROT_READ|PROT_WRITE, true);
>
> here.
>
>> +
>> +    ret = 0;
>> +unmap:
>> +    munmap(map, size);
>> +    return ret;
>> +}
>> +
>>   static char *mmap_and_merge_range(char val, unsigned long size, int 
>> prot,
>>                     bool use_prctl)
>>   {
>> @@ -458,7 +506,11 @@ static void test_prctl_fork(void)
>>         child_pid = fork();
>>       if (!child_pid) {
>> -        exit(prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0));
>> +        if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
>> +            exit(-1);
>> +        if (child_test_merge() != 0)
>> +            exit(-2);
>> +        exit(0);
>>       } else if (child_pid < 0) {
>>           ksft_test_result_fail("fork() failed\n");
>>           return;
>> @@ -467,8 +519,14 @@ static void test_prctl_fork(void)
>>       if (waitpid(child_pid, &status, 0) < 0) {
>>           ksft_test_result_fail("waitpid() failed\n");
>>           return;
>> -    } else if (WEXITSTATUS(status) != 1) {
>> -        ksft_test_result_fail("unexpected PR_GET_MEMORY_MERGE result 
>> in child\n");
>> +    }
>> +
>> +    status = WEXITSTATUS(status);
>> +    if (status != 0) {
>> +        if (status == -1)
>> +            ksft_test_result_fail("unexpected PR_GET_MEMORY_MERGE 
>> result in child\n");
>> +        else
>> +            ksft_test_result_fail("fail to merge in child\n");
>>           return;
>>       }
>>   @@ -483,7 +541,13 @@ static void test_prctl_fork(void)
>>   static int ksm_fork_exec_child(void)
>>   {
>>       /* Test if KSM is enabled for the process. */
>> -    return prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) == 1;
>> +    if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
>> +        return -1;
>> +
>> +    if (child_test_merge() != 0)
>
> You can drop the "!=0". But maybe, you can just inline the call to 
> mmap_and_merge_range() here.
>
>> +        return -2;
>> +
>> +    return 0;
>
>