Re: [PATCH v3] sched/numa, mm: do not promote folios to nodes not set N_MEMORY

[Oscar Salvador]

On Fri, Feb 16, 2024 at 02:51:24PM +0100, Oscar Salvador wrote:
> On Fri, Feb 16, 2024 at 08:40:45PM +0900, Byungchul Park wrote:
> > From 150af2f78e19217a1d03e47e3ee5279684590fb4 Mon Sep 17 00:00:00 2001
> > From: Byungchul Park <byungchul@xxxxxx>
> > Date: Fri, 16 Feb 2024 20:18:10 +0900
> > Subject: [PATCH v3] sched/numa, mm: do not promote folios to nodes not set N_MEMORY
> 
> "do not try to promote folios to memoryless nodes"

Thinking some more, promote might be misleading, just something like
"do not try to migrate memory to memoryless nodes".

As this is not a bug fix but an optimization, as we will fail anyways
in migrate_misplaced_folio() when migrate_balanced_pgdat() notices that
we do not have any memory on that code.

With the other comments addressed:

Reviewed-by: Oscar Salvador <osalvador@xxxxxxx>

> 
> because AFAICS we are just trying.
> Even if should_numa_migrate_memory() returns true, I assume that we will
> fail somewhere down the chain e.g: migrate_pages() when we see that this
> node does not any memory, right?
> 
> > A numa node might not have its local memory but CPUs. Promoting a folio
> > to the node's local memory is nonsense. So avoid nodes not set N_MEMORY
> > from getting promoted.
> 
> If you talk about memoryless nodes everybody gets it better IMHO.
> "Memoryless nodes do not have any memory to migrate to, so stop trying it."
> 
> 
> > Signed-off-by: Byungchul Park <byungchul@xxxxxx>
> > ---
> >  kernel/sched/fair.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index d7a3c63a2171..7ed9ef3c0134 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -1828,6 +1828,13 @@ bool should_numa_migrate_memory(struct task_struct *p, struct folio *folio,
> >  	int dst_nid = cpu_to_node(dst_cpu);
> >  	int last_cpupid, this_cpupid;
> >  
> > +	/*
> > +	 * A node of dst_nid might not have its local memory. Promoting
> > +	 * a folio to the node is meaningless.
> > +	 */
> > +	if (!node_state(dst_nid, N_MEMORY))
> > +		return false;
> 
> "Cannot migrate to memoryless nodes"
> 
> seems shorter and more clear.
> 
> So, what happens when we return true here? will we fail at
> migrate_pages() I guess? That is quite down the road so I guess
> this check can save us some time.
> 
> 
> -- 
> Oscar Salvador
> SUSE Labs
> 

-- 
Oscar Salvador
SUSE Labs