[PATCH 1/1] mm/nommu: remove unnecessary VMA locking

[PATCH 1/1] mm/nommu: remove unnecessary VMA locking

[Suren Baghdasaryan]

Since CONFIG_PER_VMA_LOCK depends on CONFIG_MMU, the changes in nommu
are not needed. Remove them.

Fixes: bad94decd6a4 ("mm: write-lock VMAs before removing them from VMA tree")
Reported-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Link: https://lore.kernel.org/all/Y%2F8CJQGNuMUTdLwP@localhost/
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
Fix cleanly applies over mm-unstable, SHA in "Fixes" is from that tree.

 mm/nommu.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/mm/nommu.c b/mm/nommu.c
index 2ab162d773e2..57ba243c6a37 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -588,7 +588,6 @@ static int delete_vma_from_mm(struct vm_area_struct *vma)
 		       current->pid);
 		return -ENOMEM;
 	}
-	vma_start_write(vma);
 	cleanup_vma_from_mm(vma);
 
 	/* remove from the MM's tree and list */
@@ -1520,10 +1519,6 @@ void exit_mmap(struct mm_struct *mm)
 	 */
 	mmap_write_lock(mm);
 	for_each_vma(vmi, vma) {
-		/*
-		 * No need to lock VMA because this is the only mm user and no
-		 * page fault handled can race with it.
-		 */
 		cleanup_vma_from_mm(vma);
 		delete_vma(mm, vma);
 		cond_resched();
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

Re: [PATCH 1/1] mm/nommu: remove unnecessary VMA locking

[David Hildenbrand]

On 01.03.23 20:04, Suren Baghdasaryan wrote:
> Since CONFIG_PER_VMA_LOCK depends on CONFIG_MMU, the changes in nommu
> are not needed. Remove them.
> 
> Fixes: bad94decd6a4 ("mm: write-lock VMAs before removing them from VMA tree")
> Reported-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Link: https://lore.kernel.org/all/Y%2F8CJQGNuMUTdLwP@localhost/
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
> Fix cleanly applies over mm-unstable, SHA in "Fixes" is from that tree.
> 
>   mm/nommu.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 2ab162d773e2..57ba243c6a37 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -588,7 +588,6 @@ static int delete_vma_from_mm(struct vm_area_struct *vma)
>   		       current->pid);
>   		return -ENOMEM;
>   	}
> -	vma_start_write(vma);
>   	cleanup_vma_from_mm(vma);
>   
>   	/* remove from the MM's tree and list */
> @@ -1520,10 +1519,6 @@ void exit_mmap(struct mm_struct *mm)
>   	 */
>   	mmap_write_lock(mm);
>   	for_each_vma(vmi, vma) {
> -		/*
> -		 * No need to lock VMA because this is the only mm user and no
> -		 * page fault handled can race with it.
> -		 */
>   		cleanup_vma_from_mm(vma);
>   		delete_vma(mm, vma);
>   		cond_resched();

So, i assume this should be squashed.

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


Just a general comment: usually, if review of the original series is 
still going on, it makes a lot more sense to raise such things in the 
original series so the author can fixup while things are still in 
mm-unstable. Once the series is in mm-stable, it's a different story. In 
that case, it is usually good to have the mail subjects be something 
like  "[PATCH mm-stable 1/1] ...".

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/1] mm/nommu: remove unnecessary VMA locking

[Suren Baghdasaryan]

