mm/vma: refactor vma_modify_flags_name() to vma_modify_name()

[PATCH] mm/vma: refactor vma_modify_flags_name() to vma_modify_name()

Posted by Lorenzo Stoakes 6 months, 4 weeks ago

The single instance in which we use this function doesn't actually need to
change VMA flags, so remove this parameter and update the caller
accordingly.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/madvise.c |  4 ++--
 mm/vma.c     |  4 +---
 mm/vma.h     | 15 +++++++--------
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 9de9b7c797c6..afa9e4db2adb 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -162,8 +162,8 @@ static int madvise_update_vma(vm_flags_t new_flags,
 		return 0;
 
 	if (set_new_anon_name)
-		vma = vma_modify_flags_name(&vmi, madv_behavior->prev, vma,
-			range->start, range->end, new_flags, anon_name);
+		vma = vma_modify_name(&vmi, madv_behavior->prev, vma,
+			range->start, range->end, anon_name);
 	else
 		vma = vma_modify_flags(&vmi, madv_behavior->prev, vma,
 			range->start, range->end, new_flags);
diff --git a/mm/vma.c b/mm/vma.c
index b3d880652359..fc502b741dcf 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -1650,17 +1650,15 @@ struct vm_area_struct *vma_modify_flags(
 }
 
 struct vm_area_struct
-*vma_modify_flags_name(struct vma_iterator *vmi,
+*vma_modify_name(struct vma_iterator *vmi,
 		       struct vm_area_struct *prev,
 		       struct vm_area_struct *vma,
 		       unsigned long start,
 		       unsigned long end,
-		       vm_flags_t vm_flags,
 		       struct anon_vma_name *new_name)
 {
 	VMG_VMA_STATE(vmg, vmi, prev, vma, start, end);
 
-	vmg.vm_flags = vm_flags;
 	vmg.anon_name = new_name;
 
 	return vma_modify(&vmg);
diff --git a/mm/vma.h b/mm/vma.h
index cf6e3a6371b6..acdcc515c459 100644
--- a/mm/vma.h
+++ b/mm/vma.h
@@ -290,15 +290,14 @@ __must_check struct vm_area_struct
 		unsigned long start, unsigned long end,
 		vm_flags_t vm_flags);
 
-/* We are about to modify the VMA's flags and/or anon_name. */
+/* We are about to modify the VMA's anon_name. */
 __must_check struct vm_area_struct
-*vma_modify_flags_name(struct vma_iterator *vmi,
-		       struct vm_area_struct *prev,
-		       struct vm_area_struct *vma,
-		       unsigned long start,
-		       unsigned long end,
-		       vm_flags_t vm_flags,
-		       struct anon_vma_name *new_name);
+*vma_modify_name(struct vma_iterator *vmi,
+		 struct vm_area_struct *prev,
+		 struct vm_area_struct *vma,
+		 unsigned long start,
+		 unsigned long end,
+		 struct anon_vma_name *new_name);
 
 /* We are about to modify the VMA's memory policy. */
 __must_check struct vm_area_struct
-- 
2.50.1

Re: [PATCH] mm/vma: refactor vma_modify_flags_name() to vma_modify_name()

Posted by Liam R. Howlett 6 months, 4 weeks ago

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250714 09:58]:
> The single instance in which we use this function doesn't actually need to
> change VMA flags, so remove this parameter and update the caller
> accordingly.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

David already pointed out the document fix, so it looks good.

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>


> ---
>  mm/madvise.c |  4 ++--
>  mm/vma.c     |  4 +---
>  mm/vma.h     | 15 +++++++--------
>  3 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 9de9b7c797c6..afa9e4db2adb 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -162,8 +162,8 @@ static int madvise_update_vma(vm_flags_t new_flags,
>  		return 0;
>  
>  	if (set_new_anon_name)
> -		vma = vma_modify_flags_name(&vmi, madv_behavior->prev, vma,
> -			range->start, range->end, new_flags, anon_name);
> +		vma = vma_modify_name(&vmi, madv_behavior->prev, vma,
> +			range->start, range->end, anon_name);
>  	else
>  		vma = vma_modify_flags(&vmi, madv_behavior->prev, vma,
>  			range->start, range->end, new_flags);
> diff --git a/mm/vma.c b/mm/vma.c
> index b3d880652359..fc502b741dcf 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -1650,17 +1650,15 @@ struct vm_area_struct *vma_modify_flags(
>  }
>  
>  struct vm_area_struct
> -*vma_modify_flags_name(struct vma_iterator *vmi,
> +*vma_modify_name(struct vma_iterator *vmi,
>  		       struct vm_area_struct *prev,
>  		       struct vm_area_struct *vma,
>  		       unsigned long start,
>  		       unsigned long end,
> -		       vm_flags_t vm_flags,
>  		       struct anon_vma_name *new_name)
>  {
>  	VMG_VMA_STATE(vmg, vmi, prev, vma, start, end);
>  
> -	vmg.vm_flags = vm_flags;
>  	vmg.anon_name = new_name;
>  
>  	return vma_modify(&vmg);
> diff --git a/mm/vma.h b/mm/vma.h
> index cf6e3a6371b6..acdcc515c459 100644
> --- a/mm/vma.h
> +++ b/mm/vma.h
> @@ -290,15 +290,14 @@ __must_check struct vm_area_struct
>  		unsigned long start, unsigned long end,
>  		vm_flags_t vm_flags);
>  
> -/* We are about to modify the VMA's flags and/or anon_name. */
> +/* We are about to modify the VMA's anon_name. */
>  __must_check struct vm_area_struct
> -*vma_modify_flags_name(struct vma_iterator *vmi,
> -		       struct vm_area_struct *prev,
> -		       struct vm_area_struct *vma,
> -		       unsigned long start,
> -		       unsigned long end,
> -		       vm_flags_t vm_flags,
> -		       struct anon_vma_name *new_name);
> +*vma_modify_name(struct vma_iterator *vmi,
> +		 struct vm_area_struct *prev,
> +		 struct vm_area_struct *vma,
> +		 unsigned long start,
> +		 unsigned long end,
> +		 struct anon_vma_name *new_name);
>  
>  /* We are about to modify the VMA's memory policy. */
>  __must_check struct vm_area_struct
> -- 
> 2.50.1
>

Re: [PATCH] mm/vma: refactor vma_modify_flags_name() to vma_modify_name()

Posted by Lorenzo Stoakes 6 months, 4 weeks ago

Hi Andrew,

Trivial comment fixup fix-patch enclosed, please apply on top of this.

Thanks! :)

----8<----
From a5bf29319f6d2880f3269ad87f6df7fd63f0860c Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Mon, 14 Jul 2025 15:29:59 +0100
Subject: [PATCH] mm/madvise: correct comment

We only either update vm_flags or anon vma name, not a combination of the
two.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/madvise.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index afa9e4db2adb..bb80fc5ea08f 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -145,8 +145,8 @@ static int replace_anon_vma_name(struct vm_area_struct *vma,
 }
 #endif /* CONFIG_ANON_VMA_NAME */
 /*
- * Update the vm_flags and/or anon_name on region of a vma, splitting it or
- * merging it as necessary. Must be called with mmap_lock held for writing.
+ * Update the vm_flags or anon_name on region of a vma, splitting it or merging
+ * it as necessary. Must be called with mmap_lock held for writing.
  */
 static int madvise_update_vma(vm_flags_t new_flags,
 		struct madvise_behavior *madv_behavior)
--
2.50.1

Re: [PATCH] mm/vma: refactor vma_modify_flags_name() to vma_modify_name()

Posted by David Hildenbrand 6 months, 4 weeks ago

On 14.07.25 15:58, Lorenzo Stoakes wrote:
> The single instance in which we use this function doesn't actually need to
> change VMA flags, so remove this parameter and update the caller
> accordingly.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>   mm/madvise.c |  4 ++--
>   mm/vma.c     |  4 +---
>   mm/vma.h     | 15 +++++++--------
>   3 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 9de9b7c797c6..afa9e4db2adb 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -162,8 +162,8 @@ static int madvise_update_vma(vm_flags_t new_flags,
>   		return 0;
>   
>   	if (set_new_anon_name)
> -		vma = vma_modify_flags_name(&vmi, madv_behavior->prev, vma,
> -			range->start, range->end, new_flags, anon_name);
> +		vma = vma_modify_name(&vmi, madv_behavior->prev, vma,
> +			range->start, range->end, anon_name);
>   	else

The doc of madvise_update_vma() is a bit misleading:

"Update the vm_flags and/or anon_name"

I assume it's xor? Do we want to sanity check that this will hold true?

In general, LGTM.

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/vma: refactor vma_modify_flags_name() to vma_modify_name()

Posted by Lorenzo Stoakes 6 months, 4 weeks ago

On Mon, Jul 14, 2025 at 04:18:13PM +0200, David Hildenbrand wrote:
> On 14.07.25 15:58, Lorenzo Stoakes wrote:
> > The single instance in which we use this function doesn't actually need to
> > change VMA flags, so remove this parameter and update the caller
> > accordingly.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >   mm/madvise.c |  4 ++--
> >   mm/vma.c     |  4 +---
> >   mm/vma.h     | 15 +++++++--------
> >   3 files changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 9de9b7c797c6..afa9e4db2adb 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -162,8 +162,8 @@ static int madvise_update_vma(vm_flags_t new_flags,
> >   		return 0;
> >   	if (set_new_anon_name)
> > -		vma = vma_modify_flags_name(&vmi, madv_behavior->prev, vma,
> > -			range->start, range->end, new_flags, anon_name);
> > +		vma = vma_modify_name(&vmi, madv_behavior->prev, vma,
> > +			range->start, range->end, anon_name);
> >   	else
>
> The doc of madvise_update_vma() is a bit misleading:
>
> "Update the vm_flags and/or anon_name"
>
> I assume it's xor? Do we want to sanity check that this will hold true?

Yeah should be xor, this is just a remnant of a past age :P

No need for sanity check I'd say, as the only code path that does this is very
specific - with behaviour of __MADV_SET_ANON_VMA_NAME and new_flags not updated
from vma->vm_flags.

I can send a fix-patch to update the comment for madvise_update_vma().

>
> In general, LGTM.

Cheers!

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

Re: [PATCH] mm/vma: refactor vma_modify_flags_name() to vma_modify_name()

Posted by David Hildenbrand 6 months, 4 weeks ago

On 14.07.25 16:24, Lorenzo Stoakes wrote:
> On Mon, Jul 14, 2025 at 04:18:13PM +0200, David Hildenbrand wrote:
>> On 14.07.25 15:58, Lorenzo Stoakes wrote:
>>> The single instance in which we use this function doesn't actually need to
>>> change VMA flags, so remove this parameter and update the caller
>>> accordingly.
>>>
>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>> ---
>>>    mm/madvise.c |  4 ++--
>>>    mm/vma.c     |  4 +---
>>>    mm/vma.h     | 15 +++++++--------
>>>    3 files changed, 10 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/mm/madvise.c b/mm/madvise.c
>>> index 9de9b7c797c6..afa9e4db2adb 100644
>>> --- a/mm/madvise.c
>>> +++ b/mm/madvise.c
>>> @@ -162,8 +162,8 @@ static int madvise_update_vma(vm_flags_t new_flags,
>>>    		return 0;
>>>    	if (set_new_anon_name)
>>> -		vma = vma_modify_flags_name(&vmi, madv_behavior->prev, vma,
>>> -			range->start, range->end, new_flags, anon_name);
>>> +		vma = vma_modify_name(&vmi, madv_behavior->prev, vma,
>>> +			range->start, range->end, anon_name);
>>>    	else
>>
>> The doc of madvise_update_vma() is a bit misleading:
>>
>> "Update the vm_flags and/or anon_name"
>>
>> I assume it's xor? Do we want to sanity check that this will hold true?
> 
> Yeah should be xor, this is just a remnant of a past age :P
> 
> No need for sanity check I'd say, as the only code path that does this is very
> specific - with behaviour of __MADV_SET_ANON_VMA_NAME and new_flags not updated
> from vma->vm_flags.
> 
> I can send a fix-patch to update the comment for madvise_update_vma().

With that

Acked-by: David Hildenbrand <david@redhat.com>

Cheers buddy!

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/vma: refactor vma_modify_flags_name() to vma_modify_name()

Posted by Vlastimil Babka 6 months, 4 weeks ago

On 7/14/25 15:58, Lorenzo Stoakes wrote:
> The single instance in which we use this function doesn't actually need to
> change VMA flags, so remove this parameter and update the caller
> accordingly.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>