| Message ID | 20230707043211.3682710-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mm: lock a vma before stack expansion | expand |
On Thu, 6 Jul 2023 21:32:10 -0700 Suren Baghdasaryan <surenb@google.com> wrote: > With recent changes necessitating mmap_lock to be held for write while > expanding a stack, per-VMA locks should follow the same rules and be > write-locked to prevent page faults into the VMA being expanded. Add > the necessary locking. What are the possible runtime effects of this change? > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1977,6 +1977,8 @@ static int expand_upwards(struct vm_area_struct *vma, unsigned long address) > return -ENOMEM; > } > > + /* Lock the VMA before expanding to prevent concurrent page faults */ > + vma_start_write(vma); > /* > * vma->vm_start/vm_end cannot change under us because the caller > * is required to hold the mmap_lock in read mode. We need the > @@ -2064,6 +2066,8 @@ int expand_downwards(struct vm_area_struct *vma, unsigned long address) > return -ENOMEM; > } > > + /* Lock the VMA before expanding to prevent concurrent page faults */ > + vma_start_write(vma); > /* > * vma->vm_start/vm_end cannot change under us because the caller > * is required to hold the mmap_lock in read mode. We need the > -- > 2.41.0.255.g8b1d071c50-goog
… > write-locked to prevent page faults into the VMA being expanded. Add > the necessary locking. 1. Would it a bit nicer to put the second sentence on a separate line in such a change description? 2. I noticed that you put the address “stable@vger.kernel.org” into the message field “Cc”. Would you like to specify such a hint as a tag? See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n264 3. How do you think about to add the tag “Fixes”? 4. Will a cover letter become helpful also for the presented small patch series? Regards, Markus
On Fri, Jul 7, 2023 at 7:27 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 6 Jul 2023 21:32:10 -0700 Suren Baghdasaryan <surenb@google.com> wrote: > > > With recent changes necessitating mmap_lock to be held for write while > > expanding a stack, per-VMA locks should follow the same rules and be > > write-locked to prevent page faults into the VMA being expanded. Add > > the necessary locking. > > What are the possible runtime effects of this change? During the stack expansion concurrent page faults would have to wait and visa versa (stack expansion would have to wait if there are ongoing page faults). I think it's the same runtime effects as the recent similar change requiring mmap_lock to be write lock before stack expansion. > > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -1977,6 +1977,8 @@ static int expand_upwards(struct vm_area_struct *vma, unsigned long address) > > return -ENOMEM; > > } > > > > + /* Lock the VMA before expanding to prevent concurrent page faults */ > > + vma_start_write(vma); > > /* > > * vma->vm_start/vm_end cannot change under us because the caller > > * is required to hold the mmap_lock in read mode. We need the > > @@ -2064,6 +2066,8 @@ int expand_downwards(struct vm_area_struct *vma, unsigned long address) > > return -ENOMEM; > > } > > > > + /* Lock the VMA before expanding to prevent concurrent page faults */ > > + vma_start_write(vma); > > /* > > * vma->vm_start/vm_end cannot change under us because the caller > > * is required to hold the mmap_lock in read mode. We need the > > -- > > 2.41.0.255.g8b1d071c50-goog > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On Fri, Jul 07, 2023 at 10:00:42PM +0200, Markus Elfring wrote: > … > > write-locked to prevent page faults into the VMA being expanded. Add > > the necessary locking. > > 1. Would it a bit nicer to put the second sentence on a separate line > in such a change description? > > 2. I noticed that you put the address “stable@vger.kernel.org” > into the message field “Cc”. > Would you like to specify such a hint as a tag? > > See also: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n264 > > 3. How do you think about to add the tag “Fixes”? > > 4. Will a cover letter become helpful also for the presented small patch series? Markus, your nitpicking is not useful. Please stop.
On Fri, Jul 7, 2023 at 8:03 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Jul 07, 2023 at 10:00:42PM +0200, Markus Elfring wrote: > > … > > > write-locked to prevent page faults into the VMA being expanded. Add > > > the necessary locking. > > > > 1. Would it a bit nicer to put the second sentence on a separate line > > in such a change description? Maybe. Will do if there is a need to post a v2. > > > > 2. I noticed that you put the address “stable@vger.kernel.org” > > into the message field “Cc”. > > Would you like to specify such a hint as a tag? > > > > See also: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n264 Yeah, I always forget that :( > > > > 3. How do you think about to add the tag “Fixes”? I thought about it but was not sure which patch I should list under such tag because the rules for stack expansion changed recently. > > > > 4. Will a cover letter become helpful also for the presented small patch series? Not much to say other than "add some missing locking" :) > > Markus, your nitpicking is not useful. Please stop. I'll fix the nits, at least the ones I can, if there is a need for v2.
>> … >>> write-locked to prevent page faults into the VMA being expanded. Add >>> the necessary locking. >> >> 1. Would it a bit nicer to put the second sentence on a separate line >> in such a change description? >> >> 2. I noticed that you put the address “stable@vger.kernel.org” >> into the message field “Cc”. >> Would you like to specify such a hint as a tag? >> >> See also: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n264 >> >> 3. How do you think about to add the tag “Fixes”? >> >> 4. Will a cover letter become helpful also for the presented small patch series? > > Markus, your nitpicking is not useful. … Do you like patch reviews more by other contributors? Regards, Markus
… >> Markus, your nitpicking is not useful. Please stop. > > I'll fix the nits, at least the ones I can, if there is a need for v2. Thanks for such a constructive feedback. Would you like to take any further information better into account for subsequent patch variants? Regards, Markus
On Fri, Jul 7, 2023 at 10:55 PM Markus Elfring <Markus.Elfring@web.de> wrote: > > … > >> Markus, your nitpicking is not useful. Please stop. > > > > I'll fix the nits, at least the ones I can, if there is a need for v2. > > Thanks for such a constructive feedback. > > Would you like to take any further information better into account > for subsequent patch variants? All feedback is appreciated. The comments you provided so far seem to be nice-to-haves but not critical enough to post a new version of the patch IMHO. If something more substantial is found that requires a new version then I'll address your feedback there as well. Thanks, Suren. > > Regards, > Markus
>> Would you like to take any further information better into account >> for subsequent patch variants? > > All feedback is appreciated. Thanks for such a positive response. > The comments you provided so far seem to > be nice-to-haves but not critical enough to post a new version of the > patch IMHO. I guess that our dialogue can increase the probability for more desirable improvements. I got used to point items out which can affect also the development process for Linux software components. It seems that some contributors stumble still on difficulties to handle presented patch review approaches in more constructive ways. > If something more substantial is found that requires a new > version then I'll address your feedback there as well. Will the chances grow to pick further recommendations, guidelines and related change possibilities up? Regards, Markus
diff --git a/mm/mmap.c b/mm/mmap.c index 204ddcd52625..c66e4622a557 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1977,6 +1977,8 @@ static int expand_upwards(struct vm_area_struct *vma, unsigned long address) return -ENOMEM; } + /* Lock the VMA before expanding to prevent concurrent page faults */ + vma_start_write(vma); /* * vma->vm_start/vm_end cannot change under us because the caller * is required to hold the mmap_lock in read mode. We need the @@ -2064,6 +2066,8 @@ int expand_downwards(struct vm_area_struct *vma, unsigned long address) return -ENOMEM; } + /* Lock the VMA before expanding to prevent concurrent page faults */ + vma_start_write(vma); /* * vma->vm_start/vm_end cannot change under us because the caller * is required to hold the mmap_lock in read mode. We need the
With recent changes necessitating mmap_lock to be held for write while expanding a stack, per-VMA locks should follow the same rules and be write-locked to prevent page faults into the VMA being expanded. Add the necessary locking. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- mm/mmap.c | 4 ++++ 1 file changed, 4 insertions(+)