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

[Dev Jain <dev.jain@arm.com>]

On 23/06/26 3:31 pm, Dev Jain wrote:
> 
> 
> On 16/06/26 7:13 pm, David Hildenbrand (Arm) wrote:
>> 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?
> 
> We could have a set_nonpresent_ptes() in general, but then for some non-present
> configs like in this patch, we don't need to change the pfn, while in some
> configs like setting swp ptes in one go (like I do in later patches) we have
> an underlying pfn. So I don't see an obvious simplification with a common
> helper for non-present -> non-present for nr > 1.

I think a "non-present" entry always means a softleaf? In that case, we can
simply branch on softleaf_has_pfn().

>>
>>>
>>> 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.
> 
> Ok.
> 
>>
>> 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.
> 
> So I'll move the function, do the renaming of pte and pteval, do the kerneldoc conversion
> change in one patch. Then batch and rename in the next.
> 
>>
>>
>> Nothing else jumped at me.
>>
> 
>