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

[David Hildenbrand (Arm)]

On 4/5/26 19:18, 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>

Next time, please don't send as reply to your previous version

> ---
> v2:
> - Simplified the comment to concisely state "either the VMA lock or 
>   the mmap_lock" instead of a verbose explanation (per David Hildenbrand).
> - Expanded the scope to cover 8 other fault handlers in mm/memory.c 
>   that suffered from the same stale mmap_lock comments.
> - Fixed an additional historical inaccuracy in do_anonymous_page() 
>   regarding the PTE mapping state on entry.
> - Updated a stale reference in __do_fault() from __lock_page_retry() 
>   to __folio_lock_or_retry().
> 
> 
>  mm/memory.c | 49 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index c65e82c86..2b407e3f9 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3742,8 +3742,8 @@ vm_fault_t __vmf_anon_prepare(struct vm_fault *vmf)
>   * Handle the case of a page which we actually need to copy to a new page,
>   * either due to COW or unsharing.
>   *
> - * Called with mmap_lock locked and the old page referenced, but
> - * without the ptl held.
> + * Called with either the VMA lock or the mmap_lock (FAULT_FLAG_VMA_LOCK
> + * tells you which) 

"Called with either the VMA lock or the mmap_lock *held*"

Same for other cases.

Can we shorten "FAULT_FLAG_VMA_LOCK tells you which" to "see FAULT_FLAG_VMA_LOCK"

?

-- 
Cheers,

David