Re: [PATCH v3 3/5] mm: Add RCU-based VMA lookup helper that waits for writers

On Tue, Aug 4, 2026 at 1:47 AM Lorenzo Stoakes (ARM) [off-list ref] wrote:

On Mon, Aug 03, 2026 at 12:01:21PM -0700, Suren Baghdasaryan wrote:

quoted

On Mon, Aug 3, 2026 at 4:28 AM Lorenzo Stoakes (ARM) [off-list ref] wrote:

quoted

quoted

Would be nice to have a:

Suggested-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

Ack.

Thanks!

quoted

quoted

Here given https://lore.kernel.org/linux-mm/af4Zx0gJIWbdDeY2@lucifer/ (local) :)

quoted

quoted

quoted

diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
index e20d01e8d38f..6ff05e68e61b 100644
--- a/mm/mmap_lock.c
+++ b/mm/mmap_lock.c

@@ -338,6 +338,35 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
      return NULL;
 }

+/*

Why not a kdoc comment?

Indeed. Will change.

Thanks!

quoted

quoted

quoted

+ * Find the VMA covering 'address' and lock it for reading. Waits for writers to
+ * finish if the VMA is being modified. Returns NULL if there is no VMA covering
+ * 'address'.
+ *
+ * Use only in code paths where no mmap_lock and no VMA lock is held.

Well, a VMA read lock can be held which would make this a noop essentially.

If a VMA write lock is held you're also ok as the mmap read lock will preclude
an mmap write lock, meaning vma_end_write_all() will have been called and the
write lock released.

So I think you can just drop this line?

Maybe instead of this I should say: Use when mmap_lock is not held,
otherwise use vma_start_read_locked() ? I think that was the original
reason for this comment.

Yeah that works thanks!

quoted

quoted

quoted

+ *
+ * The fast path does not take mmap_lock.
+ */
+struct vm_area_struct *vma_start_read_unlocked(struct mm_struct *mm,
+                                            unsigned long address)
+{
+     struct vm_area_struct *vma;
+
+     /* Fast path: return stable VMA covering 'address': */
+     vma = lock_vma_under_rcu(mm, address);
+     if (vma)
+             return vma;
+
+     /* Slow path: preclude VMA writers by temporarily getting mmap read lock. */
+     mmap_read_lock(mm);
+     vma = vma_lookup(mm, address);
+     if (vma && !vma_start_read_locked(vma))

This maybe warrants an unlikely() given it can only happen if refcount
overflows? Also worth having a comment to that effect here?

vma_start_read_locked()
  vma_start_read_locked()
    vma_start_read_locked_nested()
      unlikely(!__refcount_inc_not_zero_limited_acquire())

already contains "unlikely" clause. Do we need to add it in all its users?
I can modify the comment for vma_start_read_locked() stating that
refcount overflow is very unlikely. Would that work or do you want
callers to have that too?

Ah ok not an issue then. But good to add a comment at least?

Will do. Thanks!

quoted

quoted

quoted

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index edd90892f8cc..c3a0c38a3dc3 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c

@@ -129,8 +129,10 @@ struct vm_area_struct *find_vma_and_prepare_anon(struct mm_struct *mm,
  *
  * Should be called without holding mmap_lock.
  *
- * Return: A locked vma containing @address, -ENOENT if no vma is found, or
- * -ENOMEM if anon_vma couldn't be allocated.
+ * Return: A locked vma containing @address, -ENOENT if no vma is found,
+ * -ENOMEM if anon_vma couldn't be allocated, or -EAGAIN if vma refcount
+ * overflow happened due to high number of readers and the caller should
+ * retry later.

I'm guessing you're fixing this up as part of the patch? But it feels a bit
random, I mean fine but you should mention this change in the commit message +
explain why.

I'm fixing that because Vlastimil asked about these inconsistencies in
his previous review :)
I can package these fixes as a separate patch or add a comment in the
changelog, smth like "While at it, fix the comments for related
functions". Would that work?

Comment in change log is fine thanks!

--
Cheers, Lorenzo