| Message ID | 20240605002459.4091285-2-andrii@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ioctl()-based API to query VMAs from /proc/<pid>/maps | expand |
On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > +/* > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > + * next VMA. Search is done under RCU protection, without taking or assuming > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. You know this is supposed to be the _short_ description, right? Three lines is way too long. The full description goes between the arguments and the Return: line. > + * @mm: The mm_struct to check > + * @addr: The address > + * > + * Returns: The VMA associated with addr, or the next VMA. > + * May return %NULL in the case of no VMA at addr or above. > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > + */ > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > + unsigned long address) > +{ > + MA_STATE(mas, &mm->mm_mt, address, address); > + struct vm_area_struct *vma; > + int err; > + > + rcu_read_lock(); > +retry: > + vma = mas_find(&mas, ULONG_MAX); > + if (!vma) { > + err = 0; /* no VMA, return NULL */ > + goto inval; > + } > + > + if (!vma_start_read(vma)) { > + err = -EBUSY; > + goto inval; > + } > + > + /* > + * Check since vm_start/vm_end might change before we lock the VMA. > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > + * address or the next one, so we only make sure VMA wasn't updated to > + * end before the address. > + */ > + if (unlikely(vma->vm_end <= address)) { > + err = -EBUSY; > + goto inval_end_read; > + } > + > + /* Check if the VMA got isolated after we found it */ > + if (vma->detached) { > + vma_end_read(vma); > + count_vm_vma_lock_event(VMA_LOCK_MISS); > + /* The area was replaced with another one */ Surely you need to mas_reset() before you goto retry? > + goto retry; > + }
* Matthew Wilcox <willy@infradead.org> [240604 20:57]: > On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > > +/* > > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > > + * next VMA. Search is done under RCU protection, without taking or assuming > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. > > You know this is supposed to be the _short_ description, right? > Three lines is way too long. The full description goes between the > arguments and the Return: line. > > > + * @mm: The mm_struct to check > > + * @addr: The address > > + * > > + * Returns: The VMA associated with addr, or the next VMA. > > + * May return %NULL in the case of no VMA at addr or above. > > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > > + */ > > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > > + unsigned long address) > > +{ > > + MA_STATE(mas, &mm->mm_mt, address, address); > > + struct vm_area_struct *vma; > > + int err; > > + > > + rcu_read_lock(); > > +retry: > > + vma = mas_find(&mas, ULONG_MAX); > > + if (!vma) { > > + err = 0; /* no VMA, return NULL */ > > + goto inval; > > + } > > + > > + if (!vma_start_read(vma)) { > > + err = -EBUSY; > > + goto inval; > > + } > > + > > + /* > > + * Check since vm_start/vm_end might change before we lock the VMA. > > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > > + * address or the next one, so we only make sure VMA wasn't updated to > > + * end before the address. > > + */ > > + if (unlikely(vma->vm_end <= address)) { > > + err = -EBUSY; > > + goto inval_end_read; > > + } > > + > > + /* Check if the VMA got isolated after we found it */ > > + if (vma->detached) { > > + vma_end_read(vma); > > + count_vm_vma_lock_event(VMA_LOCK_MISS); > > + /* The area was replaced with another one */ > > Surely you need to mas_reset() before you goto retry? Probably more than that. We've found and may have adjusted the index/last; we should reconfigure the maple state. You should probably use mas_set(), which will reset the maple state and set the index and long to address. > > > + goto retry; > > + } >
On Wed, Jun 5, 2024 at 6:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > * Matthew Wilcox <willy@infradead.org> [240604 20:57]: > > On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > > > +/* > > > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > > > + * next VMA. Search is done under RCU protection, without taking or assuming > > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. > > > > You know this is supposed to be the _short_ description, right? > > Three lines is way too long. The full description goes between the > > arguments and the Return: line. Sure, I'll adjust. > > > > > + * @mm: The mm_struct to check > > > + * @addr: The address > > > + * > > > + * Returns: The VMA associated with addr, or the next VMA. > > > + * May return %NULL in the case of no VMA at addr or above. > > > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > > > + */ > > > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > > > + unsigned long address) > > > +{ > > > + MA_STATE(mas, &mm->mm_mt, address, address); > > > + struct vm_area_struct *vma; > > > + int err; > > > + > > > + rcu_read_lock(); > > > +retry: > > > + vma = mas_find(&mas, ULONG_MAX); > > > + if (!vma) { > > > + err = 0; /* no VMA, return NULL */ > > > + goto inval; > > > + } > > > + > > > + if (!vma_start_read(vma)) { > > > + err = -EBUSY; > > > + goto inval; > > > + } > > > + > > > + /* > > > + * Check since vm_start/vm_end might change before we lock the VMA. > > > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > > > + * address or the next one, so we only make sure VMA wasn't updated to > > > + * end before the address. > > > + */ > > > + if (unlikely(vma->vm_end <= address)) { > > > + err = -EBUSY; > > > + goto inval_end_read; > > > + } > > > + > > > + /* Check if the VMA got isolated after we found it */ > > > + if (vma->detached) { > > > + vma_end_read(vma); > > > + count_vm_vma_lock_event(VMA_LOCK_MISS); > > > + /* The area was replaced with another one */ > > > > Surely you need to mas_reset() before you goto retry? > > Probably more than that. We've found and may have adjusted the > index/last; we should reconfigure the maple state. You should probably > use mas_set(), which will reset the maple state and set the index and > long to address. Yep, makes sense, thanks. As for the `unlikely(vma->vm_end <= address)` case, I presume we want to do the same, right? Basically, on each retry start from the `address` unconditionally, no matter what's the reason for retry. > > > > > > > + goto retry; > > > + } > >
On Wed, Jun 5, 2024 at 9:13 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Jun 5, 2024 at 6:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > * Matthew Wilcox <willy@infradead.org> [240604 20:57]: > > > On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > > > > +/* > > > > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > > > > + * next VMA. Search is done under RCU protection, without taking or assuming > > > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. > > > > > > You know this is supposed to be the _short_ description, right? > > > Three lines is way too long. The full description goes between the > > > arguments and the Return: line. > > Sure, I'll adjust. > > > > > > > > + * @mm: The mm_struct to check > > > > + * @addr: The address > > > > + * > > > > + * Returns: The VMA associated with addr, or the next VMA. > > > > + * May return %NULL in the case of no VMA at addr or above. > > > > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > > > > + */ > > > > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > > > > + unsigned long address) > > > > +{ > > > > + MA_STATE(mas, &mm->mm_mt, address, address); > > > > + struct vm_area_struct *vma; > > > > + int err; > > > > + > > > > + rcu_read_lock(); > > > > +retry: > > > > + vma = mas_find(&mas, ULONG_MAX); > > > > + if (!vma) { > > > > + err = 0; /* no VMA, return NULL */ > > > > + goto inval; > > > > + } > > > > + > > > > + if (!vma_start_read(vma)) { > > > > + err = -EBUSY; > > > > + goto inval; > > > > + } > > > > + > > > > + /* > > > > + * Check since vm_start/vm_end might change before we lock the VMA. > > > > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > > > > + * address or the next one, so we only make sure VMA wasn't updated to > > > > + * end before the address. > > > > + */ > > > > + if (unlikely(vma->vm_end <= address)) { > > > > + err = -EBUSY; > > > > + goto inval_end_read; > > > > + } > > > > + > > > > + /* Check if the VMA got isolated after we found it */ > > > > + if (vma->detached) { > > > > + vma_end_read(vma); > > > > + count_vm_vma_lock_event(VMA_LOCK_MISS); > > > > + /* The area was replaced with another one */ > > > > > > Surely you need to mas_reset() before you goto retry? > > > > Probably more than that. We've found and may have adjusted the > > index/last; we should reconfigure the maple state. You should probably > > use mas_set(), which will reset the maple state and set the index and > > long to address. > > Yep, makes sense, thanks. As for the `unlikely(vma->vm_end <= > address)` case, I presume we want to do the same, right? Basically, on > each retry start from the `address` unconditionally, no matter what's > the reason for retry. ah, never mind, we don't retry in that situation, I'll just put `mas_set(&mas, address);` right before `goto retry;`. Unless we should actually retry in the case when VMA got moved before the requested address, not sure, let me know what you think. Presumably retrying will allow us to get the correct VMA without the need to fall back to mmap_lock? > > > > > > > > > > > > + goto retry; > > > > + } > > >
On Wed, Jun 5, 2024 at 9:24 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Jun 5, 2024 at 9:13 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Jun 5, 2024 at 6:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > * Matthew Wilcox <willy@infradead.org> [240604 20:57]: > > > > On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > > > > > +/* > > > > > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > > > > > + * next VMA. Search is done under RCU protection, without taking or assuming > > > > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. > > > > > > > > You know this is supposed to be the _short_ description, right? > > > > Three lines is way too long. The full description goes between the > > > > arguments and the Return: line. > > > > Sure, I'll adjust. > > > > > > > > > > > + * @mm: The mm_struct to check > > > > > + * @addr: The address > > > > > + * > > > > > + * Returns: The VMA associated with addr, or the next VMA. > > > > > + * May return %NULL in the case of no VMA at addr or above. > > > > > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > > > > > + */ > > > > > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > > > > > + unsigned long address) > > > > > +{ > > > > > + MA_STATE(mas, &mm->mm_mt, address, address); > > > > > + struct vm_area_struct *vma; > > > > > + int err; > > > > > + > > > > > + rcu_read_lock(); > > > > > +retry: > > > > > + vma = mas_find(&mas, ULONG_MAX); > > > > > + if (!vma) { > > > > > + err = 0; /* no VMA, return NULL */ > > > > > + goto inval; > > > > > + } > > > > > + > > > > > + if (!vma_start_read(vma)) { > > > > > + err = -EBUSY; > > > > > + goto inval; > > > > > + } > > > > > + > > > > > + /* > > > > > + * Check since vm_start/vm_end might change before we lock the VMA. > > > > > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > > > > > + * address or the next one, so we only make sure VMA wasn't updated to > > > > > + * end before the address. > > > > > + */ > > > > > + if (unlikely(vma->vm_end <= address)) { > > > > > + err = -EBUSY; > > > > > + goto inval_end_read; > > > > > + } > > > > > + > > > > > + /* Check if the VMA got isolated after we found it */ > > > > > + if (vma->detached) { > > > > > + vma_end_read(vma); > > > > > + count_vm_vma_lock_event(VMA_LOCK_MISS); > > > > > + /* The area was replaced with another one */ > > > > > > > > Surely you need to mas_reset() before you goto retry? > > > > > > Probably more than that. We've found and may have adjusted the > > > index/last; we should reconfigure the maple state. You should probably > > > use mas_set(), which will reset the maple state and set the index and > > > long to address. > > > > Yep, makes sense, thanks. As for the `unlikely(vma->vm_end <= > > address)` case, I presume we want to do the same, right? Basically, on > > each retry start from the `address` unconditionally, no matter what's > > the reason for retry. > > ah, never mind, we don't retry in that situation, I'll just put > `mas_set(&mas, address);` right before `goto retry;`. Unless we should > actually retry in the case when VMA got moved before the requested > address, not sure, let me know what you think. Presumably retrying > will allow us to get the correct VMA without the need to fall back to > mmap_lock? sorry, one more question as I look some more around this (unfamiliar to me) piece of code. I see that lock_vma_under_rcu counts VMA_LOCK_MISS on retry, but I see that there is actually a VMA_LOCK_RETRY stat as well. Any reason it's a MISS instead of RETRY? Should I use MISS as well, or actually count a RETRY? > > > > > > > > > > > > > > > > > > + goto retry; > > > > > + } > > > >
* Andrii Nakryiko <andrii.nakryiko@gmail.com> [240605 12:27]: > On Wed, Jun 5, 2024 at 9:24 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Jun 5, 2024 at 9:13 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Wed, Jun 5, 2024 at 6:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > > > * Matthew Wilcox <willy@infradead.org> [240604 20:57]: > > > > > On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > > > > > > +/* > > > > > > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > > > > > > + * next VMA. Search is done under RCU protection, without taking or assuming > > > > > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. > > > > > > > > > > You know this is supposed to be the _short_ description, right? > > > > > Three lines is way too long. The full description goes between the > > > > > arguments and the Return: line. > > > > > > Sure, I'll adjust. > > > > > > > > > > > > > > + * @mm: The mm_struct to check > > > > > > + * @addr: The address > > > > > > + * > > > > > > + * Returns: The VMA associated with addr, or the next VMA. > > > > > > + * May return %NULL in the case of no VMA at addr or above. > > > > > > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > > > > > > + */ > > > > > > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > > > > > > + unsigned long address) > > > > > > +{ > > > > > > + MA_STATE(mas, &mm->mm_mt, address, address); > > > > > > + struct vm_area_struct *vma; > > > > > > + int err; > > > > > > + > > > > > > + rcu_read_lock(); > > > > > > +retry: > > > > > > + vma = mas_find(&mas, ULONG_MAX); > > > > > > + if (!vma) { > > > > > > + err = 0; /* no VMA, return NULL */ > > > > > > + goto inval; > > > > > > + } > > > > > > + > > > > > > + if (!vma_start_read(vma)) { > > > > > > + err = -EBUSY; > > > > > > + goto inval; > > > > > > + } > > > > > > + > > > > > > + /* > > > > > > + * Check since vm_start/vm_end might change before we lock the VMA. > > > > > > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > > > > > > + * address or the next one, so we only make sure VMA wasn't updated to > > > > > > + * end before the address. > > > > > > + */ > > > > > > + if (unlikely(vma->vm_end <= address)) { > > > > > > + err = -EBUSY; > > > > > > + goto inval_end_read; > > > > > > + } > > > > > > + > > > > > > + /* Check if the VMA got isolated after we found it */ > > > > > > + if (vma->detached) { > > > > > > + vma_end_read(vma); > > > > > > + count_vm_vma_lock_event(VMA_LOCK_MISS); > > > > > > + /* The area was replaced with another one */ > > > > > > > > > > Surely you need to mas_reset() before you goto retry? > > > > > > > > Probably more than that. We've found and may have adjusted the > > > > index/last; we should reconfigure the maple state. You should probably > > > > use mas_set(), which will reset the maple state and set the index and > > > > long to address. > > > > > > Yep, makes sense, thanks. As for the `unlikely(vma->vm_end <= > > > address)` case, I presume we want to do the same, right? Basically, on > > > each retry start from the `address` unconditionally, no matter what's > > > the reason for retry. > > > > ah, never mind, we don't retry in that situation, I'll just put > > `mas_set(&mas, address);` right before `goto retry;`. Unless we should > > actually retry in the case when VMA got moved before the requested > > address, not sure, let me know what you think. Presumably retrying > > will allow us to get the correct VMA without the need to fall back to > > mmap_lock? > > sorry, one more question as I look some more around this (unfamiliar > to me) piece of code. I see that lock_vma_under_rcu counts > VMA_LOCK_MISS on retry, but I see that there is actually a > VMA_LOCK_RETRY stat as well. Any reason it's a MISS instead of RETRY? > Should I use MISS as well, or actually count a RETRY? > VMA_LOCK_MISS is used here because we missed the VMA due to a write happening to move the vma (rather rare). The VMA_LOCK missed the vma. VMA_LOCK_RETRY is used to indicate we need to retry under the mmap lock. A retry is needed after the VMA_LOCK did not work under rcu locking. Thanks, Liam
On Wed, Jun 5, 2024 at 10:03 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > * Andrii Nakryiko <andrii.nakryiko@gmail.com> [240605 12:27]: > > On Wed, Jun 5, 2024 at 9:24 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Wed, Jun 5, 2024 at 9:13 AM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Wed, Jun 5, 2024 at 6:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > > > > > * Matthew Wilcox <willy@infradead.org> [240604 20:57]: > > > > > > On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > > > > > > > +/* > > > > > > > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > > > > > > > + * next VMA. Search is done under RCU protection, without taking or assuming > > > > > > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. > > > > > > > > > > > > You know this is supposed to be the _short_ description, right? > > > > > > Three lines is way too long. The full description goes between the > > > > > > arguments and the Return: line. > > > > > > > > Sure, I'll adjust. > > > > > > > > > > > > > > > > > + * @mm: The mm_struct to check > > > > > > > + * @addr: The address > > > > > > > + * > > > > > > > + * Returns: The VMA associated with addr, or the next VMA. > > > > > > > + * May return %NULL in the case of no VMA at addr or above. > > > > > > > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > > > > > > > + */ > > > > > > > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > > > > > > > + unsigned long address) > > > > > > > +{ > > > > > > > + MA_STATE(mas, &mm->mm_mt, address, address); > > > > > > > + struct vm_area_struct *vma; > > > > > > > + int err; > > > > > > > + > > > > > > > + rcu_read_lock(); > > > > > > > +retry: > > > > > > > + vma = mas_find(&mas, ULONG_MAX); > > > > > > > + if (!vma) { > > > > > > > + err = 0; /* no VMA, return NULL */ > > > > > > > + goto inval; > > > > > > > + } > > > > > > > + > > > > > > > + if (!vma_start_read(vma)) { > > > > > > > + err = -EBUSY; > > > > > > > + goto inval; > > > > > > > + } > > > > > > > + > > > > > > > + /* > > > > > > > + * Check since vm_start/vm_end might change before we lock the VMA. > > > > > > > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > > > > > > > + * address or the next one, so we only make sure VMA wasn't updated to > > > > > > > + * end before the address. > > > > > > > + */ > > > > > > > + if (unlikely(vma->vm_end <= address)) { > > > > > > > + err = -EBUSY; > > > > > > > + goto inval_end_read; > > > > > > > + } > > > > > > > + > > > > > > > + /* Check if the VMA got isolated after we found it */ > > > > > > > + if (vma->detached) { > > > > > > > + vma_end_read(vma); > > > > > > > + count_vm_vma_lock_event(VMA_LOCK_MISS); > > > > > > > + /* The area was replaced with another one */ > > > > > > > > > > > > Surely you need to mas_reset() before you goto retry? > > > > > > > > > > Probably more than that. We've found and may have adjusted the > > > > > index/last; we should reconfigure the maple state. You should probably > > > > > use mas_set(), which will reset the maple state and set the index and > > > > > long to address. > > > > > > > > Yep, makes sense, thanks. As for the `unlikely(vma->vm_end <= > > > > address)` case, I presume we want to do the same, right? Basically, on > > > > each retry start from the `address` unconditionally, no matter what's > > > > the reason for retry. > > > > > > ah, never mind, we don't retry in that situation, I'll just put > > > `mas_set(&mas, address);` right before `goto retry;`. Unless we should > > > actually retry in the case when VMA got moved before the requested > > > address, not sure, let me know what you think. Presumably retrying > > > will allow us to get the correct VMA without the need to fall back to > > > mmap_lock? > > > > sorry, one more question as I look some more around this (unfamiliar > > to me) piece of code. I see that lock_vma_under_rcu counts > > VMA_LOCK_MISS on retry, but I see that there is actually a > > VMA_LOCK_RETRY stat as well. Any reason it's a MISS instead of RETRY? > > Should I use MISS as well, or actually count a RETRY? > > > > VMA_LOCK_MISS is used here because we missed the VMA due to a write > happening to move the vma (rather rare). The VMA_LOCK missed the vma. > > VMA_LOCK_RETRY is used to indicate we need to retry under the mmap lock. > A retry is needed after the VMA_LOCK did not work under rcu locking. Originally lock_vma_under_rcu() was used only inside page fault path, so these counters helped us quantify how effective VMA locking is when handling page faults. With more users of that function these counters will be affected by other paths as well. I'm not sure but I think it makes sense to use them only inside page fault path, IOW we should probably move count_vm_vma_lock_event() calls outside of lock_vma_under_rcu() and add them only when handling page faults. > > Thanks, > Liam
On Wed, Jun 5, 2024 at 4:22 PM Suren Baghdasaryan <surenb@google.com> wrote: > > On Wed, Jun 5, 2024 at 10:03 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > * Andrii Nakryiko <andrii.nakryiko@gmail.com> [240605 12:27]: > > > On Wed, Jun 5, 2024 at 9:24 AM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Wed, Jun 5, 2024 at 9:13 AM Andrii Nakryiko > > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > On Wed, Jun 5, 2024 at 6:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > > > > > > > * Matthew Wilcox <willy@infradead.org> [240604 20:57]: > > > > > > > On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > > > > > > > > +/* > > > > > > > > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > > > > > > > > + * next VMA. Search is done under RCU protection, without taking or assuming > > > > > > > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. > > > > > > > > > > > > > > You know this is supposed to be the _short_ description, right? > > > > > > > Three lines is way too long. The full description goes between the > > > > > > > arguments and the Return: line. > > > > > > > > > > Sure, I'll adjust. > > > > > > > > > > > > > > > > > > > > + * @mm: The mm_struct to check > > > > > > > > + * @addr: The address > > > > > > > > + * > > > > > > > > + * Returns: The VMA associated with addr, or the next VMA. > > > > > > > > + * May return %NULL in the case of no VMA at addr or above. > > > > > > > > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > > > > > > > > + */ > > > > > > > > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > > > > > > > > + unsigned long address) > > > > > > > > +{ > > > > > > > > + MA_STATE(mas, &mm->mm_mt, address, address); > > > > > > > > + struct vm_area_struct *vma; > > > > > > > > + int err; > > > > > > > > + > > > > > > > > + rcu_read_lock(); > > > > > > > > +retry: > > > > > > > > + vma = mas_find(&mas, ULONG_MAX); > > > > > > > > + if (!vma) { > > > > > > > > + err = 0; /* no VMA, return NULL */ > > > > > > > > + goto inval; > > > > > > > > + } > > > > > > > > + > > > > > > > > + if (!vma_start_read(vma)) { > > > > > > > > + err = -EBUSY; > > > > > > > > + goto inval; > > > > > > > > + } > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * Check since vm_start/vm_end might change before we lock the VMA. > > > > > > > > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > > > > > > > > + * address or the next one, so we only make sure VMA wasn't updated to > > > > > > > > + * end before the address. > > > > > > > > + */ > > > > > > > > + if (unlikely(vma->vm_end <= address)) { > > > > > > > > + err = -EBUSY; > > > > > > > > + goto inval_end_read; > > > > > > > > + } > > > > > > > > + > > > > > > > > + /* Check if the VMA got isolated after we found it */ > > > > > > > > + if (vma->detached) { > > > > > > > > + vma_end_read(vma); > > > > > > > > + count_vm_vma_lock_event(VMA_LOCK_MISS); > > > > > > > > + /* The area was replaced with another one */ > > > > > > > > > > > > > > Surely you need to mas_reset() before you goto retry? > > > > > > > > > > > > Probably more than that. We've found and may have adjusted the > > > > > > index/last; we should reconfigure the maple state. You should probably > > > > > > use mas_set(), which will reset the maple state and set the index and > > > > > > long to address. > > > > > > > > > > Yep, makes sense, thanks. As for the `unlikely(vma->vm_end <= > > > > > address)` case, I presume we want to do the same, right? Basically, on > > > > > each retry start from the `address` unconditionally, no matter what's > > > > > the reason for retry. > > > > > > > > ah, never mind, we don't retry in that situation, I'll just put > > > > `mas_set(&mas, address);` right before `goto retry;`. Unless we should > > > > actually retry in the case when VMA got moved before the requested > > > > address, not sure, let me know what you think. Presumably retrying > > > > will allow us to get the correct VMA without the need to fall back to > > > > mmap_lock? > > > > > > sorry, one more question as I look some more around this (unfamiliar > > > to me) piece of code. I see that lock_vma_under_rcu counts > > > VMA_LOCK_MISS on retry, but I see that there is actually a > > > VMA_LOCK_RETRY stat as well. Any reason it's a MISS instead of RETRY? > > > Should I use MISS as well, or actually count a RETRY? > > > > > > > VMA_LOCK_MISS is used here because we missed the VMA due to a write > > happening to move the vma (rather rare). The VMA_LOCK missed the vma. > > > > VMA_LOCK_RETRY is used to indicate we need to retry under the mmap lock. > > A retry is needed after the VMA_LOCK did not work under rcu locking. > > Originally lock_vma_under_rcu() was used only inside page fault path, > so these counters helped us quantify how effective VMA locking is when > handling page faults. With more users of that function these counters > will be affected by other paths as well. I'm not sure but I think it > makes sense to use them only inside page fault path, IOW we should > probably move count_vm_vma_lock_event() calls outside of > lock_vma_under_rcu() and add them only when handling page faults. Alright, seems like I should then just drop count_vm_vma_lock_event() from the API I'm adding. > > > > > Thanks, > > Liam
On Thu, Jun 6, 2024 at 9:52 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Jun 5, 2024 at 4:22 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > On Wed, Jun 5, 2024 at 10:03 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > * Andrii Nakryiko <andrii.nakryiko@gmail.com> [240605 12:27]: > > > > On Wed, Jun 5, 2024 at 9:24 AM Andrii Nakryiko > > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > On Wed, Jun 5, 2024 at 9:13 AM Andrii Nakryiko > > > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > > > On Wed, Jun 5, 2024 at 6:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > > > > > > > > > * Matthew Wilcox <willy@infradead.org> [240604 20:57]: > > > > > > > > On Tue, Jun 04, 2024 at 05:24:46PM -0700, Andrii Nakryiko wrote: > > > > > > > > > +/* > > > > > > > > > + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the > > > > > > > > > + * next VMA. Search is done under RCU protection, without taking or assuming > > > > > > > > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. > > > > > > > > > > > > > > > > You know this is supposed to be the _short_ description, right? > > > > > > > > Three lines is way too long. The full description goes between the > > > > > > > > arguments and the Return: line. > > > > > > > > > > > > Sure, I'll adjust. > > > > > > > > > > > > > > > > > > > > > > > + * @mm: The mm_struct to check > > > > > > > > > + * @addr: The address > > > > > > > > > + * > > > > > > > > > + * Returns: The VMA associated with addr, or the next VMA. > > > > > > > > > + * May return %NULL in the case of no VMA at addr or above. > > > > > > > > > + * If the VMA is being modified and can't be locked, -EBUSY is returned. > > > > > > > > > + */ > > > > > > > > > +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, > > > > > > > > > + unsigned long address) > > > > > > > > > +{ > > > > > > > > > + MA_STATE(mas, &mm->mm_mt, address, address); > > > > > > > > > + struct vm_area_struct *vma; > > > > > > > > > + int err; > > > > > > > > > + > > > > > > > > > + rcu_read_lock(); > > > > > > > > > +retry: > > > > > > > > > + vma = mas_find(&mas, ULONG_MAX); > > > > > > > > > + if (!vma) { > > > > > > > > > + err = 0; /* no VMA, return NULL */ > > > > > > > > > + goto inval; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + if (!vma_start_read(vma)) { > > > > > > > > > + err = -EBUSY; > > > > > > > > > + goto inval; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + /* > > > > > > > > > + * Check since vm_start/vm_end might change before we lock the VMA. > > > > > > > > > + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering > > > > > > > > > + * address or the next one, so we only make sure VMA wasn't updated to > > > > > > > > > + * end before the address. > > > > > > > > > + */ > > > > > > > > > + if (unlikely(vma->vm_end <= address)) { > > > > > > > > > + err = -EBUSY; > > > > > > > > > + goto inval_end_read; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + /* Check if the VMA got isolated after we found it */ > > > > > > > > > + if (vma->detached) { > > > > > > > > > + vma_end_read(vma); > > > > > > > > > + count_vm_vma_lock_event(VMA_LOCK_MISS); > > > > > > > > > + /* The area was replaced with another one */ > > > > > > > > > > > > > > > > Surely you need to mas_reset() before you goto retry? > > > > > > > > > > > > > > Probably more than that. We've found and may have adjusted the > > > > > > > index/last; we should reconfigure the maple state. You should probably > > > > > > > use mas_set(), which will reset the maple state and set the index and > > > > > > > long to address. > > > > > > > > > > > > Yep, makes sense, thanks. As for the `unlikely(vma->vm_end <= > > > > > > address)` case, I presume we want to do the same, right? Basically, on > > > > > > each retry start from the `address` unconditionally, no matter what's > > > > > > the reason for retry. > > > > > > > > > > ah, never mind, we don't retry in that situation, I'll just put > > > > > `mas_set(&mas, address);` right before `goto retry;`. Unless we should > > > > > actually retry in the case when VMA got moved before the requested > > > > > address, not sure, let me know what you think. Presumably retrying > > > > > will allow us to get the correct VMA without the need to fall back to > > > > > mmap_lock? > > > > > > > > sorry, one more question as I look some more around this (unfamiliar > > > > to me) piece of code. I see that lock_vma_under_rcu counts > > > > VMA_LOCK_MISS on retry, but I see that there is actually a > > > > VMA_LOCK_RETRY stat as well. Any reason it's a MISS instead of RETRY? > > > > Should I use MISS as well, or actually count a RETRY? > > > > > > > > > > VMA_LOCK_MISS is used here because we missed the VMA due to a write > > > happening to move the vma (rather rare). The VMA_LOCK missed the vma. > > > > > > VMA_LOCK_RETRY is used to indicate we need to retry under the mmap lock. > > > A retry is needed after the VMA_LOCK did not work under rcu locking. > > > > Originally lock_vma_under_rcu() was used only inside page fault path, > > so these counters helped us quantify how effective VMA locking is when > > handling page faults. With more users of that function these counters > > will be affected by other paths as well. I'm not sure but I think it > > makes sense to use them only inside page fault path, IOW we should > > probably move count_vm_vma_lock_event() calls outside of > > lock_vma_under_rcu() and add them only when handling page faults. > > Alright, seems like I should then just drop count_vm_vma_lock_event() > from the API I'm adding. That would be my preference but as I said, I'm not 100% sure about this direction. > > > > > > > > > Thanks, > > > Liam
diff --git a/include/linux/mm.h b/include/linux/mm.h index c41c82bcbec2..3ab52b7e124c 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -776,6 +776,8 @@ static inline void assert_fault_locked(struct vm_fault *vmf) mmap_assert_locked(vmf->vma->vm_mm); } +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, + unsigned long address); struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, unsigned long address); @@ -790,6 +792,12 @@ static inline void vma_assert_write_locked(struct vm_area_struct *vma) static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached) {} +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, + unsigned long address) +{ + return -EOPNOTSUPP; +} + static inline struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, unsigned long address) { diff --git a/mm/memory.c b/mm/memory.c index eef4e482c0c2..c9517742bd6d 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -5913,6 +5913,68 @@ struct vm_area_struct *lock_mm_and_find_vma(struct mm_struct *mm, #endif #ifdef CONFIG_PER_VMA_LOCK +/* + * find_and_lock_vma_rcu() - Find and lock the VMA for a given address, or the + * next VMA. Search is done under RCU protection, without taking or assuming + * mmap_lock. Returned VMA is guaranteed to be stable and not isolated. + + * @mm: The mm_struct to check + * @addr: The address + * + * Returns: The VMA associated with addr, or the next VMA. + * May return %NULL in the case of no VMA at addr or above. + * If the VMA is being modified and can't be locked, -EBUSY is returned. + */ +struct vm_area_struct *find_and_lock_vma_rcu(struct mm_struct *mm, + unsigned long address) +{ + MA_STATE(mas, &mm->mm_mt, address, address); + struct vm_area_struct *vma; + int err; + + rcu_read_lock(); +retry: + vma = mas_find(&mas, ULONG_MAX); + if (!vma) { + err = 0; /* no VMA, return NULL */ + goto inval; + } + + if (!vma_start_read(vma)) { + err = -EBUSY; + goto inval; + } + + /* + * Check since vm_start/vm_end might change before we lock the VMA. + * Note, unlike lock_vma_under_rcu() we are searching for VMA covering + * address or the next one, so we only make sure VMA wasn't updated to + * end before the address. + */ + if (unlikely(vma->vm_end <= address)) { + err = -EBUSY; + goto inval_end_read; + } + + /* Check if the VMA got isolated after we found it */ + if (vma->detached) { + vma_end_read(vma); + count_vm_vma_lock_event(VMA_LOCK_MISS); + /* The area was replaced with another one */ + goto retry; + } + + rcu_read_unlock(); + return vma; + +inval_end_read: + vma_end_read(vma); +inval: + rcu_read_unlock(); + count_vm_vma_lock_event(VMA_LOCK_ABORT); + return ERR_PTR(err); +} + /* * Lookup and lock a VMA under RCU protection. Returned VMA is guaranteed to be * stable and not isolated. If the VMA is not found or is being modified the
Existing lock_vma_under_rcu() API assumes exact VMA match, so it's not a 100% equivalent of find_vma(). There are use cases that do want find_vma() semantics of finding an exact VMA or the next one. Also, it's important for such an API to let user distinguish between not being able to get per-VMA lock and not having any VMAs at or after provided address. As such, this patch adds a new find_vma()-like API, find_and_lock_vma_rcu(), which finds exact or next VMA, attempts to take per-VMA lock, and if that fails, returns ERR_PTR(-EBUSY). It still returns NULL if there is no VMA at or after address. In successfuly case it will return valid and non-isolated VMA with VMA lock taken. This API will be used in subsequent patch in this patch set to implement a new user-facing API for querying process VMAs. Cc: Mike Rapoport <rppt@kernel.org> Cc: Suren Baghdasaryan <surenb@google.com> Cc: Liam Howlett <liam.howlett@oracle.com> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- include/linux/mm.h | 8 ++++++ mm/memory.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+)