On Thu, Mar 2, 2023 at 1:41 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 01.03.23 20:04, Suren Baghdasaryan wrote:
> > Since CONFIG_PER_VMA_LOCK depends on CONFIG_MMU, the changes in nommu
> > are not needed. Remove them.
> >
> > Fixes: bad94decd6a4 ("mm: write-lock VMAs before removing them from VMA tree")
> > Reported-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > Link: https://lore.kernel.org/all/Y%2F8CJQGNuMUTdLwP@localhost/
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> > Fix cleanly applies over mm-unstable, SHA in "Fixes" is from that tree.
> >
> >   mm/nommu.c | 5 -----
> >   1 file changed, 5 deletions(-)
> >
> > diff --git a/mm/nommu.c b/mm/nommu.c
> > index 2ab162d773e2..57ba243c6a37 100644
> > --- a/mm/nommu.c
> > +++ b/mm/nommu.c
> > @@ -588,7 +588,6 @@ static int delete_vma_from_mm(struct vm_area_struct *vma)
> >                      current->pid);
> >               return -ENOMEM;
> >       }
> > -     vma_start_write(vma);
> >       cleanup_vma_from_mm(vma);
> >
> >       /* remove from the MM's tree and list */
> > @@ -1520,10 +1519,6 @@ void exit_mmap(struct mm_struct *mm)
> >        */
> >       mmap_write_lock(mm);
> >       for_each_vma(vmi, vma) {
> > -             /*
> > -              * No need to lock VMA because this is the only mm user and no
> > -              * page fault handled can race with it.
> > -              */
> >               cleanup_vma_from_mm(vma);
> >               delete_vma(mm, vma);
> >               cond_resched();
>
> So, i assume this should be squashed.

Yes, that would be best.

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

Thanks1

>
>
> Just a general comment: usually, if review of the original series is
> still going on, it makes a lot more sense to raise such things in the
> original series so the author can fixup while things are still in
> mm-unstable. Once the series is in mm-stable, it's a different story. In
> that case, it is usually good to have the mail subjects be something
> like  "[PATCH mm-stable 1/1] ...".

Ok... For my education, do you mean the title of this patch should
somehow reflect that it should be folded into the original patch? Just
trying to understand the actionable item here. How would you change
this patch when posting for mm-unstable and for mm-stable?
Thanks,
Suren.

>
> --
> Thanks,
>
> David / dhildenb
>

Re: [PATCH 1/1] mm/nommu: remove unnecessary VMA locking

[David Hildenbrand]

>>
>> Just a general comment: usually, if review of the original series is
>> still going on, it makes a lot more sense to raise such things in the
>> original series so the author can fixup while things are still in
>> mm-unstable. Once the series is in mm-stable, it's a different story. In
>> that case, it is usually good to have the mail subjects be something
>> like  "[PATCH mm-stable 1/1] ...".
> 
> Ok... For my education, do you mean the title of this patch should
> somehow reflect that it should be folded into the original patch? Just
> trying to understand the actionable item here. How would you change
> this patch when posting for mm-unstable and for mm-stable?

For patches that fixup something in mm-stable (stable commit ID but not 
yet master -> we cannot squash anymore so we need separate commits), 
it's good to include "mm-stable". The main difference to patches that 
target master is that by indicating "mm-stable", everyone knows that 
this is not broken in some upstream/production kernel.


For patches that fixup something that is in mm-unstable (no stable 
commit ID -> still under review and fixup easily possible), IMHO we 
distinguish between two cases:

(1) You fixup your own patches: simply send the fixup as reply to the 
original patch. Andrew will pick it up and squash it before including it 
in mm-stable. Sometimes a complete resend of a series makes sense instead.

(2) You fixup patches from someone else: simply raise it as a review 
comment in reply to the original patch. It might make sense to send a 
patch, but usually you just raise the issue to the patch author as a 
review comment and the author will address that. Again, Andrew will pick 
it up and squash it before moving it to mm-stable.


That way, it's clearer when stumbling over patches on the mailing list 
if they fix a real issue in upstream, fix a issue in 
soon-to-be-upstream, or are simply part of a WIP series that is still 
under review.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/1] mm/nommu: remove unnecessary VMA locking

[Suren Baghdasaryan]

On Fri, Mar 3, 2023 at 1:05 AM David Hildenbrand <david@redhat.com> wrote:
>
> >>
> >> Just a general comment: usually, if review of the original series is
> >> still going on, it makes a lot more sense to raise such things in the
> >> original series so the author can fixup while things are still in
> >> mm-unstable. Once the series is in mm-stable, it's a different story. In
> >> that case, it is usually good to have the mail subjects be something
> >> like  "[PATCH mm-stable 1/1] ...".
> >
> > Ok... For my education, do you mean the title of this patch should
> > somehow reflect that it should be folded into the original patch? Just
> > trying to understand the actionable item here. How would you change
> > this patch when posting for mm-unstable and for mm-stable?
>
> For patches that fixup something in mm-stable (stable commit ID but not
> yet master -> we cannot squash anymore so we need separate commits),
> it's good to include "mm-stable". The main difference to patches that
> target master is that by indicating "mm-stable", everyone knows that
> this is not broken in some upstream/production kernel.
>
>
> For patches that fixup something that is in mm-unstable (no stable
> commit ID -> still under review and fixup easily possible), IMHO we
> distinguish between two cases:
>
> (1) You fixup your own patches: simply send the fixup as reply to the
> original patch. Andrew will pick it up and squash it before including it
> in mm-stable. Sometimes a complete resend of a series makes sense instead.
>
> (2) You fixup patches from someone else: simply raise it as a review
> comment in reply to the original patch. It might make sense to send a
> patch, but usually you just raise the issue to the patch author as a
> review comment and the author will address that. Again, Andrew will pick
> it up and squash it before moving it to mm-stable.
>
>
> That way, it's clearer when stumbling over patches on the mailing list
> if they fix a real issue in upstream, fix a issue in
> soon-to-be-upstream, or are simply part of a WIP series that is still
> under review.

Thanks for the detailed explanation, David. I'll post fixups to
mm-unstable patches by replying to the original ones from now on.
Interestingly enough, I have another fix today (internal syzcaller
found a potential deadlock) which might be interesting enough to be in
a separate patch. So, I'll post it as a separate patch and we can
discuss whether it should be squashed or kept apart.
Thanks,
Suren.

>
> --
> Thanks,
>
> David / dhildenb
>