Re: [PATCH v4 4/4] mm: huge_memory: use folio_needs_prot_numa() for pmd folio

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

On 2025/10/21 23:29, Lorenzo Stoakes wrote:
> On Mon, Oct 20, 2025 at 11:18:27PM +0800, Kefeng Wang wrote:
>>
>>
>> On 2025/10/20 21:23, David Hildenbrand wrote:
>>>
>>>>>            /*
>>>>>             * Avoid trapping faults against the zero page. The read-only
>>>>>             * data is likely to be read-cached on the local CPU and
>>>>> @@ -2490,19 +2490,13 @@ int change_huge_pmd(struct mmu_gather
>>>>> *tlb, struct vm_area_struct *vma,
>>>>>            if (pmd_protnone(*pmd))
>>>>>                goto unlock;
>>>>>
>>>>> -        folio = pmd_folio(*pmd);
>>>>> -        toptier = node_is_toptier(folio_nid(folio));
>>>>> -        /*
>>>>> -         * Skip scanning top tier node if normal numa
>>>>> -         * balancing is disabled
>>>>> -         */
>>>>> -        if (!(sysctl_numa_balancing_mode & NUMA_BALANCING_NORMAL) &&
>>>>> -            toptier)
>>>>> -            goto unlock;
>>>>> +        /* Get target node for single threaded private VMAs */
>>>>> +        if (!(vma->vm_flags & VM_SHARED) &&
>>>>> +            atomic_read(&vma->vm_mm->mm_users) == 1)
>>>>> +            target_node = numa_node_id();
>>>>
>>>> This is duplicated in both callers, and only used by
>>>> folio_needs_prot_numa(),
>>>> why not abstract this to the function also?
>>>
>>> There was a discussion on that in v3 I think where I asked the same
>>> question.
>>>
>>
>> Yes, it is in v1, for pte, we could avoid 512 times check and the
>> numa_node_id(), so we leave it as is.
>>
> 
> OK so what you're saying is that in change_pte_range() you only need do the
> check once (per PMD) rather than for each PTE entry.
> 
> It might be worth having this as a separate helper function as it sucks to
> duplicate that.
> 
> I actually really don't like that we pass in a 'target node' but only when it's
> single-threaded. That's a bit silly.
> 
> E.g.:
> 
> static inline bool vma_is_single_threaded_private(struct vm_area_struct *vma)
> {
> 	if (vma->vm_flags & VM_SHARED)
> 		return false;
> 
> 	return atomic_read(&vma->vm_mm->mm_users) == 1;
> }
> 
> Then you can pass in a boolean and change:
> 
> 	/*
> 	 * Don't mess with PTEs if page is already on the node
> 	 * a single-threaded process is running on.
> 	 */
> 	nid = folio_nid(folio);
> 	if (target_node == nid)
> 		return false;
> 
> To:
> 
> 	/*
> 	 * Don't mess with PTEs if page is already on the node
> 	 * a single-threaded process is running on.
> 	 */
> 	if (is_private_single_threaded && folio_nid(folio) == numa_node_id())
> 		return false;
> 
> Which makes a lot more sense than passing in NUMA_NO_NODE for shared VMAs.
> 

OK, will add the helper to remove the duplication.

> Now we're sharing this I really don't know why the function is still in
> mprotect.c by the way? Shouldn't it be in mempolicy.c + have a stub
> function for !CONFIG_NUMA_BALANCING?

Do you mean that moving folio_can_map_numa_prot() near change_prot_numa()
function in mempolicy.c?  if so, I will change it.