Re: [PATCH v2 1/4] mm: support promotion-only NUMA hinting scans

[David Hildenbrand (Arm)]

On 9/11/26 02:18, Gregory Price wrote:
> From: "Gregory Price (Meta)" <gourry@xxxxxxxxxx>
> 
> folio_can_map_prot_numa() derives the eligible memory from the global
> balancing mode. Combined mode needs the scanner to distinguish placement
> scans from promotion-only scans.
> 
> Add MM_CP_PROT_NUMA_PROMO_ONLY and let change_prot_numa() callers request
> a promotion-only protection walk. Carry the choice with each walk so the
> PTE and PMD paths use the same decision.
> 
> Initially derive the value from the normal balancing mode, preserving the
> existing behavior for the following fixes.
> 
> Fixes: c574bbe91703 ("NUMA balancing: optimize page placement for memory tiering system")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>
> ---
>  include/linux/mm.h  |  4 +++-
>  kernel/sched/fair.c |  7 ++++++-
>  mm/huge_memory.c    |  3 ++-
>  mm/internal.h       |  5 +++--
>  mm/mempolicy.c      | 22 +++++++++++++---------
>  mm/mprotect.c       |  4 +++-
>  6 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 969594074fd2..2d1b59a27629 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3408,6 +3408,8 @@ int get_cmdline(struct task_struct *task, char *buffer, int buflen);
>  #define  MM_CP_UFFD_RWP_RESOLVE            (1UL << 5) /* resolve rwp */
>  #define  MM_CP_UFFD_RWP_ALL                (MM_CP_UFFD_RWP | \
>  					    MM_CP_UFFD_RWP_RESOLVE)
> +/* Whether a MM_CP_PROT_NUMA change is for promotion only */
> +#define  MM_CP_PROT_NUMA_PROMO_ONLY        (1UL << 6)

BTW, shouldn't we just be using BIT()?

>  
>  bool can_change_pte_writable(struct vm_area_struct *vma, unsigned long addr,
>  			     pte_t pte);
> @@ -4736,7 +4738,7 @@ void vma_set_file(struct vm_area_struct *vma, struct file *file);
>  
>  #ifdef CONFIG_NUMA_BALANCING
>  unsigned long change_prot_numa(struct vm_area_struct *vma,
> -			unsigned long start, unsigned long end);
> +			unsigned long start, unsigned long end, bool promo_only);

While at it use two tabs.

>  #endif
>  
>  struct vm_area_struct *find_extend_vma_locked(struct mm_struct *,
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8dff37059faf..81359b414947 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4129,8 +4129,10 @@ static void task_numa_work(struct callback_head *work)
>  	unsigned long nr_pte_updates = 0;
>  	long pages, virtpages;
>  	struct vma_iterator vmi;
> +	unsigned int numab_mode = READ_ONCE(sysctl_numa_balancing_mode);

const and all the way to the top?

>  	bool vma_pids_skipped;
>  	bool vma_pids_forced = false;
> +	bool promo_only;
>  
>  	WARN_ON_ONCE(p != container_of(work, struct task_struct, numa_work));
>  
> @@ -4304,11 +4306,14 @@ static void task_numa_work(struct callback_head *work)
>  			continue;
>  		}
>  
> +		promo_only = !(numab_mode & NUMA_BALANCING_NORMAL);

bool promo_only = !(numab_mode & NUMA_BALANCING_NORMAL);


and in the later patch

if (vma_is_ro_file(vma))
	promo_only = true;

?

But ...

> +
>  		do {
>  			start = max(start, vma->vm_start);
>  			end = ALIGN(start + (pages << PAGE_SHIFT), HPAGE_SIZE);
>  			end = min(end, vma->vm_end);
> -			nr_pte_updates = change_prot_numa(vma, start, end);
> +			nr_pte_updates = change_prot_numa(vma, start, end,
> +							  promo_only);


Stupid question: why can't/shouldn't change_prot_numa() query
sysctl_numa_balancing_mode? Why do we have to query this outside of the function
and forward it?

I mean, change_prot_numa() gets the vma and can query
sysctl_numa_balancing_mode. Why not move that into the function and avoid the
boolean parameter?

We do have a single change_prot_numa() caller in the tree ...

>  
>  			/*
>  			 * Try to scan sysctl_numa_balancing_size worth of
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 30b7c63b0e35..2f9ada1bbcfc 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2784,7 +2784,8 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  			goto unlock;
>  
>  		if (!folio_can_map_prot_numa(pmd_folio(*pmd), vma,
> -					     vma_is_single_threaded_private(vma)))
> +					     vma_is_single_threaded_private(vma),
> +					     cp_flags & MM_CP_PROT_NUMA_PROMO_ONLY))

As raised, maybe just forward cp_flags



-- 
Cheers,

David