Re: [PATCH v4 04/12] mm/memory: Batch set uffd-wp markers during zapping

["David Hildenbrand (Arm)" <david@kernel.org>]

On 5/26/26 08:36, Dev Jain wrote:
> In preparation for the next patch, enable batch setting of uffd-wp ptes.

Avoid talking about "next patch". Phrase it as "In preparation for batching...".

> 
> The code paths passing nr > 1 to zap_install_uffd_wp_if_needed() produce
> that nr through either folio_pte_batch or swap_pte_batch, therefore
> batching is correct:
> 
> 1) all ptes belong to the same type of VMA (anonymous or non-anonymous,
>    wp-armed or non-wp-armed)
> 
> 2) all ptes being marked with uffd-wp or all being not marked (same is the
>    case with the pte_swp_uffd_wp_any check)
> 
> 3) uffd_supports_wp_marker() is independent of the function parameters
> 
> Note that we will have to use set_pte_at() in a loop instead of set_ptes()
> since the latter cannot handle present->non-present conversion for
> nr_pages > 1.

Below you have

> +	/* The current status of the pte should be "cleared" before calling */
> +	WARN_ON_ONCE(!pte_none(ptep_get(ptep)));

Could we easily extend set_ptes() to allow for non-present -> non-present with
nr > 1?

> 
> Rename the function to cond_install_uffd_wp_ptes, and convert the
> documentation to kerneldoc format.
> 
> Move the function to memory.c since this has grown too long to be kept
> in mm_inline.h, while retaining the inline hint.

Well, for functions in the header you really need "static inline", so it's not a
hint and I would just drop that comment.

However, the move while modifying a larger function is rather suboptimal for
reviewing (more like "spot the difference between these two pictures"). I'd
suggest moving it in one patch, and thenb modifying it in a second (this) patch
instead. You can perform the rename already while moving I think.


Nothing else jumped at me.

-- 
Cheers,

David