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

[Stefan Roesch <shr@devkernel.io>]

Johannes Weiner <hannes@cmpxchg.org> writes:

> On Wed, Mar 08, 2023 at 02:16:36PM -0800, Stefan Roesch wrote:
>> Johannes Weiner <hannes@cmpxchg.org> writes:
>> > On Thu, Feb 23, 2023 at 08:39:58PM -0800, Stefan Roesch wrote:
>> >> @@ -2405,8 +2417,20 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>> >>  		goto no_vmas;
>> >>
>> >>  	for_each_vma(vmi, vma) {
>> >> -		if (!(vma->vm_flags & VM_MERGEABLE))
>> >> +		if (!vma_ksm_mergeable(vma))
>> >>  			continue;
>> >> +		if (!(vma->vm_flags & VM_MERGEABLE)) {
>> >
>> > IMO, the helper obscures the interaction between the vma flag and the
>> > per-process flag here. How about:
>> >
>> > 		if (!(vma->vm_flags & VM_MERGEABLE)) {
>> > 			if (!test_bit(MMF_VM_MERGE_ANY, &vma->vm_mm->flags))
>> > 				continue;
>> >
>> > 			/*
>> > 			 * With per-process merging enabled, have the MM scan
>> > 			 * enroll any existing and new VMAs on the fly.
>> > 			 *
>> > 			ksm_madvise();
>> > 		}
>> >
>> >> +			unsigned long flags = vma->vm_flags;
>> >> +
>> >> +			/* madvise failed, use next vma */
>> >> +			if (ksm_madvise(vma, vma->vm_start, vma->vm_end, MADV_MERGEABLE, &flags))
>> >> +				continue;
>> >> +			/* vma, not supported as being mergeable */
>> >> +			if (!(flags & VM_MERGEABLE))
>> >> +				continue;
>> >> +
>> >> +			vm_flags_set(vma, VM_MERGEABLE);
>> >
>> > I don't understand the local flags. Can't it pass &vma->vm_flags to
>> > ksm_madvise()? It'll set VM_MERGEABLE on success. And you know it
>> > wasn't set before because the whole thing is inside the !set
>> > branch. The return value doesn't seem super useful, it's only the flag
>> > setting that matters:
>> >
>> > 			ksm_madvise(vma, vma->vm_start, vma->vm_end, MADV_MERGEABLE, &vma->vm_flags);
>> > 			/* madvise can fail, and will skip special vmas (pfnmaps and such) */
>> > 			if (!(vma->vm_flags & VM_MERGEABLE))
>> > 				continue;
>> >
>>
>> vm_flags is defined as const. I cannot pass it directly inside the
>> function, this is the reason, I'm using a local variable for it.
>
> Oops, good catch.
>
> However, while looking at the flag helpers, I'm also realizing that
> modifications requires the mmap_sem in write mode, which this code
> doesn't. This function might potentially scan the entire process
> address space, so you can't just change the lock mode, either.
>
> Staring more at this, do you actually need to set VM_MERGEABLE on the
> individual vmas? There are only a few places that check VM_MERGEABLE,
> and AFAICS they can all just check for MMF_VM_MERGE_ANY also.
>
> You'd need to factor out the vma compatibility checks from
> ksm_madvise(), and skip over special vmas during the mm scan. But
> those tests are all stable under the read lock, so that's fine.
>
> The other thing ksm_madvise() does is ksm_enter() - but that's
> obviously not needed from inside the loop over ksm_enter'd mms. :)

The check alone for MMF_VM_MERGE_ANY is not sufficient. We also
need to check if the respective VMA is mergeable. I'll split off the
checks in ksm_madvise to its own function, so it can be called from
where VM_MERGEABLE is currently checked.

With the above change, the function unmerge_vmas is no longer needed.