Re: [PATCH] mm/damon/core: Refactor damos_set_effective_quota

[SeongJae Park <sj@kernel.org>]

Hi Honggyu,


Thank you for this patch.  Definitely DAMON needs many refactoring and
cleanups.

The subject of the patch is a little bit ambiguous to me.  Could we make it
more specific, e.g., "set esz of damos_set_effective_quota() as ULONG_MAX by
default"?

On Fri, 22 Nov 2024 17:18:34 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> The damos_set_effective_quota checks quota contidions but there are some
> duplicate checks for quota->goals inside.

From the above, I was thinking the duplicates should be somewhat unnecessary.
And indeed the function list_empty(&quota->goal) multiple times.  But, that's
not for a unnecessary reason.  We need to know whether 'esz' is set or not.
The check is for that.

> 
> This patch reduces one of if statement to simplify the esz calculation
> logic.

More technically speaking, this patch gives 'esz' a safe default value, and
removes the checks of 'esz' initialization, which is no more necessary owing to
the default value change.

Honggyu, could you please update the subject and message to more clearly
explain the change?

> 
> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> ---
>  mm/damon/core.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 511c3f61ab44..eb2761866dda 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1542,7 +1542,7 @@ static unsigned long damos_quota_score(struct damos_quota *quota)
>  static void damos_set_effective_quota(struct damos_quota *quota)
>  {
>  	unsigned long throughput;
> -	unsigned long esz;
> +	unsigned long esz = UINT_MAX;

Let's use ULONG_MAX.

>  
>  	if (!quota->ms && list_empty(&quota->goals)) {
>  		quota->esz = quota->sz;
> @@ -1564,10 +1564,7 @@ static void damos_set_effective_quota(struct damos_quota *quota)
>  				quota->total_charged_ns;
>  		else
>  			throughput = PAGE_SIZE * 1024;
> -		if (!list_empty(&quota->goals))
> -			esz = min(throughput * quota->ms, esz);
> -		else
> -			esz = throughput * quota->ms;
> +		esz = min(throughput * quota->ms, esz);
>  	}
>  
>  	if (quota->sz && quota->sz < esz)
> -- 
> 2.34.1


Thanks,
SJ