Re: [PATCH v3 3/7] selftest/mm: Fix ksm_funtional_test failures

[Donet Tom <donettom@linux.ibm.com>]

On 8/8/25 8:28 AM, Wei Yang wrote:
> On Thu, Aug 07, 2025 at 02:56:28PM +0530, Donet Tom wrote:
>> On 8/6/25 8:24 PM, Wei Yang wrote:
>>> On Wed, Aug 06, 2025 at 06:30:37PM +0530, Donet Tom wrote:
>>> [...]
>>>>> Child process inherit the ksm_merging_pages from parent, which is reasonable
>>>>> to me. But I am confused why ksm_unmerge() would just reset ksm_merging_pages
>>>>> for parent and leave ksm_merging_pages in child process unchanged.
>>>>>
>>>>> ksm_unmerge() writes to /sys/kernel/mm/ksm/run, which is a system wide sysfs
>>>>> interface. I expect it applies to both parent and child.
>>>> I am not very familiar with the KSM code, but from what I understand:
>>>>
>>>> The ksm_merging_pages counter is maintained per mm_struct. When
>>>> we write to /sys/kernel/mm/ksm/run, unmerging is triggered, and the
>>>> counters are updated for all mm_structs present in the ksm_mm_slot list.
>>>>
>>>> A mm_struct gets added to this list  when MADV_MERGEABLE is called.
>>>> In the case of the child process, since MADV_MERGEABLE has not been
>>>> invoked yet, its mm_struct is not part of the list. As a result,
>>>> its ksm_merging_pages counter is not reset.
>>>>
>>> Would this flag be inherited during fork? VM_MERGEABLE is saved in related vma
>>> I don't see it would be dropped during fork. Maybe missed.
>>>
>>>>>> value remained unchanged. That’s why get_my_merging_page() in the child was
>>>>>> returning a non-zero value.
>>>>>>
>>>>> I guess you mean the get_my_merging_page() in __mmap_and_merge_range() return
>>>>> a non-zero value. But there is ksm_unmerge() before it. Why this ksm_unmerge()
>>>>> couldn't reset the value, but a ksm_unmerge() in parent could.
>>>>>
>>>>>> Initially, I fixed the issue by calling ksm_unmerge() before the fork(), and
>>>>>> that
>>>>>> resolved the problem. Later, I decided it would be cleaner to move the
>>>>>> ksm_unmerge() call to the test cleanup phase.
>>>>>>
>>>>> Also all the tests before test_prctl_fork(), except test_prctl(), calls
>>>>>
>>>>>      ksft_test_result(!range_maps_duplicates());
>>>>>
>>>>> If the previous tests succeed, it means there is no duplicate pages. This
>>>>> means ksm_merging_pages should be 0 before test_prctl_fork() if other tests
>>>>> pass. And the child process would inherit a 0 ksm_merging_pages. (A quick test
>>>>> proves it.)
>>>> If I understand correctly, all the tests are calling MADV_UNMERGEABLE,
>>>> which internally calls break_ksm() in the kernel. This function replaces the
>>>> KSM page with an exclusive anonymous page. However, the
>>>> ksm_merging_pages counters are not updated at this point.
>>>>
>>>> The function range_maps_duplicates(map, size) checks whether the pages
>>>> have been unmerged. Since break_ksm() does perform the unmerge, this
>>>> function returns false, and the test passes.
>>>>
>>>> The ksm_merging_pages update happens later via the ksm_scan_thread().
>>>> That’s why we observe that ksm_merging_pages values are not reset
>>>> immediately after the test finishes.
>>>>
>>> Not familiar with ksm internal. But the ksm_merging_pages counter still has
>>> non-zero value when all merged pages are unmerged makes me feel odd.
>>>
>>>> If we add a sleep(1) after the MADV_UNMERGEABLE call, we can see that
>>>> the ksm_merging_pages values are reset after the sleep.
>>>>
>>>> Once the test completes successfully, we can call ksm_unmerge(), which
>>>> will immediately reset the ksm_merging_pages value. This way, in the fork
>>>> test, the child process will also see the correct value.
>>>>> So which part of the story I missed?
>>>>>
>>>> So, during the cleanup phase after a successful test, we can call
>>>> ksm_unmerge() to reset the counter. Do you see any issue with
>>>> this approach?
>>>>
>>> It looks there is no issue with an extra ksm_unmerge().
>>>
>>> But one more question. Why an extra ksm_unmerge() could help.
>>>
>>> Here is what we have during test:
>>>
>>>
>>>     test_prot_none()
>>>         !range_maps_duplicates()
>>>         ksm_unmerge()                  1) <--- newly add
>>>     test_prctl_fork()
>>>         >--- in child
>>>         __mmap_and_merge_range()
>>>             ksm_unmerge()              2) <--- already have
>>>
>>> As you mentioned above ksm_unmerge() would immediately reset
>>> ksm_merging_pages, why ksm_unmerge() at 2) still leave ksm_merging_pages
>>> non-zero? And the one at 1) could help.
>>
> >From the debugging, what I understood is:
>> When we perform fork(), MADV_MERGEABLE, or PR_SET_MEMORY_MERGE, the
>> mm_struct of the process gets added to the ksm_mm_slot list. As a
>> result, both the parent and child processes’ mm_struct structures
>> will be present in ksm_mm_slot.
>>
>> When KSM merges the pages, it creates a ksm_rmap_item for each page,
>> and the ksm_merging_pages counter is incremented accordingly.
>>
>> Since the parent process did the merge, its mm_struct is present in
>> ksm_mm_slot, and ksm_rmap_item entries are created for all the merged
>> pages.
>>
>> When a process is forked, the child’s mm_struct is also added to
>> ksm_mm_slot, and it inherits the ksm_merging_pages count. However,
>> no ksm_rmap_item entries are created for the child process because it
>> did not do any merge.
>>
>> When ksm_unmerge() is called, it iterates over all processes in
>> ksm_mm_slot. In our case, both the parent and child are present. It
>> first processes the parent, which has ksm_rmap_item entries, so it
>> unmerges the pages and resets the ksm_merging_pages counter.
>>
>> For the child, since it did not perform any actual merging, it does not
>> have any ksm_rmap_item entries. Therefore, there are no pages to unmerge,
>> and the counter remains unchanged.
>>
> Thanks for the detailed analysis.
>
> So the key is child has no ksm_rmap_item which will not clear ksm_merging_page
> on ksm_unmerge().
>
>> So, only processes that performed KSM merging will have their counters
>> updated during ksm_unmerge(). The child process, having not initiated any
>> merging, retains the inherited counter value without any update.
>>
>> So from a testing point of view, I think it is better to reset the
>> counters as part of the cleanup code to ensure that the next tests do
>> not get incorrect values.
>>
> Hmm... I agree from the test point of view based on current situation.
>
> While maybe this is also a check point for later version.

Are you okay to proceed with the current patch in this series?

>
>> The question I have is: is it correct to keep the inherited
>> |ksm_merging_page|
>> value in the child or Should we reset it to 0 during |ksm_fork()|?
>>
> Very good question. There looks to be something wrong, but I am not sure this
> is the correct way.

ok.

I am going through it and will come up with a fix along with a test for 
this scenario. I will post it as a separate series.


>
>>> Or there is still some timing issue like sleep(1) you did?
>>>