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

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

[Dev Jain]

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.

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);
 	ksm_add_vma(vma);
 	*vmap = vma;
 	return 0;
-- 
2.30.2

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

[Andrew Morton]

On Thu,  6 Mar 2025 12:00:37 +0530 Dev Jain <dev.jain@arm.com> 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.

Please fully describe the userspace-visible effects of this bug.

> Fixes: 613bec092fe7 ("mm: mmap: register suitable readonly file vmas for khugepaged")
> Signed-off-by: Dev Jain <dev.jain@arm.com>

Thanks, I'll add cc:stable.

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

[Dev Jain]

On 06/03/25 12:08 pm, Andrew Morton wrote:
> On Thu,  6 Mar 2025 12:00:37 +0530 Dev Jain <dev.jain@arm.com> 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.
> 
> Please fully describe the userspace-visible effects of this bug.

Apologies.

The userspace-visible effect should be this: khugepaged will 
unnecessarily scan mm's which haven't yet faulted in. Note that it won't 
actually collapse because all PTEs are none.

Now that I think about it, the mm is going to have a file VMA anyways 
during fork+exec, so the mm already gets registered during mmap due to 
the non-anon case (I *think*), so at least one of either the mmap 
registration or fault-time registration is redundant.

> 
>> Fixes: 613bec092fe7 ("mm: mmap: register suitable readonly file vmas for khugepaged")
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
> 
> Thanks, I'll add cc:stable.

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

[Lorenzo Stoakes]

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.

>
> 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!


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

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

[Dev Jain]

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.

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

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.

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

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

[Lorenzo Stoakes]

+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
> > >
>