Received-SPF: pass (google.com: domain of linux-kernel+bounces-202912-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240605002459.4091285-1-andrii@kernel.org> <20240605002459.4091285-2-andrii@kernel.org>
 <Zl-38XrUw9entlFR@casper.infradead.org> <uevtozlryyqw5vj2duuzowupknfynmreruiw6m7bcxryjppqpm@7g766emooxfh>
 <CAEf4BzZFpidjJzRMWboZYY03U8M22Yo1sqXconi36V11XA-ZfA@mail.gmail.com>
In-Reply-To: <CAEf4BzZFpidjJzRMWboZYY03U8M22Yo1sqXconi36V11XA-ZfA@mail.gmail.com>
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date: Wed, 5 Jun 2024 09:24:07 -0700
Message-ID: <CAEf4BzYDhtkYt=qn2YgrnRkZ0tpa3EPAiCUcBkdUa-9DKN22dQ@mail.gmail.com>
Subject: Re: [PATCH v3 1/9] mm: add find_vma()-like API but RCU protected and
 taking VMA lock
To: "Liam R. Howlett" <Liam.Howlett@oracle.com>, Matthew Wilcox <willy@infradead.org>, 
	Andrii Nakryiko <andrii@kernel.org>, linux-fsdevel@vger.kernel.org, brauner@kernel.org, 
	viro@zeniv.linux.org.uk, akpm@linux-foundation.org, 
	linux-kernel@vger.kernel.org, bpf@vger.kernel.org, gregkh@linuxfoundation.org, 
	linux-mm@kvack.org, surenb@google.com, rppt@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 5, 2024 at 9:13=E2=80=AFAM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jun 5, 2024 at 6:33=E2=80=AFAM Liam R. Howlett <Liam.Howlett@orac=
le.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 add=
ress, or the
> > > > + * next VMA. Search is done under RCU protection, without taking o=
r assuming
> > > > + * mmap_lock. Returned VMA is guaranteed to be stable and not isol=
ated.
> > >
> > > 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 ret=
urned.
> > > > + */
> > > > +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 =3D mas_find(&mas, ULONG_MAX);
> > > > +   if (!vma) {
> > > > +           err =3D 0; /* no VMA, return NULL */
> > > > +           goto inval;
> > > > +   }
> > > > +
> > > > +   if (!vma_start_read(vma)) {
> > > > +           err =3D -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 c=
overing
> > > > +    * address or the next one, so we only make sure VMA wasn't upd=
ated to
> > > > +    * end before the address.
> > > > +    */
> > > > +   if (unlikely(vma->vm_end <=3D address)) {
> > > > +           err =3D -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 <=3D
> 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;
> > > > +   }
> > >