[PATCH] mm/damon/core: accumulate applied size to quota->charged_sz

[PATCH] mm/damon/core: accumulate applied size to quota->charged_sz

[Honggyu Kim]

While testing DAMOS_MIGRATE_COLD action, Yunjeong found that the
quota->charged_sz is way less than actually migrated size.

In damos_apply_scheme(), quota->charged_sz is used when checking whether
the quota exceeds the effective size.  The current implementation
accumulates the charged_sz with sz(region size), but many pages in the
region might be discarded by DAMOS filters.

In order to make the quota calculation more accurate, charged_sz is
better to be accumulated by the actually applied size, which is
sz_applied in the code.

Reported-by: Yunjeong Mun <yunjeong.mun@sk.com>
Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
---
 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 511c3f61ab44..dd1b56466650 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1390,7 +1390,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		ktime_get_coarse_ts64(&end);
 		quota->total_charged_ns += timespec64_to_ns(&end) -
 			timespec64_to_ns(&begin);
-		quota->charged_sz += sz;
+		quota->charged_sz += sz_applied;
 		if (quota->esz && quota->charged_sz >= quota->esz) {
 			quota->charge_target_from = t;
 			quota->charge_addr_from = r->ar.end + 1;

base-commit: adc218676eef25575469234709c2d87185ca223a
-- 
2.34.1

Re: [PATCH] mm/damon/core: accumulate applied size to quota->charged_sz

[SeongJae Park]

On Fri, 29 Nov 2024 17:31:01 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> While testing DAMOS_MIGRATE_COLD action, Yunjeong found that the
> quota->charged_sz is way less than actually migrated size.

You mean "more", not "less", right?

And, is it a problem for your use case?  Could you please share more details?

> 
> In damos_apply_scheme(), quota->charged_sz is used when checking whether
> the quota exceeds the effective size.  The current implementation
> accumulates the charged_sz with sz(region size), but many pages in the
> region might be discarded by DAMOS filters.
> 
> In order to make the quota calculation more accurate, charged_sz is
> better to be accumulated by the actually applied size, which is
> sz_applied in the code.

This is an intended behavior.  Quota is for making a limit of resource
consumption from DAMOS.  Trying a DAMOS action consumes some of the system
resource even if it was eventually filtered out or failed, so we charge those.

We can consider changes of the behaior depending on problems and use cases, but
it is unclear to me.  Could you please elaborate, as I requested above?


Thanks,
SJ

[...]

Re: [PATCH] mm/damon/core: accumulate applied size to quota->charged_sz

[Honggyu Kim]

Hi SeongJae,

Thanks for the feedback.  Please see my comments below.

On Fri, 29 Nov 2024 11:55:29 -0800 SeongJae Park <sj@kernel.org> wrote:
> On Fri, 29 Nov 2024 17:31:01 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
> > While testing DAMOS_MIGRATE_COLD action, Yunjeong found that the
> > quota->charged_sz is way less than actually migrated size.
> 
> You mean "more", not "less", right?

Correct.  I wrote it opposite.  quota->charged_sz is highly accumulated
even if zero pages are affected by the migrate_cold action.

> And, is it a problem for your use case?  Could you please share more details?

I think this is for general use cases.  I thought this is logically
quite straightforward but I will see if I can provide more details if
you need.

> > 
> > In damos_apply_scheme(), quota->charged_sz is used when checking whether
> > the quota exceeds the effective size.  The current implementation
> > accumulates the charged_sz with sz(region size), but many pages in the
> > region might be discarded by DAMOS filters.
> > 
> > In order to make the quota calculation more accurate, charged_sz is
> > better to be accumulated by the actually applied size, which is
> > sz_applied in the code.
> 
> This is an intended behavior.  Quota is for making a limit of resource
> consumption from DAMOS.

Sure. We know about the purpose of quota.

> Trying a DAMOS action consumes some of the system
> resource even if it was eventually filtered out or failed, so we charge those.

I would like to have more discussion on this.  Although trying a DAMOS
action consumes some resouce but it seems to be much smaller compared to
applying actions. (especially in migrate actions)

You might think it takes some resource, but it is more close to the concept
of time quota.  We just compare the size with esz when making decision
of quota usage so I'm not quite sure if we really have to accumulate
the size of discarded pages.

> We can consider changes of the behaior depending on problems and use cases, but
> it is unclear to me.  Could you please elaborate, as I requested above?

It might take some time to prepare for the backup data but I just share
what I have thought about it.

Roughly speaking, we found that quota->charged_sz increases 90 MiB for
each interval in our test, but we saw no migration happened when
applying the migrate_cold action.

Let's say that there is a region of 90 MiB and that consists of 23040
pages.  If there are 10 MiB of pages at the end of the region, but the
initial pages are not candidates because of filter setup.  In this case,
the current implementation may consume the most of charged_sz at the
beginning so it discards the last 10 MiB of pages that we are
interested because quota->charged_sz + sz is already greater than
quota->esz although no actions applied.

My explanation is not good enough, but hope this can be helpful.

Thanks,
Honggyu

> 
> Thanks,
> SJ
> 
> [...]

Re: [PATCH] mm/damon/core: accumulate applied size to quota->charged_sz

[SeongJae Park]

On Mon, 2 Dec 2024 21:24:05 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> Hi SeongJae,
> 
> Thanks for the feedback.  Please see my comments below.
> 
> On Fri, 29 Nov 2024 11:55:29 -0800 SeongJae Park <sj@kernel.org> wrote:
> > On Fri, 29 Nov 2024 17:31:01 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> > 
> > > While testing DAMOS_MIGRATE_COLD action, Yunjeong found that the
> > > quota->charged_sz is way less than actually migrated size.
> > 
> > You mean "more", not "less", right?
> 
> Correct.  I wrote it opposite.  quota->charged_sz is highly accumulated
> even if zero pages are affected by the migrate_cold action.
> 
> > And, is it a problem for your use case?  Could you please share more details?
> 
> I think this is for general use cases.  I thought this is logically
> quite straightforward but I will see if I can provide more details if
> you need.

Even if this is for general use cases, providing a concrete example would be
always nice.  Especially for this kind of behavioral changes, I'd like to have
the details.

> 
> > > 
> > > In damos_apply_scheme(), quota->charged_sz is used when checking whether
> > > the quota exceeds the effective size.  The current implementation
> > > accumulates the charged_sz with sz(region size), but many pages in the
> > > region might be discarded by DAMOS filters.
> > > 
> > > In order to make the quota calculation more accurate, charged_sz is
> > > better to be accumulated by the actually applied size, which is
> > > sz_applied in the code.
> > 
> > This is an intended behavior.  Quota is for making a limit of resource
> > consumption from DAMOS.
> 
> Sure. We know about the purpose of quota.
> 
> > Trying a DAMOS action consumes some of the system
> > resource even if it was eventually filtered out or failed, so we charge those.
> 
> I would like to have more discussion on this.  Although trying a DAMOS
> action consumes some resouce but it seems to be much smaller compared to
> applying actions. (especially in migrate actions)
> 
> You might think it takes some resource, but it is more close to the concept
> of time quota.  We just compare the size with esz when making decision
> of quota usage so I'm not quite sure if we really have to accumulate
> the size of discarded pages.

I agree failing a DAMOS action would be lighter.  But, it's still not zero.
Depending on use case, I think that could be somewhat cannot be ignored.  For
example, if a huge amount of memory is filtered-out by page-granular DAMOS
filters (e.g., young), the overhead would be more than what we want to ignore.

> 
> > We can consider changes of the behaior depending on problems and use cases, but
> > it is unclear to me.  Could you please elaborate, as I requested above?
> 
> It might take some time to prepare for the backup data but I just share
> what I have thought about it.

This is a behavioral change.  I'd like to make such change only if the theory
is very clear, or there is a real problem.

> 
> Roughly speaking, we found that quota->charged_sz increases 90 MiB for
> each interval in our test, but we saw no migration happened when
> applying the migrate_cold action.
> 
> Let's say that there is a region of 90 MiB and that consists of 23040
> pages.  If there are 10 MiB of pages at the end of the region, but the
> initial pages are not candidates because of filter setup.  In this case,
> the current implementation may consume the most of charged_sz at the
> beginning so it discards the last 10 MiB of pages that we are
> interested because quota->charged_sz + sz is already greater than
> quota->esz although no actions applied.

I'm not very sure if I'm understanding your concern well.  Are you saying DAMOS
will _never_ apply the action to the 10 MiB region?

DAMOS resets the age of a region after applying an action[1], regardless of the
failure.  Hence in this example, yes, DAMOS may not apply the action to the 10
MiB region immediately, due to the quota.  But in the next interval or aftr,
DAMOS will eventually apply the action to the region.

Or, are you saying DAMOS action will not _immediately_ be applied to the 10 MiB
region, and the slow speed is your concern?

On a level of time scales of real workloads, I think that's not a big problem.
If that's causing a real problem, users could also adjust the quota online on
their own.

Of course I might underestimating something, and I will be happy to be
enlightened.  Real data and use cases would be very helpful for that.

> 
> My explanation is not good enough, but hope this can be helpful.

It is very helpful, thank you :)

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/mm/damon/design.rst?h=v6.13-rc1#n354


Thanks,
SJ

[...]