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.