Re: [RFC PATCH v3 1/7] mm/damon/paddr: refactor DAMOS_PAGEOUT with migration_mode

[SeongJae Park <sj@kernel.org>]

On Fri,  5 Apr 2024 15:08:50 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> This is a preparation patch that introduces migration modes.
> 
> The damon_pa_pageout is renamed to damon_pa_migrate and it receives an
> extra argument for migration_mode.

I personally think keeping damon_pa_pageout() as is and adding a new function
(damon_pa_migrate()) with some duplicated code is also ok, but this approach
also looks fine to me.  So I have no strong opinion here, but just letting you
know I would have no objection at both approaches.

> 
> No functional changes applied.
> 
> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> ---
>  mm/damon/paddr.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 081e2a325778..277a1c4d833c 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -224,7 +224,12 @@ static bool damos_pa_filter_out(struct damos *scheme, struct folio *folio)
>  	return false;
>  }
>  
> -static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
> +enum migration_mode {
> +	MIG_PAGEOUT,
> +};

To avoid name conflicts, what about renaming to 'damos_migration_mode' and
'DAMOS_MIG_PAGEOUT'?

> +
> +static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
> +				      enum migration_mode mm)

My poor brain has a bit confused with the name.  What about calling it 'mode'?

>  {
>  	unsigned long addr, applied;
>  	LIST_HEAD(folio_list);
> @@ -249,7 +254,14 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
>  put_folio:
>  		folio_put(folio);
>  	}
> -	applied = reclaim_pages(&folio_list);
> +	switch (mm) {
> +	case MIG_PAGEOUT:
> +		applied = reclaim_pages(&folio_list);
> +		break;
> +	default:
> +		/* Unexpected migration mode. */
> +		return 0;
> +	}
>  	cond_resched();
>  	return applied * PAGE_SIZE;
>  }
> @@ -297,7 +309,7 @@ static unsigned long damon_pa_apply_scheme(struct damon_ctx *ctx,
>  {
>  	switch (scheme->action) {
>  	case DAMOS_PAGEOUT:
> -		return damon_pa_pageout(r, scheme);
> +		return damon_pa_migrate(r, scheme, MIG_PAGEOUT);
>  	case DAMOS_LRU_PRIO:
>  		return damon_pa_mark_accessed(r, scheme);
>  	case DAMOS_LRU_DEPRIO:
> -- 
> 2.34.1


Thanks,
SJ