On Mon, 8 Apr 2024 21:06:44 +0900 Honggyu Kim <honggyu.kim@xxxxxx> wrote:
> On Fri, 5 Apr 2024 12:24:30 -0700 SeongJae Park <sj@xxxxxxxxxx> wrote:
> > On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim <honggyu.kim@xxxxxx> 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@xxxxxx>
> > > Signed-off-by: Hyeongtak Ji <hyeongtak.ji@xxxxxx>
> > > ---
> > > 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
[...]
