Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Apr 2022 16:10:24 +0530
From:   Jagdish Gediya <jvgediya@linux.ibm.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, aneesh.kumar@linux.ibm.com,
        baolin.wang@linux.alibaba.com, dave.hansen@linux.intel.com,
        ying.huang@intel.com
Subject: Re: [PATCH v2 1/5] mm: demotion: Set demotion list differently
Message-ID: <Ylf6GI1J5cIXagyl@li-6e1fa1cc-351b-11b2-a85c-b897023bb5f3.ibm.com>
References: <20220413092206.73974-1-jvgediya@linux.ibm.com>
 <20220413092206.73974-2-jvgediya@linux.ibm.com>
 <20220414100214.00005ad8@Huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220414100214.00005ad8@Huawei.com>
Precedence: bulk

On Thu, Apr 14, 2022 at 10:02:14AM +0100, Jonathan Cameron wrote:
> On Wed, 13 Apr 2022 14:52:02 +0530
> Jagdish Gediya <jvgediya@linux.ibm.com> wrote:
> 
> > Sharing used_targets between multiple nodes in a single
> > pass limits some of the opportunities for demotion target
> > sharing.
> > 
> > Don't share the used targets between multiple nodes in a
> > single pass, instead accumulate all the used targets in
> > source nodes shared by all pass, and reset 'used_targets'
> > to source nodes while finding demotion targets for any new
> > node.
> > 
> > This results into some more opportunities to share demotion
> > targets between multiple source nodes, e.g. with below NUMA
> > topology, where node 0 & 1 are cpu + dram nodes, node 2 & 3
> > are equally slower memory only nodes, and node 4 is slowest
> > memory only node,
> > 
> > available: 5 nodes (0-4)
> > node 0 cpus: 0 1
> > node 0 size: n MB
> > node 0 free: n MB
> > node 1 cpus: 2 3
> > node 1 size: n MB
> > node 1 free: n MB
> > node 2 cpus:
> > node 2 size: n MB
> > node 2 free: n MB
> > node 3 cpus:
> > node 3 size: n MB
> > node 3 free: n MB
> > node 4 cpus:
> > node 4 size: n MB
> > node 4 free: n MB
> > node distances:
> > node   0   1   2   3   4
> >   0:  10  20  40  40  80
> >   1:  20  10  40  40  80
> >   2:  40  40  10  40  80
> >   3:  40  40  40  10  80
> >   4:  80  80  80  80  10
> > 
> > The existing implementation gives below demotion targets,
> > 
> > node    demotion_target
> >  0              3, 2
> >  1              4
> >  2              X
> >  3              X
> >  4              X
> > 
> > With this patch applied, below are the demotion targets,
> > 
> > node    demotion_target
> >  0              3, 2
> >  1              3, 2
> 
> Is there an easy way to make the allocation stateful enough so
> that when it sees two identical choices, it alternates between
> them?  Whilst it's going to be workload dependent, my view
> of 'ideal' for this would be.
> 
>    0              3
>    1              2
> 
> Maybe we'll just have to make do with most systems this effects
> having to have some fun userspace code that does cleverer
> balancing - possibly using HMAT info rather than just SLIT
> to give us visibility of interconnect bottlenecks that make
> some migration paths 'unwise'.
>   
> I'm not sure the current HMAT presentation via sysfs gives
> us enough info though so we'll probably need to extend that.
> 
> Jonathan

This patch series also have the support to override the default
demotion targets found by the kernel, however current implementation
of this user space interface doesn't support to set per node demotion
targets, but I am going to modify the user space inetrface according
to Huang's suggestion which can control exact desired targets for
specific nodes.

> >  2              4
> >  3              4
> >  4              X
> > 
> > e.g. with below NUMA topology, where node 0, 1 & 2 are
> > cpu + dram nodes and node 3 is slow memory node,
> > 
> > available: 4 nodes (0-3)
> > node 0 cpus: 0 1
> > node 0 size: n MB
> > node 0 free: n MB
> > node 1 cpus: 2 3
> > node 1 size: n MB
> > node 1 free: n MB
> > node 2 cpus: 4 5
> > node 2 size: n MB
> > node 2 free: n MB
> > node 3 cpus:
> > node 3 size: n MB
> > node 3 free: n MB
> > node distances:
> > node   0   1   2   3
> >   0:  10  20  20  40
> >   1:  20  10  20  40
> >   2:  20  20  10  40
> >   3:  40  40  40  10
> > 
> > The existing implementation gives below demotion targets,
> > 
> > node    demotion_target
> >  0              3
> >  1              X
> >  2              X
> >  3              X
> > 
> > With this patch applied, below are the demotion targets,
> > 
> > node    demotion_target
> >  0              3
> >  1              3
> >  2              3
> >  3              X
> > 
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > ---
> >  mm/migrate.c | 25 ++++++++++++++-----------
> >  1 file changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index de175e2fdba5..516f4e1348c1 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -2383,7 +2383,7 @@ static void __set_migration_target_nodes(void)
> >  {
> >  	nodemask_t next_pass	= NODE_MASK_NONE;
> >  	nodemask_t this_pass	= NODE_MASK_NONE;
> > -	nodemask_t used_targets = NODE_MASK_NONE;
> > +	nodemask_t source_nodes = NODE_MASK_NONE;
> >  	int node, best_distance;
> >  
> >  	/*
> > @@ -2401,20 +2401,23 @@ static void __set_migration_target_nodes(void)
> >  again:
> >  	this_pass = next_pass;
> >  	next_pass = NODE_MASK_NONE;
> > +
> >  	/*
> > -	 * To avoid cycles in the migration "graph", ensure
> > -	 * that migration sources are not future targets by
> > -	 * setting them in 'used_targets'.  Do this only
> > -	 * once per pass so that multiple source nodes can
> > -	 * share a target node.
> > -	 *
> > -	 * 'used_targets' will become unavailable in future
> > -	 * passes.  This limits some opportunities for
> > -	 * multiple source nodes to share a destination.
> > +	 * Accumulate source nodes to avoid the cycle in migration
> > +	 * list.
> >  	 */
> > -	nodes_or(used_targets, used_targets, this_pass);
> > +	nodes_or(source_nodes, source_nodes, this_pass);
> >  
> >  	for_each_node_mask(node, this_pass) {
> > +		/*
> > +		 * To avoid cycles in the migration "graph", ensure
> > +		 * that migration sources are not future targets by
> > +		 * setting them in 'used_targets'. Reset used_targets
> > +		 * to source nodes for each node in this pass so that
> > +		 * multiple source nodes can share a target node.
> > +		 */
> > +		nodemask_t used_targets = source_nodes;
> > +
> >  		best_distance = -1;
> >  
> >  		/*
>