Re: [RFC PATCH v2 2/2] mm/damon/paddr: Allow multiple migrate targets

[SeongJae Park <sj@kernel.org>]

Hi Bijan,

On Fri, 20 Jun 2025 13:04:58 -0500 Bijan Tabatabai <bijan311@gmail.com> wrote:

> From: Bijan Tabatabai <bijantabatab@micron.com>
> 
> The migrate_{hot,cold} DAMONS actions take a parameter, target_nid, to
> indicate what node the actions should migrate pages to. In this patch,
> we allow passing in a list of migration targets into target_nid. When
> this is done, the mirgate_{hot, cold} actions will migrate pages between
> the specified nodes using the global interleave weights found at
> /sys/kernel/mm/mempolicy/weighted_interleave/node<N>. This functionality
> can be used to dynamically adjust how pages are interleaved in response
> to changes in bandwidth utilization to improve performance, as discussed
> in [1]. When only a single migration target is passed to target_nid, the
> migrate_{hot,cold} actions will act the same as before.
[...]
>  include/linux/damon.h    |   8 +--
>  mm/damon/core.c          |   9 ++--
>  mm/damon/lru_sort.c      |   2 +-
>  mm/damon/paddr.c         | 108 +++++++++++++++++++++++++++++++++++++--
>  mm/damon/reclaim.c       |   2 +-
>  mm/damon/sysfs-schemes.c |  14 +++--
>  samples/damon/mtier.c    |   6 ++-
>  samples/damon/prcl.c     |   2 +-
>  8 files changed, 131 insertions(+), 20 deletions(-)

If we keep pursuing making DAMON users be able to specify multiple migration
destination nodes and their weights[1], I think we may need only paddr.c part
change of this patch in the final version of this great work.

[...]
>  static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
>  		unsigned long *sz_filter_passed)
>  {
>  	unsigned long addr, applied;
> -	LIST_HEAD(folio_list);
> +	struct rmap_walk_control rwc;
[...]
>  
>  	addr = r->ar.start;
>  	while (addr < r->ar.end) {
> @@ -522,15 +599,38 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
>  		else
>  			*sz_filter_passed += folio_size(folio);
>  
> +		/*
> +		 * If there is only one target node, migrate there. Otherwise,
> +		 * interleave across the nodes according to the global
> +		 * interleave weights
> +		 */
> +		if (nr_nodes == 1) {
> +			target_nid = first_node(s->target_nids);
> +		} else {
> +			target_nid = NUMA_NO_NODE;
> +			/* Updates target_nid */
> +			rmap_walk(folio, &rwc);
> +		}

So we are doing rmap_walk(), which is known to be not very fast, for getting
the target node id of this page, in a way very similar to that of weighted
interleaving, right?  I don't think we really need to behave that same to
weighted interleaving with the cost.

I'd hence suggest to implement and use a simple weights handling mechanism
here.  It could be roud-robin way, like weighted interleaving, or probabilistic
way, using damon_rand().

The round-robin way may be simpler in my opinion.  For example,

unsigned int damos_pa_nid_to_migrate(struct damos_migrate_dest *dest)
{
	static unsigned int nr_migrated = 0;
	unsigned int total_weight = 0;
	unsigned int weights_to_ignore;
	size_t i;

	for (i = 0; i < dest->nr_dests; i++)
		total_weight += dest->weight_arr[i];
	weights_to_ignore = nr_migrate++ % total_weight;
	total_weight = 0;
	for (i = 0; i < dest->nr_dests; i++) {
		total_weight += dest->weight_arr[i];
		if (total_weight >= weights_to_ignore)
			return dest->node_id_arr[i];
	}
	WARN_ON_ONCE(1, "I don't know what I did wrong");
	return 0;
}

Then, we could replace the above rmap_walk() call with this one.  What do you
think?

Nothing stands out from other parts to me.  I will do more thorough read on the
next revision, though.

[1] https://lore.kernel.org/20250621173639.35906-1-sj@kernel.org


Thanks,
SJ

[...]