[RFC PATCH 07/18] mm/damon/paddr: report filter-passed bytes back for DAMOS_STAT action

SeongJae Park posted 18 patches 12 months ago
[RFC PATCH 07/18] mm/damon/paddr: report filter-passed bytes back for DAMOS_STAT action
Posted by SeongJae Park 12 months ago
DAMOS_STAT action handling of paddr DAMON operations set implementation
is simply ignoring the filters, and therefore not reporting back the
filter-passed bytes.  Apply the filtrs and report back the information.

Before this change, DAMOS_STAT was doing nothing for DAMOS filters.
Hence users might see some performance regressions.  Such regression for
use cases where no filter is added to the scheme will be negligible,
since this change implementation avoid unnecessary filtering works if no
filter is installed.

For old users who were using DAMOS_STAT with filters, the regression
could be visible depending on the size of the region and the overhead of
the installed DAMOS filters.  But, because the filters were completely
ignored before in the use case, no real users would really depend on
such use case that makes no point.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/paddr.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 5944316a0b4c..b0c283808ba6 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -485,6 +485,39 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
 	return applied * PAGE_SIZE;
 }
 
+static bool damon_pa_scheme_has_filter(struct damos *s)
+{
+	struct damos_filter *f;
+
+	damos_for_each_filter(f, s)
+		return true;
+	return false;
+}
+
+static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s,
+		unsigned long *sz_filter_passed)
+{
+	unsigned long addr;
+	LIST_HEAD(folio_list);
+
+	if (!damon_pa_scheme_has_filter(s))
+		return 0;
+
+	for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
+		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+
+		if (!folio)
+			continue;
+
+		if (damos_pa_filter_out(s, folio))
+			goto put_folio;
+		else
+			*sz_filter_passed += folio_size(folio);
+put_folio:
+		folio_put(folio);
+	}
+	return 0;
+}
 
 static unsigned long damon_pa_apply_scheme(struct damon_ctx *ctx,
 		struct damon_target *t, struct damon_region *r,
@@ -501,7 +534,7 @@ static unsigned long damon_pa_apply_scheme(struct damon_ctx *ctx,
 	case DAMOS_MIGRATE_COLD:
 		return damon_pa_migrate(r, scheme, sz_filter_passed);
 	case DAMOS_STAT:
-		break;
+		return damon_pa_stat(r, scheme, sz_filter_passed);
 	default:
 		/* DAMOS actions that not yet supported by 'paddr'. */
 		break;
-- 
2.39.5
Re: [RFC PATCH 07/18] mm/damon/paddr: report filter-passed bytes back for DAMOS_STAT action
Posted by Joshua Hahn 11 months, 1 week ago
On Wed, 18 Dec 2024 20:03:16 -0800 SeongJae Park <sj@kernel.org> wrote:

Hello SJ,
I hope you are doing well! Sorry to add noise to an old mail, but I recently
saw Usama's patch that improves this function, and it brought my attention
to this series, so I have been reading it today.

I was unsure if I should send this mail because I had a nit / naive question
about the patch:

[...snip...]

> +static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s,
> +		unsigned long *sz_filter_passed)
> +{
> +	unsigned long addr;
> +	LIST_HEAD(folio_list);
> +
> +	if (!damon_pa_scheme_has_filter(s))
> +		return 0;
> +
> +	for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
> +		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
> +
> +		if (!folio)
> +			continue;
> +
> +		if (damos_pa_filter_out(s, folio))
> +			goto put_folio;
> +		else
> +			*sz_filter_passed += folio_size(folio);
> +put_folio:
> +		folio_put(folio);
> +	}
> +	return 0;

Is there a reason that we decide to use a goto statement here? As far as I
can tell, this is the only place this goto statement is used, and the else
case bleeds into it as well. That is, I believe that the following could be
more readable:

		if (!damos_pa_filter_out(s, folio))
			*sz_filter_passed += folio_size(folio);

		folio_put(folio);
	}
	return 0;

[...snip...]

Again, I am sorry if this is a naive question. Thank you for your time,
I hope you have a great day!
Joshua
Re: [RFC PATCH 07/18] mm/damon/paddr: report filter-passed bytes back for DAMOS_STAT action
Posted by SeongJae Park 11 months, 1 week ago
Hi Joshua,

On Mon, 13 Jan 2025 11:49:37 -0800 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:

> On Wed, 18 Dec 2024 20:03:16 -0800 SeongJae Park <sj@kernel.org> wrote:
> 
> Hello SJ,
> I hope you are doing well! Sorry to add noise to an old mail, but I recently
> saw Usama's patch that improves this function, and it brought my attention
> to this series, so I have been reading it today.
> 
> I was unsure if I should send this mail because I had a nit / naive question
> about the patch:
> 
> [...snip...]
> 
> > +static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s,
> > +		unsigned long *sz_filter_passed)
> > +{
> > +	unsigned long addr;
> > +	LIST_HEAD(folio_list);
> > +
> > +	if (!damon_pa_scheme_has_filter(s))
> > +		return 0;
> > +
> > +	for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
> > +		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
> > +
> > +		if (!folio)
> > +			continue;
> > +
> > +		if (damos_pa_filter_out(s, folio))
> > +			goto put_folio;
> > +		else
> > +			*sz_filter_passed += folio_size(folio);
> > +put_folio:
> > +		folio_put(folio);
> > +	}
> > +	return 0;
> 
> Is there a reason that we decide to use a goto statement here?

There was no special reason.  I wrote this function by copying
damon_pa_migrate() and removing parts that not necessary.  As a result, the
weird 'goto' has remained.

> As far as I
> can tell, this is the only place this goto statement is used, and the else
> case bleeds into it as well. That is, I believe that the following could be
> more readable:
> 
> 		if (!damos_pa_filter_out(s, folio))
> 			*sz_filter_passed += folio_size(folio);
> 
> 		folio_put(folio);
> 	}
> 	return 0;

I agree.  This looks much better!  If you don't mind, pleae send a patch!

> 
> [...snip...]
> 
> Again, I am sorry if this is a naive question.

No worry, thank you for this nice and important question!  I believe
readability matters and DAMON code needs many helps for that :)

> Thank you for your time,
> I hope you have a great day!

You too!


Thanks,
SJ

[...]