| Message ID | 20230301190457.1498985-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] mm/nommu: remove unnecessary VMA locking | expand |
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] ...".
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 >
>> >> 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.
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 >
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();
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(-)