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

[Dev Jain]

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.
> 
>>
>> 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.
>