[PATCH] mm/damon/core: Refactor damos_set_effective_quota

[PATCH] mm/damon/core: Refactor damos_set_effective_quota

[Honggyu Kim]

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

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

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;
 
 	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

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

[SeongJae Park]

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

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

[Honggyu Kim]

On Fri, 22 Nov 2024 11:29:26 -0800 SeongJae Park <sj@kernel.org> wrote:
> 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"?

Hi SeongJae,

Thanks for the review, but IMHO, setting the default value of esz is to remove
one of duplicate if statement, not the goal of this patch.

So I think the following title might be better.

  mm/damon/core: remove duplicate list_empty quota->goals check

Is this okay for you?

> 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.

Ack. I will apply this and send v2 soon.

Thanks,
Honggyu

> >  
> >  	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

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

[SeongJae Park]

On Mon, 25 Nov 2024 13:49:33 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> On Fri, 22 Nov 2024 11:29:26 -0800 SeongJae Park <sj@kernel.org> wrote:
> > 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"?
> 
> Hi SeongJae,
> 
> Thanks for the review, but IMHO, setting the default value of esz is to remove
> one of duplicate if statement, not the goal of this patch.
> 
> So I think the following title might be better.
> 
>   mm/damon/core: remove duplicate list_empty quota->goals check
> 
> Is this okay for you?

Yes, this looks specific and non-ambiguous enough to me.

[...]
> > > --- 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.
> 
> Ack. I will apply this and send v2 soon.

Thank you, I will review it soon :)


Thanks,
SJ

[...]