Re: [PATCH 3/4] mm: prevent KSM from completely breaking VMA merging

[Lorenzo Stoakes]

On Mon, May 19, 2025 at 08:04:22PM +0200, David Hildenbrand wrote:
>
> > > +/*
> > > + * Are we guaranteed no driver can change state such as to preclude KSM merging?
> > > + * If so, let's set the KSM mergeable flag early so we don't break VMA merging.
> > > + *
> > > + * This is applicable when PR_SET_MEMORY_MERGE has been set on the mm_struct via
> > > + * prctl() causing newly mapped VMAs to have the KSM mergeable VMA flag set.
> > > + *
> > > + * If this is not the case, then we set the flag after considering mergeability,
> > > + * which will prevent mergeability as, when PR_SET_MEMORY_MERGE is set, a new
> > > + * VMA will not have the KSM mergeability VMA flag set, but all other VMAs will,
> > > + * preventing any merge.
> >
> > Hmmm, so an ordinary MAP_PRIVATE of any file (executable etc.) will get
> > VM_MERGEABLE set but not be able to merge?
> >
> > Probably these are not often expected to be merged ...
> >
> > Preventing merging should really only happen because of VMA flags that
> > are getting set: VM_PFNMAP, VM_MIXEDMAP, VM_DONTEXPAND, VM_IO.
> >
> >
> > I am not 100% sure why we bail out on special mappings: all we have to
> > do is reliably identify anon pages, and we should be able to do that.
> >
> > GUP does currently refuses any VM_PFNMAP | VM_IO, and KSM uses GUP,
> > which might need a tweak then (maybe the solution could be to ... not
> > use GUP but a folio_walk).
>
> Oh, someone called "David" already did that. Nice :)
>
> So we *should* be able to drop
>
> * VM_PFNMAP: we correctly identify CoWed pages
> * VM_MIXEDMAP: we correctly identify CoWed pages
> * VM_IO: should not affect CoWed pages
> * VM_DONTEXPAND: no idea why that should even matter here

I objected in the other thread but now realise I forgot we're talking about
MAP_PRIVATE... So we can do the CoW etc. Right.

Then we just need to be able to copy the thing on CoW... but what about
write-through etc. cache settings? I suppose we don't care once CoW'd...

But is this common enough of a use case to be worth the hassle of checking this
is all ok?

I don't know KSM well enough to comment on VM_DONTEXPAND but obviously
meaningful for purposes of _VMA merging_. We refuse to merge all of these.

Anyway this all feels like something for the future :)

It'd make life easier here yes, but we'd have to be _really sure_ there isn't
_some .mmap() hook somewhere_ that's doing something crazy.

Which is another reason why I really really hate .mmap() as-is and why I'm
changing it.

So it may still be more conservative to leave things as they are even if this
change was made... Guess it depends how much we care about 'crazy' drivers.

>
> --
> Cheers,
>
> David / dhildenb
>