Re: [PATCH] mm: migrate: set demotion targets differently

From: Jagdish Gediya <jvgediya@linux.ibm.com>
To: Dave Hansen <dave.hansen@intel.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] mm: migrate: set demotion targets differently
Date: Tue, 29 Mar 2022 22:16:54 +0530	[thread overview]
Message-ID: <YkM3/vKRoy8/fnvD@li-6e1fa1cc-351b-11b2-a85c-b897023bb5f3.ibm.com> (raw)
In-Reply-To: <55161160-1084-c81d-d116-00f5bcaa1268@intel.com>

On Tue, Mar 29, 2022 at 07:31:12AM -0700, Dave Hansen wrote:
> On 3/29/22 04:52, Jagdish Gediya wrote:
> > The current implementation to identify the demotion
> > targets limits some of the opportunities to share
> > the demotion targets between multiple source nodes.
> 
> This changelog is a bit unsatisfying.  It basically says: the current
> code isn't working, throw some more code at the problem.

The current code works but it doesn't utilize all the possibilities
for the demotion. One of the comment in the current code says that
'used_targets will become unavailable in future passes. This limits
some opportunities for multiple source nodes to share a destination.'
This patch removes those limitation. The current limitation is single
used_target nodemask sharing between all the nodes, This patch prepares
the used_targets from scratch for each individual node and then tries
to avoid the looping based on what demotion targets are not possible
for that particular node based on the data we accumulate in newly
defined src_nodes array.

For example, with below numa topology where node 0,1 & 2 are cpu + dram
node and node 3 is slow memory node,
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

once node 3 is utilized for node 0, it can not be shared for node 1 & 2
because in current code, used_targets will have that set even when we
find demotion targets for  1 & 2.

> I'd love to see some more information about *why* the current code
> doesn't work.  Is it purely a bug or was it mis-designed?

I think the design seems to be intended because as per the comment
in the current code, it was known that there are limits to the node
sharing as a demotion target.

> I actually wrote it intending for it to handle cases like you describe
> while not leaving lots of nodes without demotion targets.

if you see the examples I have given in the commit message, the current
code misses many opportunities for the demotion, so current
implementation to avoid the looping is not the best as I have explained
above, that is where this patch is required.