Date: Mon,  8 Apr 2024 10:52:28 -0700
From: SeongJae Park <sj@...nel.org>
To: Honggyu Kim <honggyu.kim@...com>
Cc: SeongJae Park <sj@...nel.org>,
	damon@...ts.linux.dev,
	linux-mm@...ck.org,
	akpm@...ux-foundation.org,
	apopple@...dia.com,
	baolin.wang@...ux.alibaba.com,
	dave.jiang@...el.com,
	hyeongtak.ji@...com,
	kernel_team@...ynix.com,
	linmiaohe@...wei.com,
	linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org,
	mathieu.desnoyers@...icios.com,
	mhiramat@...nel.org,
	rakie.kim@...com,
	rostedt@...dmis.org,
	surenb@...gle.com,
	yangx.jy@...itsu.com,
	ying.huang@...el.com,
	ziy@...dia.com,
	42.hyeyoo@...il.com,
	art.jeongseob@...il.com
Subject: Re: [RFC PATCH v3 5/7] mm/damon/paddr: introduce DAMOS_MIGRATE_COLD action for demotion

On Mon,  8 Apr 2024 21:06:44 +0900 Honggyu Kim <honggyu.kim@...com> wrote:

> On Fri,  5 Apr 2024 12:24:30 -0700 SeongJae Park <sj@...nel.org> wrote:
> > On Fri,  5 Apr 2024 15:08:54 +0900 Honggyu Kim <honggyu.kim@...com> wrote:
[...]
> > > Here is one of the example usage of this 'migrate_cold' action.
> > > 
> > >   $ cd /sys/kernel/mm/damon/admin/kdamonds/<N>
> > >   $ cat contexts/<N>/schemes/<N>/action
> > >   migrate_cold
> > >   $ echo 2 > contexts/<N>/schemes/<N>/target_nid
> > >   $ echo commit > state
> > >   $ numactl -p 0 ./hot_cold 500M 600M &
> > >   $ numastat -c -p hot_cold
> > > 
> > >   Per-node process memory usage (in MBs)
> > >   PID             Node 0 Node 1 Node 2 Total
> > >   --------------  ------ ------ ------ -----
> > >   701 (hot_cold)     501      0    601  1101
> > > 
> > > Since there are some common routines with pageout, many functions have
> > > similar logics between pageout and migrate cold.
> > > 
> > > damon_pa_migrate_folio_list() is a minimized version of
> > > shrink_folio_list(), but it's minified only for demotion.
> > 
> > MIGRATE_COLD is not only for demotion, right?  I think the last two words are
> > better to be removed for reducing unnecessary confuses.
> 
> You mean the last two sentences?  I will remove them if you feel it's
> confusing.

Yes.  My real intended suggestion was 's/only for demotion/only for
migration/', but entirely removing the sentences is also ok for me.

> 
> > > 
> > > Signed-off-by: Honggyu Kim <honggyu.kim@...com>
> > > Signed-off-by: Hyeongtak Ji <hyeongtak.ji@...com>
> > > ---
> > >  include/linux/damon.h    |   2 +
> > >  mm/damon/paddr.c         | 146 ++++++++++++++++++++++++++++++++++++++-
> > >  mm/damon/sysfs-schemes.c |   4 ++
> > >  3 files changed, 151 insertions(+), 1 deletion(-)
[...]
> > > --- a/mm/damon/paddr.c
> > > +++ b/mm/damon/paddr.c
[...]
> > > +{
> > > +	unsigned int nr_succeeded;
> > > +	nodemask_t allowed_mask = NODE_MASK_NONE;
> > > +
> > 
> > I personally prefer not having empty lines in the middle of variable
> > declarations/definitions.  Could we remove this empty line?
> 
> I can remove it, but I would like to have more discussion about this
> issue.  The current implementation allows only a single migration
> target with "target_nid", but users might want to provide fall back
> migration target nids.
> 
> For example, if more than two CXL nodes exist in the system, users might
> want to migrate cold pages to any CXL nodes.  In such cases, we might
> have to make "target_nid" accept comma separated node IDs.  nodemask can
> be better but we should provide a way to change the scanning order.
> 
> I would like to hear how you think about this.

Good point.  I think we could later extend the sysfs file to receive the
comma-separated numbers, or even mask.  For simplicity, adding sysfs files
dedicated for the different format of inputs could also be an option (e.g.,
target_nids_list, target_nids_mask).  But starting from this single node as is
now looks ok to me.

[...]
> > > +	/* 'folio_list' is always empty here */
> > > +
> > > +	/* Migrate folios selected for migration */
> > > +	nr_migrated += migrate_folio_list(&migrate_folios, pgdat, target_nid);
> > > +	/* Folios that could not be migrated are still in @migrate_folios */
> > > +	if (!list_empty(&migrate_folios)) {
> > > +		/* Folios which weren't migrated go back on @folio_list */
> > > +		list_splice_init(&migrate_folios, folio_list);
> > > +	}
> > 
> > Let's not use braces for single statement
> > (https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces).
> 
> Hmm.. I know the convention but left it as is because of the comment.
> If I remove the braces, it would have a weird alignment for the two
> lines for comment and statement lines.

I don't really hate such alignment.  But if you don't like it, how about moving
the comment out of the if statement?  Having one comment for one-line if
statement looks not bad to me.

> 
> > > +
> > > +	try_to_unmap_flush();
> > > +
> > > +	list_splice(&ret_folios, folio_list);
> > 
> > Can't we move remaining folios in migrate_folios to ret_folios at once?
> 
> I will see if it's possible.

Thank you.  Not a strict request, though.

[...]
> > > +	nid = folio_nid(lru_to_folio(folio_list));
> > > +	do {
> > > +		struct folio *folio = lru_to_folio(folio_list);
> > > +
> > > +		if (nid == folio_nid(folio)) {
> > > +			folio_clear_active(folio);
> > 
> > I think this was necessary for demotion, but now this should be removed since
> > this function is no more for demotion but for migrating random pages, right?
> 
> Yeah,  it can be removed because we do migration instead of demotion,
> but I need to make sure if it doesn't change the performance evaluation
> results.

Yes, please ensure the test results are valid :)


Thanks,
SJ

[...]

Powered by blists - more mailing lists