On Fri, 5 Apr 2024 15:08:51 +0900 Honggyu Kim <honggyu.kim@xxxxxx> wrote:
> The alloc_demote_folio can be used out of vmscan.c so it'd be better to
> remove static keyword from it.
>
> This function can also be used for both demotion and promotion so it'd
> be better to rename it from alloc_demote_folio to alloc_migrate_folio.
I'm not sure if renaming is really needed, but has no strong opinion.
>
> Signed-off-by: Honggyu Kim <honggyu.kim@xxxxxx>
I have one more trivial comment below, but finds no blocker for me.
Reviewed-by: SeongJae Park <sj@xxxxxxxxxx>
> ---
> mm/internal.h | 1 +
> mm/vmscan.c | 10 +++++++---
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index f309a010d50f..c96ff9bc82d0 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -866,6 +866,7 @@ extern unsigned long __must_check vm_mmap_pgoff(struct file *, unsigned long,
> unsigned long, unsigned long);
>
> extern void set_pageblock_order(void);
> +struct folio *alloc_migrate_folio(struct folio *src, unsigned long private);
> unsigned long reclaim_pages(struct list_head *folio_list);
> unsigned int reclaim_clean_pages_from_list(struct zone *zone,
> struct list_head *folio_list);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 4255619a1a31..9e456cac03b4 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -910,8 +910,7 @@ static void folio_check_dirty_writeback(struct folio *folio,
> mapping->a_ops->is_dirty_writeback(folio, dirty, writeback);
> }
>
> -static struct folio *alloc_demote_folio(struct folio *src,
> - unsigned long private)
> +struct folio *alloc_migrate_folio(struct folio *src, unsigned long private)
> {
> struct folio *dst;
> nodemask_t *allowed_mask;
> @@ -935,6 +934,11 @@ static struct folio *alloc_demote_folio(struct folio *src,
> if (dst)
> return dst;
>
> + /*
> + * Allocation failed from the target node so try to allocate from
> + * fallback nodes based on allowed_mask.
> + * See fallback_alloc() at mm/slab.c.
> + */
I think this might better to be a separate cleanup patch, but given its small
size, I have no strong opinion.
> mtc->gfp_mask &= ~__GFP_THISNODE;
> mtc->nmask = allowed_mask;
>
> @@ -973,7 +977,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios,
> node_get_allowed_targets(pgdat, &allowed_mask);
>
> /* Demotion ignores all cpuset and mempolicy settings */
> - migrate_pages(demote_folios, alloc_demote_folio, NULL,
> + migrate_pages(demote_folios, alloc_migrate_folio, NULL,
> (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION,
> &nr_succeeded);
>
> --
> 2.34.1
Thanks,
SJ
