Re: [PATCH v4 3/4] mm: mprotect: convert to folio_needs_prot_numa()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2025/10/20 21:10, Lorenzo Stoakes wrote:
> On Mon, Oct 20, 2025 at 02:18:44PM +0800, Kefeng Wang wrote:
>> The prot_numa_skip() naming is not good since it updates the folio
>> access time except checking whether to skip prot NUMA, so rename it
>> to folio_needs_prot_numa(), and cleanup it a bit, remove ret by
> 
> Hmm not sure 'folio_needs_prot_numa()' is any better in terms of indicating
> that you're updating the access time to be honest.
> 
> Also it seems to suggest that you're determining whether a mapping of the
> folio should be made a NUMA hint by the folio alone rather than the reality
> that the mapping is being considered for NUMA hinting and you're checking
> to see if you actually have to do it.
> 
> prot_numa_hint_needed() seems better to me?
> 
> folio_xxx() stuff all seems to be derived from the folio alone.

Naming is hard, David suggested it with folio_ prefix in v2,
the most check are derived from folio, but I could update it if
we like the new prot_numa_hint_needed().

> 
>> directly return value instead of goto style, also make it non-static
>> function so that it can be reused.
> 
> It's worth saying you plan to reuse it in change_huge_pmd() in the next
> commit, otherwise 'so it can be reused' seems like like premature not
> optimisation but abstract perhaps?

Will add change_huge_pmd() part in the message.

> 
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> 
> this functionally LGTM so With the naming, comment nits addressed:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> 
>> ---
>>   mm/internal.h |  3 +++
>>   mm/mprotect.c | 43 ++++++++++++++++++++++---------------------
>>   2 files changed, 25 insertions(+), 21 deletions(-)
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 6691d3ea55af..b521b5177d3c 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -1403,6 +1403,9 @@ int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
>>   		      unsigned long addr, int *flags, bool writable,
>>   		      int *last_cpupid);
>>
>> +bool folio_needs_prot_numa(struct folio *folio, struct vm_area_struct *vma,
>> +		int target_node);
>> +
>>   void free_zone_device_folio(struct folio *folio);
>>   int migrate_device_coherent_folio(struct folio *folio);
>>
>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>> index 6236d120c8e6..1369ba6f6294 100644
>> --- a/mm/mprotect.c
>> +++ b/mm/mprotect.c
>> @@ -118,26 +118,30 @@ static int mprotect_folio_pte_batch(struct folio *folio, pte_t *ptep,
>>   	return folio_pte_batch_flags(folio, NULL, ptep, &pte, max_nr_ptes, flags);
>>   }
>>
>> -static bool prot_numa_skip(struct vm_area_struct *vma, int target_node,
>> -		struct folio *folio)
>> +/**
>> + * folio_needs_prot_numa() - Whether the folio needs prot numa
>> + * @folio: The folio.
>> + * @vma: The VMA mapping.
>> + * @target_node: The numa node being accessed.
>> + *
>> + * Return: True if folio needs prot numa and the access time of
>> + *	   folio is adjusted. False otherwise.
> 
> Yeah this comment isn't really helpful :)
> 
> You're basically putting the function name into a longer form.
> 
> You should mention NUMA hinting, that we are checking to see if the folio
> actually indicates that we need to make the mapping one which causes a NUMA
> hinting fault, as there are cases where it's simply unnecessary.
> 
> You should also explain that you're checking a folio whose mapping is
> already being considered for being made NUMA hintable.
> 
> As right now it isn't clear that you're not somehow checking for prot numa
> based on the folio alone :)

Ignore the naming，borrowing from your suggestion,

/**
  * folio_needs_prot_numa() - check whether the folio needs prot numa
  * @folio: The folio whose mapping considered for being made NUMA hintable
  * @vma: The VMA mapping.
  * @target_node: The numa node being accessed.
  *
  * This function checks to see if the folio actually indicates that we
  * need to make the mapping one which causes a NUMA hinting fault,
  * as there are cases where it's simply unnecessary, and the folio's
  * access time is adjusted for memory tiering if prot numa needed. 

  *
  * Return: True if the mapping of the folio need to be changed, false 
otherwise.
  */

Please forgive my pool English.