* Suren Baghdasaryan <surenb@xxxxxxxxxx> [240213 13:25]: > On Tue, Feb 13, 2024 at 10:14 AM Lokesh Gidra <lokeshgidra@xxxxxxxxxx> wrote: > > > > On Tue, Feb 13, 2024 at 9:06 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote: > > > > > > * Lokesh Gidra <lokeshgidra@xxxxxxxxxx> [240213 06:25]: > > > > On Mon, Feb 12, 2024 at 7:33 PM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote: > > > > > > > > > > * Lokesh Gidra <lokeshgidra@xxxxxxxxxx> [240212 19:19]: > > > > > > All userfaultfd operations, except write-protect, opportunistically use > > > > > > per-vma locks to lock vmas. On failure, attempt again inside mmap_lock > > > > > > critical section. > > > > > > > > > > > > Write-protect operation requires mmap_lock as it iterates over multiple > > > > > > vmas. > > > > > > > > > > > > Signed-off-by: Lokesh Gidra <lokeshgidra@xxxxxxxxxx> > > > > > > --- > > > > > > fs/userfaultfd.c | 13 +- > > > > > > include/linux/userfaultfd_k.h | 5 +- > > > > > > mm/userfaultfd.c | 392 ++++++++++++++++++++++++++-------- > > > > > > 3 files changed, 312 insertions(+), 98 deletions(-) > > > > > > > > > > > ... > > > > > > I just remembered an issue with the mmap tree that exists today that you > > > needs to be accounted for in this change. > > > > > > If you hit a NULL VMA, you need to fall back to the mmap_lock() scenario > > > today. > > > > Unless I'm missing something, isn't that already handled in the patch? > > We get the VMA outside mmap_lock critical section only via > > lock_vma_under_rcu() (in lock_vma() and find_and_lock_vmas()) and in > > both cases if we get NULL in return, we retry in mmap_lock critical > > section with vma_lookup(). Wouldn't that suffice? > > I think that case is handled correctly by lock_vma(). Yeah, it looks good. I had a bit of a panic as I forgot to check that and I was thinking of a previous version. I rechecked and v5 looks good. > > Sorry for coming back a bit late. The overall patch looks quite good > but the all these #ifdef CONFIG_PER_VMA_LOCK seem unnecessary to me. > Why find_and_lock_vmas() and lock_mm_and_find_vmas() be called the > same name (find_and_lock_vmas()) and in one case it would lock only > the VMA and in the other case it takes mmap_lock? Similarly > unlock_vma() would in one case unlock the VMA and in the other drop > the mmap_lock? That would remove all these #ifdefs from the code. > Maybe this was already discussed? Yes, I don't think we should be locking the mm in lock_vma(), as it makes things hard to follow. We could use something like uffd_prepare(), uffd_complete() but I thought of those names rather late in the cycle, but I've already caused many iterations of this patch set and that clean up didn't seem as vital as simplicity and clarity of the locking code. Thanks, Liam
