Re: [PATCH] DAMON: Fix return value from damos_wmark_metric_value

[SeongJae Park <sj@kernel.org>]

Hi Alex,


Thank you for this patch!  I have a few comments below.

For the consistency, could you please use 'mm/damon/core:' as the patch subject
prefix instead of 'DAMON:'?

On Sun, 28 Apr 2024 18:49:33 +0300 Alex Rusuf <yorha.op@gmail.com> wrote:

> Signed-off-by: Alex Rusuf <yorha.op@gmail.com>

Could you pleae add more detailed explanation about this patch, including why
the return value is wrong?

And, I think we would better to add below tags?

Fixes: ee801b7dd782 ("mm/damon/schemes: activate schemes based on a watermarks mechanism")

> ---
>  mm/damon/core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 6d503c1c125e..994ccd272555 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1480,12 +1480,14 @@ static bool kdamond_need_stop(struct damon_ctx *ctx)
>  	return true;
>  }
>  
> -static unsigned long damos_wmark_metric_value(enum damos_wmark_metric metric)
> +static int damos_wmark_metric_value(enum damos_wmark_metric metric,
> +				    unsigned long *metric_value)

Because the return value is no more the metric value, what about renaming this
function to, say, 'damos_get_wmark_metric_value()'?

>  {
>  	switch (metric) {
>  	case DAMOS_WMARK_FREE_MEM_RATE:
> -		return global_zone_page_state(NR_FREE_PAGES) * 1000 /
> +		*metric_value = global_zone_page_state(NR_FREE_PAGES) * 1000 /
>  		       totalram_pages();
> +		return 0;
>  	default:
>  		break;
>  	}
> @@ -1500,10 +1502,10 @@ static unsigned long damos_wmark_wait_us(struct damos *scheme)
>  {
>  	unsigned long metric;
>  
> -	if (scheme->wmarks.metric == DAMOS_WMARK_NONE)
> +	if (scheme->wmarks.metric == DAMOS_WMARK_NONE ||
> +	    damos_wmark_metric_value(scheme->wmarks.metric, &metric))
>  		return 0;

DAMOS_WMARK_NONE check can be handled from damos_wmark_metric_value() function.
Could you please remove the check here?

>  
> -	metric = damos_wmark_metric_value(scheme->wmarks.metric);
>  	/* higher than high watermark or lower than low watermark */
>  	if (metric > scheme->wmarks.high || scheme->wmarks.low > metric) {
>  		if (scheme->wmarks.activated)
> -- 
> 2.42.0


Thanks,
SJ