Hi SeongJae,
On Mon, 8 Apr 2024 10:52:28 -0700 SeongJae Park <s...@kernel.org> wrote:
> On Mon, 8 Apr 2024 21:06:44 +0900 Honggyu Kim <honggyu....@sk.com> wrote:
>
> > On Fri, 5 Apr 2024 12:24:30 -0700 SeongJae Park <s...@kernel.org> wrote:
> > > On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim <honggyu....@sk.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.
Ack.
> >
> > > >
> > > > Signed-off-by: Honggyu Kim <honggyu....@sk.com>
> > > > Signed-off-by: Hyeongtak Ji <hyeongtak...@sk.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.
If you think we can start from a single node, then I will keep it as is.
But are you okay if I change the same 'target_nid' to accept
comma-separated numbers later? Or do you want to introduce another knob
such as 'target_nids_list'? What about rename 'target_nid' to
'target_nids' at the first place?
> [...]
> > > > + /* '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.
Ack. I will manage this in the next revision.
> >
> > > > +
> > > > + 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 :)
Sure. Thanks for your review in details!
Please note that I will be out of office this week so won't be able to
answer quickly.
Thanks,
Honggyu
>
> Thanks,
> SJ
>
> [...]
>
