Re: [PATCH v8 3/3] selftests/mm: add new selftests for KSM

Re: [PATCH v8 3/3] selftests/mm: add new selftests for KSM

[David Hildenbrand]

On 16.04.23 00:59, Stefan Roesch wrote:
> This adds three new tests to the selftests for KSM.  These tests use the
> new prctl API's to enable and disable KSM.
> 
> 1) add new prctl flags to prctl header file in tools dir
> 
>     This adds the new prctl flags to the include file prct.h in the
>     tools directory.  This makes sure they are available for testing.
> 
> 2) add KSM prctl merge test to ksm_tests
> 
>     This adds the -t option to the ksm_tests program.  The -t flag
>     allows to specify if it should use madvise or prctl ksm merging.
> 
> 3) add two functions for debugging merge outcome for ksm_tests
> 
>     This adds two functions to report the metrics in /proc/self/ksm_stat
>     and /sys/kernel/debug/mm/ksm. The debug output is enabled with the
>     -d option.
> 
> 4) add KSM prctl test to ksm_functional_tests
> 
>     This adds a test to the ksm_functional_test that verifies that the
>     prctl system call to enable / disable KSM works.
> 
> 5) add KSM fork test to ksm_functional_test
> 
>     Add fork test to verify that the MMF_VM_MERGE_ANY flag is inherited
>     by the child process.
> 
> Signed-off-by: Stefan Roesch <shr@devkernel.io>
> Cc: Bagas Sanjaya <bagasdotme@gmail.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Rik van Riel <riel@surriel.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---

Thanks!

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v8 3/3] selftests/mm: add new selftests for KSM

[David Hildenbrand]

