On Wed, Jun 5, 2024 at 10:03 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
>
> * Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [240605 12:27]:
> > On Wed, Jun 5, 2024 at 9:24 AM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Wed, Jun 5, 2024 at 9:13 AM Andrii Nakryiko
> > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >
> > > > On Wed, Jun 5, 2024 at 6:33 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> > > > >
> > > > > * Matthew Wilcox <willy@xxxxxxxxxxxxx> [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
