Re: [PATCH v3 1/9] mm: add find_vma()-like API but RCU protected and taking VMA lock

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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;
> > > > +   }
> > >