Re: [PATCH v3] mm/memory: update stale locking comments for fault handlers

[David Hildenbrand (Arm)]

On 4/23/26 18:56, Aditya Sharma wrote:
> Update the comments for wp_page_copy(), do_wp_page(), do_swap_page(),
> do_anonymous_page(), __do_fault(), do_fault(), handle_pte_fault(),
> __handle_mm_fault(), and handle_mm_fault() to concisely clarify that
> they can be entered holding either the mmap_lock or the VMA lock,
> and that the lock may be released upon returning VM_FAULT_RETRY.
> 
> Additionally, make the following corrections:
> - In do_anonymous_page(), correct the outdated claim that the function
>   is entered with the PTE "mapped but not yet locked". Since
>   handle_pte_fault() unmaps the empty PTE before routing to
>   do_pte_missing(), the comment now correctly states it is entered
>   with the PTE unmapped and unlocked.
> - In __do_fault(), update the stale reference from __lock_page_retry()
>   to __folio_lock_or_retry().
> 
> Signed-off-by: Aditya Sharma <adi.sharma@xxxxxxxxxxx>
> ---

Some more nits below:

> - * We enter with non-exclusive mmap_lock (to exclude vma changes,
> - * but allow concurrent faults), with pte both mapped and locked.
> - * We return with mmap_lock still held, but pte unmapped and unlocked.
> + * We enter with either the VMA lock or the mmap_lock held (see
> + * FAULT_FLAG_VMA_LOCK) and pte both mapped and locked. We return with
> + * the same lock still held, but pte unmapped and unlocked.
>   */
>  static vm_fault_t do_wp_page(struct vm_fault *vmf)
>  	__releases(vmf->ptl)
> @@ -4696,11 +4696,11 @@ static void check_swap_exclusive(struct folio *folio, swp_entry_t entry,
>  }
>  
>  /*
> - * We enter with non-exclusive mmap_lock (to exclude vma changes,
> - * but allow concurrent faults), and pte mapped but not yet locked.
> + * We enter with either the VMA lock or the mmap_lock held (see
> + * FAULT_FLAG_VMA_LOCK), and pte mapped but not yet locked.
>   * We return with pte unmapped and unlocked.
>   *
> - * We return with the mmap_lock locked or unlocked in the same cases
> + * We return with the lock locked or unlocked in the same cases
>   * as does filemap_fault().

Maybe phrase the second part similar to the other statements further below:

"When returning, the lock may have been released in the same cases as done by
filemap_fault()".

>   */
>  vm_fault_t do_swap_page(struct vm_fault *vmf)
> @@ -5210,9 +5210,10 @@ static struct folio *alloc_anon_folio(struct vm_fault *vmf)
>  }
>  
>  /*
> - * We enter with non-exclusive mmap_lock (to exclude vma changes,
> - * but allow concurrent faults), and pte mapped but not yet locked.
> - * We return with mmap_lock still held, but pte unmapped and unlocked.
> + * We enter with either the VMA lock or the mmap_lock held (see
> + * FAULT_FLAG_VMA_LOCK), and pte unmapped and unlocked.
> + * We return with the lock still held, but pte unmapped and unlocked.
> + * If VM_FAULT_RETRY is returned, the lock may have been released.
>   */
>  static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>  {
> @@ -5330,9 +5331,10 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>  }
>  
>  /*
> - * The mmap_lock must have been held on entry, and may have been
> - * released depending on flags and vma->vm_ops->fault() return value.
> - * See filemap_fault() and __lock_page_retry().
> + * Either the VMA lock or the mmap_lock must have been held on entry,

Do we want to also add a "see .." here?

> + * and may have been released depending on flags and vma->vm_ops->fault()
> + * return value.
> + * See filemap_fault() and __folio_lock_or_retry().
>   */
>  static vm_fault_t __do_fault(struct vm_fault *vmf)
>  {
> @@ -5893,11 +5895,11 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
>  }

[...]

In general LGTM!

-- 
Cheers,

David