Re: [PATCH] mm/vma: Do not register private-anon mappings with khugepaged during mmap

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

+cc Greg

On Thu, Mar 06, 2025 at 08:02:10PM +0530, Dev Jain wrote:
>
>
> On 06/03/25 1:48 pm, Lorenzo Stoakes wrote:
> > On Thu, Mar 06, 2025 at 12:00:37PM +0530, Dev Jain wrote:
> > > We already are registering private-anon VMAs with khugepaged during fault
> > > time, in do_huge_pmd_anonymous_page(). Commit "register suitable readonly
> > > file vmas for khugepaged" moved the khugepaged registration logic from
> > > shmem_mmap to the generic mmap path. Make this logic specific for non-anon
> > > mappings.
> >
> > This does sound reasonable, thanks! Though does need to be expanded as
> > Andrew says for user-visible effects just to be crystal clear.
>
> Sure.

Thanks!

>
> >
> > >
> > > Fixes: 613bec092fe7 ("mm: mmap: register suitable readonly file vmas for khugepaged")
> > > Signed-off-by: Dev Jain <dev.jain@arm.com>
> > > ---
> > >   mm/vma.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/vma.c b/mm/vma.c
> > > index af1d549b179c..730a26bf14a5 100644
> > > --- a/mm/vma.c
> > > +++ b/mm/vma.c
> > > @@ -2377,7 +2377,8 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap)
> > >   	 * vma_merge_new_range() calls khugepaged_enter_vma() too, the below
> > >   	 * call covers the non-merge case.
> > >   	 */
> > > -	khugepaged_enter_vma(vma, map->flags);
> > > +	if (!vma_is_anonymous(vma))
> > > +		khugepaged_enter_vma(vma, map->flags);
> >
> > This really needs a comment :) as a Joe Bloggs coder coming to this my
> > immediate thought would be 'huh? Why?'
> >
> > Something like:
> >
> > 	/* Just added so khugepaged has nothing to do. We call again on fault. */
> >
> > Would be great.
> >
> > Thanks!
>
> Sure.

Thanks!

>
> BTW does this patch merit a CC:stable? I am not aware of the general
> practice but I am not sure if this is a *serious bug*, as per
> submitting-patches.rst.

I think it's fine to send 'not serious' bugs too :P I mean this is a perf
regression right? We don't want that in our stable kernels.

Unless Greg suggests otherwise (cc'd) I'd say it does merit it as there seems to
be no risk, only benefit and reduces overhead, so from my point of view seems
sensible.

>
> >
> >
> > >   	ksm_add_vma(vma);
> > >   	*vmap = vma;
> > >   	return 0;
> > > --
> > > 2.30.2
> > >
>