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

[Johannes Weiner <hannes@cmpxchg.org>]

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. :)