Re: [RFC PATCH v3 5/7] mm/damon/paddr: introduce DAMOS_MIGRATE_COLD action for demotion
Hi Honggyu, On Tue, 9 Apr 2024 18:54:14 +0900 Honggyu Kim wrote: > On Mon, 8 Apr 2024 10:52:28 -0700 SeongJae Park wrote: > > On Mon, 8 Apr 2024 21:06:44 +0900 Honggyu Kim wrote: > > > On Fri, 5 Apr 2024 12:24:30 -0700 SeongJae Park wrote: > > > > On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim > > > > wrote: [...] > > > 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? I have no strong concern or opinion about this at the moment. Please feel free to renaming it to 'taget_nids' if you think that's better. [...] > Please note that I will be out of office this week so won't be able to > answer quickly. No problem, I hope you to take and enjoy your time :) Thanks, SJ [...]
Re: [RFC PATCH v3 5/7] mm/damon/paddr: introduce DAMOS_MIGRATE_COLD action for demotion
Hi SeongJae,
On Mon, 8 Apr 2024 10:52:28 -0700 SeongJae Park wrote:
> On Mon, 8 Apr 2024 21:06:44 +0900 Honggyu Kim wrote:
>
> > On Fri, 5 Apr 2024 12:24:30 -0700 SeongJae Park wrote:
> > > On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim wrote:
> [...]
> > > > Here is one of the example usage of this 'migrate_cold' action.
> > > >
> > > > $ cd /sys/kernel/mm/damon/admin/kdamonds/
> > > > $ cat contexts//schemes//action
> > > > migrate_cold
> > > > $ echo 2 > contexts//schemes//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 0601 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
> > > > Signed-off-by: Hyeongtak Ji
> > > > ---
> > > > 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(_folios, pgdat,
> > > > target_nid);
> > > > + /* Folios that could not be migrated are still in
> > > > @migrate_folios */
> > > > + if (!list_empty(_folios)) {
> > > > + /* Folios which weren't migrated go back on @folio_list
> > > > */
> > > > + list_splice_init(_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(_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 =
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 wrote:
> On Fri, 5 Apr 2024 12:24:30 -0700 SeongJae Park wrote:
> > On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim wrote:
[...]
> > > Here is one of the example usage of this 'migrate_cold' action.
> > >
> > > $ cd /sys/kernel/mm/damon/admin/kdamonds/
> > > $ cat contexts//schemes//action
> > > migrate_cold
> > > $ echo 2 > contexts//schemes//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 0601 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
> > > Signed-off-by: Hyeongtak Ji
> > > ---
> > > 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(_folios, pgdat, target_nid);
> > > + /* Folios that could not be migrated are still in @migrate_folios */
> > > + if (!list_empty(_folios)) {
> > > + /* Folios which weren't migrated go back on @folio_list */
> > > + list_splice_init(_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(_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
[...]
Re: [RFC PATCH v3 5/7] mm/damon/paddr: introduce DAMOS_MIGRATE_COLD action for demotion
On Fri, 5 Apr 2024 12:24:30 -0700 SeongJae Park wrote:
> On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim wrote:
>
> > This patch introduces DAMOS_MIGRATE_COLD action, which is similar to
> > DAMOS_PAGEOUT, but migrate folios to the given 'target_nid' in the sysfs
> > instead of swapping them out.
> >
> > The 'target_nid' sysfs knob is created by this patch to inform the
> > migration target node ID.
>
> Isn't it created by the previous patch?
Right. I didn't fix the commit message after split this patch. I will
fix it.
> >
> > Here is one of the example usage of this 'migrate_cold' action.
> >
> > $ cd /sys/kernel/mm/damon/admin/kdamonds/
> > $ cat contexts//schemes//action
> > migrate_cold
> > $ echo 2 > contexts//schemes//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 0601 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.
> >
> > Signed-off-by: Honggyu Kim
> > Signed-off-by: Hyeongtak Ji
> > ---
> > include/linux/damon.h| 2 +
> > mm/damon/paddr.c | 146 ++-
> > mm/damon/sysfs-schemes.c | 4 ++
> > 3 files changed, 151 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 24ea33a03d5d..df8671e69a70 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -105,6 +105,7 @@ struct damon_target {
> > * @DAMOS_NOHUGEPAGE: Call ``madvise()`` for the region with
> > MADV_NOHUGEPAGE.
> > * @DAMOS_LRU_PRIO:Prioritize the region on its LRU lists.
> > * @DAMOS_LRU_DEPRIO: Deprioritize the region on its LRU lists.
> > + * @DAMOS_MIGRATE_COLD: Migrate for the given cold region.
>
> Whether it will be for cold region or not is depending on the target access
> pattern. What about 'Migrate the regions in coldest regions first manner.'?
> Or, simply 'Migrate the regions (prioritize cold)' here, and explain about the
> prioritization under quota on the detailed comments part?
"Migrate the regions in coldest regions first manner under quota" sounds
better. I will change it.
> Also, let's use tab consistently.
Yeah, it's a mistake. will fix it.
> > * @DAMOS_STAT:Do nothing but count the stat.
> > * @NR_DAMOS_ACTIONS: Total number of DAMOS actions
> > *
> > @@ -122,6 +123,7 @@ enum damos_action {
> > DAMOS_NOHUGEPAGE,
> > DAMOS_LRU_PRIO,
> > DAMOS_LRU_DEPRIO,
> > + DAMOS_MIGRATE_COLD,
> > DAMOS_STAT, /* Do nothing but only record the stat */
> > NR_DAMOS_ACTIONS,
> > };
> > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> > index 277a1c4d833c..fe217a26f788 100644
> > --- a/mm/damon/paddr.c
> > +++ b/mm/damon/paddr.c
> > @@ -12,6 +12,9 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > +#include
> >
> > #include "../internal.h"
> > #include "ops-common.h"
> > @@ -226,8 +229,137 @@ static bool damos_pa_filter_out(struct damos *scheme,
> > struct folio *folio)
> >
> > enum migration_mode {
> > MIG_PAGEOUT,
> > + MIG_MIGRATE_COLD,
> > };
> >
> > +static unsigned int migrate_folio_list(struct list_head *migrate_folios,
> > + struct pglist_data *pgdat,
> > + int target_nid)
>
> To avoid name collisions, I'd prefer having damon_pa_prefix. I show this
> patch
> is defining damon_pa_migrate_folio_list() below, though. What about
> __damon_pa_migrate_folio_list()?
Ack. I will change it to __damon_pa_migrate_folio_list().
> > +{
> > + 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
Re: [RFC PATCH v3 5/7] mm/damon/paddr: introduce DAMOS_MIGRATE_COLD action for demotion
On Fri, 5 Apr 2024 16:55:57 +0900 Hyeongtak Ji wrote:
> On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim wrote:
>
> ...snip...
>
> > +static unsigned long damon_pa_migrate_pages(struct list_head *folio_list,
> > + enum migration_mode mm,
> > + int target_nid)
> > +{
> > + int nid;
> > + unsigned int nr_migrated = 0;
> > + LIST_HEAD(node_folio_list);
> > + unsigned int noreclaim_flag;
> > +
> > + if (list_empty(folio_list))
> > + return nr_migrated;
>
> How about checking if `target_nid` is `NUMA_NO_NODE` or not earlier,
>
> > +
> > + noreclaim_flag = memalloc_noreclaim_save();
> > +
> > + 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);
> > + list_move(>lru, _folio_list);
> > + continue;
> > + }
> > +
> > + nr_migrated += damon_pa_migrate_folio_list(_folio_list,
> > + NODE_DATA(nid), mm,
> > + target_nid);
> > + nid = folio_nid(lru_to_folio(folio_list));
> > + } while (!list_empty(folio_list));
> > +
> > + nr_migrated += damon_pa_migrate_folio_list(_folio_list,
> > + NODE_DATA(nid), mm,
> > + target_nid);
> > +
> > + memalloc_noreclaim_restore(noreclaim_flag);
> > +
> > + return nr_migrated;
> > +}
> > +
>
> ...snip...
>
> > +static unsigned int migrate_folio_list(struct list_head *migrate_folios,
> > + struct pglist_data *pgdat,
> > + int target_nid)
> > +{
> > + unsigned int nr_succeeded;
> > + nodemask_t allowed_mask = NODE_MASK_NONE;
> > +
> > + struct migration_target_control mtc = {
> > + /*
> > +* Allocate from 'node', or fail quickly and quietly.
> > +* When this happens, 'page' will likely just be discarded
> > +* instead of migrated.
> > +*/
> > + .gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) |
> > __GFP_NOWARN |
> > + __GFP_NOMEMALLOC | GFP_NOWAIT,
> > + .nid = target_nid,
> > + .nmask = _mask
> > + };
> > +
> > + if (pgdat->node_id == target_nid || target_nid == NUMA_NO_NODE)
> > + return 0;
>
> instead of here.
Agree. As I replied on the previous reply, I think this check can be done from
the caller (or the caller of the caller) of this function.
>
> > +
> > + if (list_empty(migrate_folios))
> > + return 0;
Same for this.
> > +
> > + /* Migration ignores all cpuset and mempolicy settings */
> > + migrate_pages(migrate_folios, alloc_migrate_folio, NULL,
> > + (unsigned long), MIGRATE_ASYNC, MR_DAMON,
> > + _succeeded);
> > +
> > + return nr_succeeded;
> > +}
> > +
>
> ...snip...
>
> Kind regards,
> Hyeongtak
>
Thanks,
SJ
Re: [RFC PATCH v3 5/7] mm/damon/paddr: introduce DAMOS_MIGRATE_COLD action for demotion
On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim wrote:
> This patch introduces DAMOS_MIGRATE_COLD action, which is similar to
> DAMOS_PAGEOUT, but migrate folios to the given 'target_nid' in the sysfs
> instead of swapping them out.
>
> The 'target_nid' sysfs knob is created by this patch to inform the
> migration target node ID.
Isn't it created by the previous patch?
>
> Here is one of the example usage of this 'migrate_cold' action.
>
> $ cd /sys/kernel/mm/damon/admin/kdamonds/
> $ cat contexts//schemes//action
> migrate_cold
> $ echo 2 > contexts//schemes//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 0601 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.
>
> Signed-off-by: Honggyu Kim
> Signed-off-by: Hyeongtak Ji
> ---
> include/linux/damon.h| 2 +
> mm/damon/paddr.c | 146 ++-
> mm/damon/sysfs-schemes.c | 4 ++
> 3 files changed, 151 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 24ea33a03d5d..df8671e69a70 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -105,6 +105,7 @@ struct damon_target {
> * @DAMOS_NOHUGEPAGE:Call ``madvise()`` for the region with
> MADV_NOHUGEPAGE.
> * @DAMOS_LRU_PRIO: Prioritize the region on its LRU lists.
> * @DAMOS_LRU_DEPRIO:Deprioritize the region on its LRU lists.
> + * @DAMOS_MIGRATE_COLD: Migrate for the given cold region.
Whether it will be for cold region or not is depending on the target access
pattern. What about 'Migrate the regions in coldest regions first manner.'?
Or, simply 'Migrate the regions (prioritize cold)' here, and explain about the
prioritization under quota on the detailed comments part?
Also, let's use tab consistently.
> * @DAMOS_STAT: Do nothing but count the stat.
> * @NR_DAMOS_ACTIONS:Total number of DAMOS actions
> *
> @@ -122,6 +123,7 @@ enum damos_action {
> DAMOS_NOHUGEPAGE,
> DAMOS_LRU_PRIO,
> DAMOS_LRU_DEPRIO,
> + DAMOS_MIGRATE_COLD,
> DAMOS_STAT, /* Do nothing but only record the stat */
> NR_DAMOS_ACTIONS,
> };
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 277a1c4d833c..fe217a26f788 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -12,6 +12,9 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> #include "../internal.h"
> #include "ops-common.h"
> @@ -226,8 +229,137 @@ static bool damos_pa_filter_out(struct damos *scheme,
> struct folio *folio)
>
> enum migration_mode {
> MIG_PAGEOUT,
> + MIG_MIGRATE_COLD,
> };
>
> +static unsigned int migrate_folio_list(struct list_head *migrate_folios,
> +struct pglist_data *pgdat,
> +int target_nid)
To avoid name collisions, I'd prefer having damon_pa_prefix. I show this patch
is defining damon_pa_migrate_folio_list() below, though. What about
__damon_pa_migrate_folio_list()?
> +{
> + 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?
> + struct migration_target_control mtc = {
> + /*
> + * Allocate from 'node', or fail quickly and quietly.
> + * When this happens, 'page' will likely just be discarded
> + * instead of migrated.
> + */
> + .gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) |
> __GFP_NOWARN |
> + __GFP_NOMEMALLOC | GFP_NOWAIT,
> + .nid = target_nid,
> + .nmask = _mask
> + };
> +
> + if (pgdat->node_id == target_nid || target_nid == NUMA_NO_NODE)
> + return 0;
> +
> + if (list_empty(migrate_folios))
> + return 0;
Can't these checks be done by the caller?
> +
> + /* Migration ignores all cpuset and mempolicy settings */
> + migrate_pages(migrate_folios, alloc_migrate_folio, NULL,
> + (unsigned long), MIGRATE_ASYNC, MR_DAMON,
> + _succeeded);
> +
> + return nr_succeeded;
> +}
> +
> +static unsigned int damon_pa_migrate_folio_list(struct list_head *folio_list,
> + struct pglist_data *pgdat,
> +
Re: [RFC PATCH v3 5/7] mm/damon/paddr: introduce DAMOS_MIGRATE_COLD action for demotion
On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim wrote:
...snip...
> +static unsigned long damon_pa_migrate_pages(struct list_head *folio_list,
> + enum migration_mode mm,
> + int target_nid)
> +{
> + int nid;
> + unsigned int nr_migrated = 0;
> + LIST_HEAD(node_folio_list);
> + unsigned int noreclaim_flag;
> +
> + if (list_empty(folio_list))
> + return nr_migrated;
How about checking if `target_nid` is `NUMA_NO_NODE` or not earlier,
> +
> + noreclaim_flag = memalloc_noreclaim_save();
> +
> + 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);
> + list_move(>lru, _folio_list);
> + continue;
> + }
> +
> + nr_migrated += damon_pa_migrate_folio_list(_folio_list,
> +NODE_DATA(nid), mm,
> +target_nid);
> + nid = folio_nid(lru_to_folio(folio_list));
> + } while (!list_empty(folio_list));
> +
> + nr_migrated += damon_pa_migrate_folio_list(_folio_list,
> +NODE_DATA(nid), mm,
> +target_nid);
> +
> + memalloc_noreclaim_restore(noreclaim_flag);
> +
> + return nr_migrated;
> +}
> +
...snip...
> +static unsigned int migrate_folio_list(struct list_head *migrate_folios,
> +struct pglist_data *pgdat,
> +int target_nid)
> +{
> + unsigned int nr_succeeded;
> + nodemask_t allowed_mask = NODE_MASK_NONE;
> +
> + struct migration_target_control mtc = {
> + /*
> + * Allocate from 'node', or fail quickly and quietly.
> + * When this happens, 'page' will likely just be discarded
> + * instead of migrated.
> + */
> + .gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) |
> __GFP_NOWARN |
> + __GFP_NOMEMALLOC | GFP_NOWAIT,
> + .nid = target_nid,
> + .nmask = _mask
> + };
> +
> + if (pgdat->node_id == target_nid || target_nid == NUMA_NO_NODE)
> + return 0;
instead of here.
> +
> + if (list_empty(migrate_folios))
> + return 0;
> +
> + /* Migration ignores all cpuset and mempolicy settings */
> + migrate_pages(migrate_folios, alloc_migrate_folio, NULL,
> + (unsigned long), MIGRATE_ASYNC, MR_DAMON,
> + _succeeded);
> +
> + return nr_succeeded;
> +}
> +
...snip...
Kind regards,
Hyeongtak
