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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, May 28, 2025 at 04:50:18PM +0100, Lorenzo Stoakes wrote:
> On Wed, May 28, 2025 at 11:38:32PM +0800, xu.xin16@zte.com.cn wrote:
> > > +static void update_ksm_flags(struct mmap_state *map)
> > > +{
> > > +	map->flags = ksm_vma_flags(map->mm, map->file, map->flags);
> > > +}
> > > +
> > > +/*
> > > + * 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.
> > > + */
> > > +static bool can_set_ksm_flags_early(struct mmap_state *map)
> > > +{
> > > +	struct file *file = map->file;
> > > +
> > > +	/* Anonymous mappings have no driver which can change them. */
> > > +	if (!file)
> > > +		return true;
> > > +
> > > +	/* shmem is safe. */
> >
> > Excuse me, why it's safe here? Does KSM support shmem?
>
> Because shmem_mmap() doesn't do anything which would invalidate the KSM.
>
> Yeah I think I misinterpreted actually - looks like shmem isn't supported
> (otherwise VM_SHARED would be set rendering the VMA incompatible), _but_
> as with all file-backed mappings, MAP_PRIVATE mappings _are_.
>
> So this is still relevant :)

Will update the commit message to correct the erroneous reference to shmem.

>
> >
> > > +	if (shmem_file(file))
> > > +		return true;
> > > +
> > > +	/*
> > > +	 * If .mmap_prepare() is specified, then the driver will have already
> > > +	 * manipulated state prior to updating KSM flags.
> > > +	 */
> >
> > Recommend expanding the comments here with slightly more verbose explanations to improve
> > code comprehension. Consider adding the following note (even though your commit log is
> > already sufficiently clear.   :)
> > /*
> > * If .mmap_prepare() is specified, then the driver will have already
> > * manipulated state prior to updating KSM flags. So no need to worry
> > * about mmap callbacks modifying vm_flags after the KSM flag has been
> > * updated here, which could otherwise affect KSM eligibility.
> > */
>
> While this comment is really nice actually, I think we're probably ok with the
> shorter version given the commit log goes into substantial detail.

Actually on second thoughts, as I'm respinning, I'll update this and replace
with (a slightly adjusted version of) your excellent comment! :)

>
> >
> >
> > > +	if (file->f_op->mmap_prepare)
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > > +