Johannes Weiner <hannes@xxxxxxxxxxx> writes:
> On Wed, Mar 08, 2023 at 02:16:36PM -0800, Stefan Roesch wrote:
>> Johannes Weiner <hannes@xxxxxxxxxxx> 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.
