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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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.

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?

Thanks