> +/* Verify that KSM can be enabled / queried with prctl. */
> +static void test_prctl(void)
> +{
> +	int ret;
> +
> +	ksft_print_msg("[RUN] %s\n", __func__);
> +
> +	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
> +	if (ret < 0 && errno == EINVAL) {
> +		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
> +		return;
> +	} else if (ret) {
> +		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
> +	}

Just realized we're missing a "return;" in case of the failure here.

-- 
Thanks,

David / dhildenb

Re: [PATCH v8 3/3] selftests/mm: add new selftests for KSM

[David Hildenbrand]

On 17.04.23 16:35, David Hildenbrand wrote:
>> +/* Verify that KSM can be enabled / queried with prctl. */
>> +static void test_prctl(void)
>> +{
>> +	int ret;
>> +
>> +	ksft_print_msg("[RUN] %s\n", __func__);
>> +
>> +	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
>> +	if (ret < 0 && errno == EINVAL) {
>> +		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
>> +		return;
>> +	} else if (ret) {
>> +		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
>> +	}
> 
> Just realized we're missing a "return;" in case of the failure here.
> 


And we should probably fix that as well:

ERROR: do not initialise globals to 0
#235: FILE: tools/testing/selftests/mm/ksm_tests.c:57:
+int debug = 0;

total: 1 errors, 0 warnings, 512 lines checked


-- 
Thanks,

David / dhildenb

Re: [PATCH v8 3/3] selftests/mm: add new selftests for KSM

[Stefan Roesch]

David Hildenbrand <david@redhat.com> writes:

> On 17.04.23 16:35, David Hildenbrand wrote:
>>> +/* Verify that KSM can be enabled / queried with prctl. */
>>> +static void test_prctl(void)
>>> +{
>>> +	int ret;
>>> +
>>> +	ksft_print_msg("[RUN] %s\n", __func__);
>>> +
>>> +	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
>>> +	if (ret < 0 && errno == EINVAL) {
>>> +		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
>>> +		return;
>>> +	} else if (ret) {
>>> +		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
>>> +	}
>> Just realized we're missing a "return;" in case of the failure here.
>>
>
>
> And we should probably fix that as well:
>
> ERROR: do not initialise globals to 0
> #235: FILE: tools/testing/selftests/mm/ksm_tests.c:57:
> +int debug = 0;
>
> total: 1 errors, 0 warnings, 512 lines checked

I'll fix it in the next version.'

Re: [PATCH v8 3/3] selftests/mm: add new selftests for KSM

[Stefan Roesch]

David Hildenbrand <david@redhat.com> writes:

>> +/* Verify that KSM can be enabled / queried with prctl. */
>> +static void test_prctl(void)
>> +{
>> +	int ret;
>> +
>> +	ksft_print_msg("[RUN] %s\n", __func__);
>> +
>> +	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
>> +	if (ret < 0 && errno == EINVAL) {
>> +		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
>> +		return;
>> +	} else if (ret) {
>> +		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
>> +	}
>
> Just realized we're missing a "return;" in case of the failure here.
>
I'll fix it in the next version.

Re: [PATCH v8 1/3] mm: add new api to enable ksm per process

[David Hildenbrand]

> +static void __ksm_add_vma(struct vm_area_struct *vma)
> +{
> +	unsigned long vm_flags = vma->vm_flags;
> +
> +	if (vm_flags & VM_MERGEABLE)
> +		return;
> +
> +	if (vma_ksm_compatible(vma)) {
> +		vm_flags |= VM_MERGEABLE;
> +		vm_flags_reset(vma, vm_flags);
> +	}
> +}

We can do the following simplification on top:

diff --git a/mm/ksm.c b/mm/ksm.c
index 905c8edce5cf..26e7f585d65d 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2511,15 +2511,11 @@ static int ksm_scan_thread(void *nothing)

  static void __ksm_add_vma(struct vm_area_struct *vma)
  {
-	unsigned long vm_flags = vma->vm_flags;
-
-	if (vm_flags & VM_MERGEABLE)
+	if (vma->vm_flags & VM_MERGEABLE)
  		return;

-	if (vma_ksm_compatible(vma)) {
-		vm_flags |= VM_MERGEABLE;
-		vm_flags_reset(vma, vm_flags);
-	}
+	if (vma_ksm_compatible(vma))
+		vm_flags_set(vma, VM_MERGEABLE);
  }

  /**
-- 
2.39.2


I have some patches based on your patch set (handling/testing unmerging 
whens setting PR_SET_MEMORY_MERGE=0. Will send out once you changes are 
queued.

-- 
Thanks,

David / dhildenb

Re: [PATCH v8 1/3] mm: add new api to enable ksm per process

[Stefan Roesch]

David Hildenbrand <david@redhat.com> writes:

>> +static void __ksm_add_vma(struct vm_area_struct *vma)
>> +{
>> +	unsigned long vm_flags = vma->vm_flags;
>> +
>> +	if (vm_flags & VM_MERGEABLE)
>> +		return;
>> +
>> +	if (vma_ksm_compatible(vma)) {
>> +		vm_flags |= VM_MERGEABLE;
>> +		vm_flags_reset(vma, vm_flags);
>> +	}
>> +}
>
> We can do the following simplification on top:
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 905c8edce5cf..26e7f585d65d 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2511,15 +2511,11 @@ static int ksm_scan_thread(void *nothing)
>
>  static void __ksm_add_vma(struct vm_area_struct *vma)
>  {
> -	unsigned long vm_flags = vma->vm_flags;
> -
> -	if (vm_flags & VM_MERGEABLE)
> +	if (vma->vm_flags & VM_MERGEABLE)
>  		return;
>
> -	if (vma_ksm_compatible(vma)) {
> -		vm_flags |= VM_MERGEABLE;
> -		vm_flags_reset(vma, vm_flags);
> -	}
> +	if (vma_ksm_compatible(vma))
> +		vm_flags_set(vma, VM_MERGEABLE);
>  }
>
>  /**
> --
> 2.39.2
>
>

The next version has the above change.

> I have some patches based on your patch set (handling/testing unmerging whens
> setting PR_SET_MEMORY_MERGE=0. Will send out once you changes are queued.