* [PATCH v1 1/2] mm/damon: Move invalid folio and has filter to ops-common
2025-07-29 13:53 [PATCH v1 0/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
@ 2025-07-29 13:53 ` Yueyang Pan
2025-07-29 18:20 ` SeongJae Park
2025-07-29 13:53 ` [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
2025-07-29 18:34 ` [PATCH v1 0/2] " SeongJae Park
2 siblings, 1 reply; 10+ messages in thread
From: Yueyang Pan @ 2025-07-29 13:53 UTC (permalink / raw)
To: SeongJae Park, Andrew Morton, Usama Arif
Cc: damon, linux-mm, linux-kernel, PanJason
From: PanJason <pyyjason@gmail.com>
This patch moves the damon_pa_invalid_damos_folio and
damon_pa_scheme_has_filter to ops-common. renaming them
to damon_invalid_damos_folio and damon_scheme_has_filter.
Doing so allows us to reuse their logic in the vaddr version
of DAMOS_STAT
---
mm/damon/ops-common.c | 19 +++++++++++++++++++
mm/damon/ops-common.h | 3 +++
mm/damon/paddr.c | 29 +++++------------------------
3 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
index 99321ff5cb92..7d3b48cc0f86 100644
--- a/mm/damon/ops-common.c
+++ b/mm/damon/ops-common.c
@@ -412,3 +412,22 @@ unsigned long damon_migrate_pages(struct list_head *folio_list, int target_nid)
return nr_migrated;
}
+
+bool damon_scheme_has_filter(struct damos *s)
+{
+ struct damos_filter *f;
+ damos_for_each_ops_filter(f, s)
+ return true;
+ return false;
+}
+
+bool damon_invalid_damos_folio(struct folio *folio, struct damos *s)
+{
+ if (!folio)
+ return true;
+ if (folio == s->last_applied) {
+ folio_put(folio);
+ return true;
+ }
+ return false;
+}
diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h
index 61ad54aaf256..4e905477fdce 100644
--- a/mm/damon/ops-common.h
+++ b/mm/damon/ops-common.h
@@ -21,3 +21,6 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
bool damos_folio_filter_match(struct damos_filter *filter, struct folio *folio);
unsigned long damon_migrate_pages(struct list_head *folio_list, int target_nid);
+
+bool damon_scheme_has_filter(struct damos *s);
+bool damon_invalid_damos_folio(struct folio *folio, struct damos *s);
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 53a55c5114fb..a8b7048e871e 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -114,16 +114,6 @@ static bool damos_pa_filter_out(struct damos *scheme, struct folio *folio)
return scheme->ops_filters_default_reject;
}
-static bool damon_pa_invalid_damos_folio(struct folio *folio, struct damos *s)
-{
- if (!folio)
- return true;
- if (folio == s->last_applied) {
- folio_put(folio);
- return true;
- }
- return false;
-}
static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
unsigned long *sz_filter_passed)
@@ -152,7 +142,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
addr = r->ar.start;
while (addr < r->ar.end) {
folio = damon_get_folio(PHYS_PFN(addr));
- if (damon_pa_invalid_damos_folio(folio, s)) {
+ if (damon_invalid_damos_folio(folio, s)) {
addr += PAGE_SIZE;
continue;
}
@@ -192,7 +182,7 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
addr = r->ar.start;
while (addr < r->ar.end) {
folio = damon_get_folio(PHYS_PFN(addr));
- if (damon_pa_invalid_damos_folio(folio, s)) {
+ if (damon_invalid_damos_folio(folio, s)) {
addr += PAGE_SIZE;
continue;
}
@@ -239,7 +229,7 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
addr = r->ar.start;
while (addr < r->ar.end) {
folio = damon_get_folio(PHYS_PFN(addr));
- if (damon_pa_invalid_damos_folio(folio, s)) {
+ if (damon_invalid_damos_folio(folio, s)) {
addr += PAGE_SIZE;
continue;
}
@@ -262,28 +252,19 @@ 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_ops_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;
struct folio *folio;
- if (!damon_pa_scheme_has_filter(s))
+ if (!damon_scheme_has_filter(s))
return 0;
addr = r->ar.start;
while (addr < r->ar.end) {
folio = damon_get_folio(PHYS_PFN(addr));
- if (damon_pa_invalid_damos_folio(folio, s)) {
+ if (damon_invalid_damos_folio(folio, s)) {
addr += PAGE_SIZE;
continue;
}
--
2.47.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v1 1/2] mm/damon: Move invalid folio and has filter to ops-common
2025-07-29 13:53 ` [PATCH v1 1/2] mm/damon: Move invalid folio and has filter to ops-common Yueyang Pan
@ 2025-07-29 18:20 ` SeongJae Park
0 siblings, 0 replies; 10+ messages in thread
From: SeongJae Park @ 2025-07-29 18:20 UTC (permalink / raw)
To: Yueyang Pan
Cc: SeongJae Park, Andrew Morton, Usama Arif, damon, linux-mm,
linux-kernel
On Tue, 29 Jul 2025 06:53:29 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:
> From: PanJason <pyyjason@gmail.com>
>
> This patch moves the damon_pa_invalid_damos_folio and
> damon_pa_scheme_has_filter to ops-common. renaming them
> to damon_invalid_damos_folio and damon_scheme_has_filter.
> Doing so allows us to reuse their logic in the vaddr version
> of DAMOS_STAT
You forgot adding your Signed-off-by:
> ---
> mm/damon/ops-common.c | 19 +++++++++++++++++++
> mm/damon/ops-common.h | 3 +++
> mm/damon/paddr.c | 29 +++++------------------------
> 3 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index 99321ff5cb92..7d3b48cc0f86 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -412,3 +412,22 @@ unsigned long damon_migrate_pages(struct list_head *folio_list, int target_nid)
>
> return nr_migrated;
> }
> +
> +bool damon_scheme_has_filter(struct damos *s)
> +{
> + struct damos_filter *f;
> + damos_for_each_ops_filter(f, s)
> + return true;
> + return false;
> +}
> +
> +bool damon_invalid_damos_folio(struct folio *folio, struct damos *s)
> +{
> + if (!folio)
> + return true;
> + if (folio == s->last_applied) {
> + folio_put(folio);
> + return true;
> + }
> + return false;
> +}
Unless you have different opinions about this function I mentioned on the reply
to the next patch, let's not move this function.
> diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h
> index 61ad54aaf256..4e905477fdce 100644
> --- a/mm/damon/ops-common.h
> +++ b/mm/damon/ops-common.h
> @@ -21,3 +21,6 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
>
> bool damos_folio_filter_match(struct damos_filter *filter, struct folio *folio);
> unsigned long damon_migrate_pages(struct list_head *folio_list, int target_nid);
> +
> +bool damon_scheme_has_filter(struct damos *s);
> +bool damon_invalid_damos_folio(struct folio *folio, struct damos *s);
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 53a55c5114fb..a8b7048e871e 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -114,16 +114,6 @@ static bool damos_pa_filter_out(struct damos *scheme, struct folio *folio)
> return scheme->ops_filters_default_reject;
> }
>
> -static bool damon_pa_invalid_damos_folio(struct folio *folio, struct damos *s)
> -{
> - if (!folio)
> - return true;
> - if (folio == s->last_applied) {
> - folio_put(folio);
> - return true;
> - }
> - return false;
> -}
>
> static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
> unsigned long *sz_filter_passed)
> @@ -152,7 +142,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
> addr = r->ar.start;
> while (addr < r->ar.end) {
> folio = damon_get_folio(PHYS_PFN(addr));
> - if (damon_pa_invalid_damos_folio(folio, s)) {
> + if (damon_invalid_damos_folio(folio, s)) {
> addr += PAGE_SIZE;
> continue;
> }
> @@ -192,7 +182,7 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
> addr = r->ar.start;
> while (addr < r->ar.end) {
> folio = damon_get_folio(PHYS_PFN(addr));
> - if (damon_pa_invalid_damos_folio(folio, s)) {
> + if (damon_invalid_damos_folio(folio, s)) {
> addr += PAGE_SIZE;
> continue;
> }
> @@ -239,7 +229,7 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
> addr = r->ar.start;
> while (addr < r->ar.end) {
> folio = damon_get_folio(PHYS_PFN(addr));
> - if (damon_pa_invalid_damos_folio(folio, s)) {
> + if (damon_invalid_damos_folio(folio, s)) {
> addr += PAGE_SIZE;
> continue;
> }
> @@ -262,28 +252,19 @@ 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_ops_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;
> struct folio *folio;
>
> - if (!damon_pa_scheme_has_filter(s))
> + if (!damon_scheme_has_filter(s))
> return 0;
>
> addr = r->ar.start;
> while (addr < r->ar.end) {
> folio = damon_get_folio(PHYS_PFN(addr));
> - if (damon_pa_invalid_damos_folio(folio, s)) {
> + if (damon_invalid_damos_folio(folio, s)) {
> addr += PAGE_SIZE;
> continue;
> }
> --
> 2.47.3
Other than above, looks good to me.
Thanks,
SJ
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr
2025-07-29 13:53 [PATCH v1 0/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
2025-07-29 13:53 ` [PATCH v1 1/2] mm/damon: Move invalid folio and has filter to ops-common Yueyang Pan
@ 2025-07-29 13:53 ` Yueyang Pan
2025-07-29 14:11 ` David Hildenbrand
` (2 more replies)
2025-07-29 18:34 ` [PATCH v1 0/2] " SeongJae Park
2 siblings, 3 replies; 10+ messages in thread
From: Yueyang Pan @ 2025-07-29 13:53 UTC (permalink / raw)
To: SeongJae Park, Andrew Morton, Usama Arif
Cc: damon, linux-mm, linux-kernel, PanJason
From: PanJason <pyyjason@gmail.com>
This patch adds support for damos_stat in virtual address space.
It leverages the walk_page_range to walk the page table and gets
the folio from page table. The last folio scanned is stored in
damos->last_applied to prevent double counting.
---
mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 112 insertions(+), 1 deletion(-)
diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index 87e825349bdf..3e319b51cfd4 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
return applied * PAGE_SIZE;
}
+struct damos_va_stat_private {
+ struct damos *scheme;
+ unsigned long *sz_filter_passed;
+};
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
+ unsigned long next, struct mm_walk *walk)
+{
+ struct damos_va_stat_private *priv = walk->private;
+ struct damos *s = priv->scheme;
+ unsigned long *sz_filter_passed = priv->sz_filter_passed;
+ struct folio *folio;
+ spinlock_t *ptl;
+ pmd_t pmde;
+
+ ptl = pmd_lock(walk->mm, pmd);
+ pmde = pmdp_get(pmd);
+
+ if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
+ goto unlock;
+
+ /* Tell page walk code to not split the PMD */
+ walk->action = ACTION_CONTINUE;
+
+ folio = damon_get_folio(pmd_pfn(pmde));
+ if (!folio)
+ goto unlock;
+
+ if (damon_invalid_damos_folio(folio, s))
+ goto update_last_applied;
+
+ if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
+ *sz_filter_passed += folio_size(folio);
+ }
+
+ folio_put(folio);
+update_last_applied:
+ s->last_applied = folio;
+unlock:
+ spin_unlock(ptl);
+ return 0;
+}
+#else
+#define damon_va_stat_pmd_entry NULL
+#endif
+
+static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
+ unsigned long next, struct mm_walk *walk)
+{
+ struct damos_va_stat_private *priv = walk->private;
+ struct damos *s = priv->scheme;
+ unsigned long *sz_filter_passed = priv->sz_filter_passed;
+ struct folio *folio;
+ pte_t ptent;
+
+ ptent = ptep_get(pte);
+ if (pte_none(ptent) || !pte_present(ptent))
+ return 0;
+
+ folio = damon_get_folio(pte_pfn(ptent));
+ if (!folio)
+ return 0;
+
+ if (damon_invalid_damos_folio(folio, s))
+ goto update_last_applied;
+
+ if (!damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL)){
+ *sz_filter_passed += folio_size(folio);
+ }
+
+ folio_put(folio);
+
+update_last_applied:
+ s->last_applied = folio;
+ return 0;
+}
+
+static unsigned long damos_va_stat(struct damon_target *target,
+ struct damon_region *r, struct damos *s,
+ unsigned long *sz_filter_passed)
+{
+
+ struct damos_va_stat_private priv;
+ struct mm_struct *mm;
+ struct mm_walk_ops walk_ops = {
+ .pmd_entry = damos_va_stat_pmd_entry,
+ .pte_entry = damos_va_stat_pte_entry,
+ .walk_lock = PGWALK_RDLOCK,
+ };
+
+ priv.scheme = s;
+ priv.sz_filter_passed = sz_filter_passed;
+
+ if (!damon_scheme_has_filter(s)){
+ return 0;
+ }
+
+ mm = damon_get_mm(target);
+ if (!mm)
+ return 0;
+
+ mmap_read_lock(mm);
+ walk_page_range(mm, r->ar.start, r->ar.end, &walk_ops, &priv);
+ mmap_read_unlock(mm);
+ mmput(mm);
+ pr_debug("Call va_stat: %lu\n", *sz_filter_passed);
+ return 0;
+
+}
+
static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
struct damon_target *t, struct damon_region *r,
struct damos *scheme, unsigned long *sz_filter_passed)
@@ -916,7 +1027,7 @@ static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
case DAMOS_MIGRATE_COLD:
return damos_va_migrate(t, r, scheme, sz_filter_passed);
case DAMOS_STAT:
- return 0;
+ return damos_va_stat(t, r, scheme, sz_filter_passed);
default:
/*
* DAMOS actions that are not yet supported by 'vaddr'.
--
2.47.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr
2025-07-29 13:53 ` [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
@ 2025-07-29 14:11 ` David Hildenbrand
2025-07-29 17:46 ` SeongJae Park
2025-07-30 22:24 ` Yueyang Pan
2025-07-29 18:15 ` SeongJae Park
2025-07-29 18:21 ` SeongJae Park
2 siblings, 2 replies; 10+ messages in thread
From: David Hildenbrand @ 2025-07-29 14:11 UTC (permalink / raw)
To: Yueyang Pan, SeongJae Park, Andrew Morton, Usama Arif
Cc: damon, linux-mm, linux-kernel
On 29.07.25 15:53, Yueyang Pan wrote:
> From: PanJason <pyyjason@gmail.com>
>
> This patch adds support for damos_stat in virtual address space.
> It leverages the walk_page_range to walk the page table and gets
> the folio from page table. The last folio scanned is stored in
> damos->last_applied to prevent double counting.
> ---
> mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 87e825349bdf..3e319b51cfd4 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
> return applied * PAGE_SIZE;
> }
>
> +struct damos_va_stat_private {
> + struct damos *scheme;
> + unsigned long *sz_filter_passed;
> +};
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
> + unsigned long next, struct mm_walk *walk)
> +{
> + struct damos_va_stat_private *priv = walk->private;
> + struct damos *s = priv->scheme;
> + unsigned long *sz_filter_passed = priv->sz_filter_passed;
> + struct folio *folio;
> + spinlock_t *ptl;
> + pmd_t pmde;
> +
> + ptl = pmd_lock(walk->mm, pmd);
> + pmde = pmdp_get(pmd);
> +
> + if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
> + goto unlock;
> +
> + /* Tell page walk code to not split the PMD */
> + walk->action = ACTION_CONTINUE;
> +
> + folio = damon_get_folio(pmd_pfn(pmde));
> + if (!folio)
> + goto unlock;
> +
> + if (damon_invalid_damos_folio(folio, s))
> + goto update_last_applied;
> +
> + if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
> + *sz_filter_passed += folio_size(folio);
See my comment below regarding vm_normal_page and folio references.
But this split into two handlers is fairly odd. Usually we only have a
pmd_entry callback (see madvise_cold_or_pageout_pte_range as an
example), and handle !CONFIG_TRANSPARENT_HUGEPAGE in there.
Then, there is also no need to mess with ACTION_CONTINUE
> + }
> +
> + folio_put(folio);
> +update_last_applied:
> + s->last_applied = folio;
> +unlock:
> + spin_unlock(ptl);
> + return 0;
> +}
> +#else
> +#define damon_va_stat_pmd_entry NULL
> +#endif
> +
> +static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
> + unsigned long next, struct mm_walk *walk)
> +{
> + struct damos_va_stat_private *priv = walk->private;
> + struct damos *s = priv->scheme;
> + unsigned long *sz_filter_passed = priv->sz_filter_passed;
> + struct folio *folio;
> + pte_t ptent;
> +
> + ptent = ptep_get(pte);
> + if (pte_none(ptent) || !pte_present(ptent))
> + return 0;
> +
> + folio = damon_get_folio(pte_pfn(ptent));
> + if (!folio)
> + return 0;
We have vm_normal_folio() and friends for a reason -- so you don't have
to do pte_pfn() manually.
... and now I am confused. We are holding the PTL, so why would you have
to grab+put a folio reference here *at all*.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr
2025-07-29 14:11 ` David Hildenbrand
@ 2025-07-29 17:46 ` SeongJae Park
2025-07-30 22:24 ` Yueyang Pan
1 sibling, 0 replies; 10+ messages in thread
From: SeongJae Park @ 2025-07-29 17:46 UTC (permalink / raw)
To: David Hildenbrand
Cc: SeongJae Park, Yueyang Pan, Andrew Morton, Usama Arif, damon,
linux-mm, linux-kernel
Hi Pan and David, thank you for this patch and comments!
On Tue, 29 Jul 2025 16:11:32 +0200 David Hildenbrand <david@redhat.com> wrote:
> On 29.07.25 15:53, Yueyang Pan wrote:
> > From: PanJason <pyyjason@gmail.com>
> >
> > This patch adds support for damos_stat in virtual address space.
> > It leverages the walk_page_range to walk the page table and gets
> > the folio from page table. The last folio scanned is stored in
> > damos->last_applied to prevent double counting.
> > ---
> > mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 112 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index 87e825349bdf..3e319b51cfd4 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
> > return applied * PAGE_SIZE;
> > }
> >
> > +struct damos_va_stat_private {
> > + struct damos *scheme;
> > + unsigned long *sz_filter_passed;
> > +};
> > +
> > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > +static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
> > + unsigned long next, struct mm_walk *walk)
> > +{
> > + struct damos_va_stat_private *priv = walk->private;
> > + struct damos *s = priv->scheme;
> > + unsigned long *sz_filter_passed = priv->sz_filter_passed;
> > + struct folio *folio;
> > + spinlock_t *ptl;
> > + pmd_t pmde;
> > +
> > + ptl = pmd_lock(walk->mm, pmd);
> > + pmde = pmdp_get(pmd);
> > +
> > + if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
> > + goto unlock;
> > +
> > + /* Tell page walk code to not split the PMD */
> > + walk->action = ACTION_CONTINUE;
> > +
> > + folio = damon_get_folio(pmd_pfn(pmde));
> > + if (!folio)
> > + goto unlock;
> > +
> > + if (damon_invalid_damos_folio(folio, s))
> > + goto update_last_applied;
> > +
> > + if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
> > + *sz_filter_passed += folio_size(folio);
>
> See my comment below regarding vm_normal_page and folio references.
>
> But this split into two handlers is fairly odd. Usually we only have a
> pmd_entry callback (see madvise_cold_or_pageout_pte_range as an
> example), and handle !CONFIG_TRANSPARENT_HUGEPAGE in there.
>
> Then, there is also no need to mess with ACTION_CONTINUE
I don't really mind this, but I agree keeping the consisteency would be good.
Pan, could you please unify the handlers into one?
>
> > + }
> > +
> > + folio_put(folio);
> > +update_last_applied:
> > + s->last_applied = folio;
> > +unlock:
> > + spin_unlock(ptl);
> > + return 0;
> > +}
> > +#else
> > +#define damon_va_stat_pmd_entry NULL
> > +#endif
> > +
> > +static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
> > + unsigned long next, struct mm_walk *walk)
> > +{
> > + struct damos_va_stat_private *priv = walk->private;
> > + struct damos *s = priv->scheme;
> > + unsigned long *sz_filter_passed = priv->sz_filter_passed;
> > + struct folio *folio;
> > + pte_t ptent;
> > +
> > + ptent = ptep_get(pte);
> > + if (pte_none(ptent) || !pte_present(ptent))
> > + return 0;
> > +
> > + folio = damon_get_folio(pte_pfn(ptent));
> > + if (!folio)
> > + return 0;
>
> We have vm_normal_folio() and friends for a reason -- so you don't have
> to do pte_pfn() manually.
>
> ... and now I am confused. We are holding the PTL, so why would you have
> to grab+put a folio reference here *at all*.
We don't have to. I think Pan does so because other similar existing code in
this file is also doing so. I was doing so because I wanted to use the handy
damon_get_folio() and those are not making real problems.
But, yes, unnecessary things are unnecessary things. Pan, could you please use
vm_normal_folio() instead of damon_get_folio() and remove the related
folio_put() call?
I will also work on cleanup of existing unnecessary folio reference
manipulations, regardless of this patch series.
Thanks,
SJ
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr
2025-07-29 14:11 ` David Hildenbrand
2025-07-29 17:46 ` SeongJae Park
@ 2025-07-30 22:24 ` Yueyang Pan
1 sibling, 0 replies; 10+ messages in thread
From: Yueyang Pan @ 2025-07-30 22:24 UTC (permalink / raw)
To: David Hildenbrand
Cc: SeongJae Park, Andrew Morton, Usama Arif, damon, linux-mm,
linux-kernel
On Tue, Jul 29, 2025 at 04:11:32PM +0200, David Hildenbrand wrote:
> On 29.07.25 15:53, Yueyang Pan wrote:
> > From: PanJason <pyyjason@gmail.com>
> >
> > This patch adds support for damos_stat in virtual address space.
> > It leverages the walk_page_range to walk the page table and gets
> > the folio from page table. The last folio scanned is stored in
> > damos->last_applied to prevent double counting.
> > ---
> > mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 112 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index 87e825349bdf..3e319b51cfd4 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
> > return applied * PAGE_SIZE;
> > }
> > +struct damos_va_stat_private {
> > + struct damos *scheme;
> > + unsigned long *sz_filter_passed;
> > +};
> > +
> > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > +static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
> > + unsigned long next, struct mm_walk *walk)
> > +{
> > + struct damos_va_stat_private *priv = walk->private;
> > + struct damos *s = priv->scheme;
> > + unsigned long *sz_filter_passed = priv->sz_filter_passed;
> > + struct folio *folio;
> > + spinlock_t *ptl;
> > + pmd_t pmde;
> > +
> > + ptl = pmd_lock(walk->mm, pmd);
> > + pmde = pmdp_get(pmd);
> > +
> > + if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
> > + goto unlock;
> > +
> > + /* Tell page walk code to not split the PMD */
> > + walk->action = ACTION_CONTINUE;
> > +
> > + folio = damon_get_folio(pmd_pfn(pmde));
> > + if (!folio)
> > + goto unlock;
> > +
> > + if (damon_invalid_damos_folio(folio, s))
> > + goto update_last_applied;
> > +
> > + if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
> > + *sz_filter_passed += folio_size(folio);
>
> See my comment below regarding vm_normal_page and folio references.
>
> But this split into two handlers is fairly odd. Usually we only have a
> pmd_entry callback (see madvise_cold_or_pageout_pte_range as an example),
> and handle !CONFIG_TRANSPARENT_HUGEPAGE in there.
>
> Then, there is also no need to mess with ACTION_CONTINUE
>
Hi David. Thanks for your comment. I was not aware of the convention so I
followed the existing code. I had a discussion with SJ today and in the
next version I will change this by combining *pmd_entry() and *pte_entry().
We will also format the existing code in future patches.
> > + }
> > +
> > + folio_put(folio);
> > +update_last_applied:
> > + s->last_applied = folio;
> > +unlock:
> > + spin_unlock(ptl);
> > + return 0;
> > +}
> > +#else
> > +#define damon_va_stat_pmd_entry NULL
> > +#endif
> > +
> > +static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
> > + unsigned long next, struct mm_walk *walk)
> > +{
> > + struct damos_va_stat_private *priv = walk->private;
> > + struct damos *s = priv->scheme;
> > + unsigned long *sz_filter_passed = priv->sz_filter_passed;
> > + struct folio *folio;
> > + pte_t ptent;
> > +
> > + ptent = ptep_get(pte);
> > + if (pte_none(ptent) || !pte_present(ptent))
> > + return 0;
> > +
> > + folio = damon_get_folio(pte_pfn(ptent));
> > + if (!folio)
> > + return 0;
>
> We have vm_normal_folio() and friends for a reason -- so you don't have to
> do pte_pfn() manually.
>
> ... and now I am confused. We are holding the PTL, so why would you have to
> grab+put a folio reference here *at all*.
>
Thanks for pointing out. I thought someone could still change the folio and
realized it could not happen with PTL. I will use vm_normal_folio* in the
next version.
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr
2025-07-29 13:53 ` [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
2025-07-29 14:11 ` David Hildenbrand
@ 2025-07-29 18:15 ` SeongJae Park
2025-07-29 18:21 ` SeongJae Park
2 siblings, 0 replies; 10+ messages in thread
From: SeongJae Park @ 2025-07-29 18:15 UTC (permalink / raw)
To: Yueyang Pan
Cc: SeongJae Park, Andrew Morton, Usama Arif, damon, linux-mm,
linux-kernel
On Tue, 29 Jul 2025 06:53:30 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:
> From: PanJason <pyyjason@gmail.com>
>
> This patch adds support for damos_stat in virtual address space.
> It leverages the walk_page_range to walk the page table and gets
> the folio from page table. The last folio scanned is stored in
> damos->last_applied to prevent double counting.
Thank you for this patch, Pan! I left a few comments below. I think those are
mostly insignificant change requests, though.
> ---
> mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 87e825349bdf..3e319b51cfd4 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
> return applied * PAGE_SIZE;
> }
>
> +struct damos_va_stat_private {
> + struct damos *scheme;
> + unsigned long *sz_filter_passed;
> +};
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
> + unsigned long next, struct mm_walk *walk)
> +{
> + struct damos_va_stat_private *priv = walk->private;
> + struct damos *s = priv->scheme;
> + unsigned long *sz_filter_passed = priv->sz_filter_passed;
> + struct folio *folio;
> + spinlock_t *ptl;
> + pmd_t pmde;
> +
> + ptl = pmd_lock(walk->mm, pmd);
> + pmde = pmdp_get(pmd);
> +
> + if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
> + goto unlock;
> +
> + /* Tell page walk code to not split the PMD */
> + walk->action = ACTION_CONTINUE;
As David suggested, let's unify this with pte handler following the pattern of
madvise_cold_or_pageout_pte_range() and drop above ACTION_CONTINUE code, unless
you have different opinions.
> +
> + folio = damon_get_folio(pmd_pfn(pmde));
As also David suggested, let's use vm_normal_folio_pmd() instead, and drop
unnecessary folio_put().
> + if (!folio)
> + goto unlock;
damon_invalid_damos_folio() returns true if folio is NULL, so I think above
check is unnecessary.
> +
> + if (damon_invalid_damos_folio(folio, s))
> + goto update_last_applied;
Because we didn't really apply the DAMOS action, I think it is more proper to
goto 'unlock' directly.
Oh, and I now realize damon_invalid_damos_folio() puts the folio for none-NULL
invalid folio...
Because the code is simple, let's implement and use 'va' version
invalid_damos_folio(), say, damon_va_invalid_damos_folio(), which doesn't put
the folio.
> +
> + if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
> + *sz_filter_passed += folio_size(folio);
> + }
Let's remove braces for single statement, as suggested[1] by the coding style.
> +
> + folio_put(folio);
> +update_last_applied:
> + s->last_applied = folio;
> +unlock:
> + spin_unlock(ptl);
> + return 0;
> +}
> +#else
> +#define damon_va_stat_pmd_entry NULL
> +#endif
> +
> +static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
> + unsigned long next, struct mm_walk *walk)
> +{
> + struct damos_va_stat_private *priv = walk->private;
> + struct damos *s = priv->scheme;
> + unsigned long *sz_filter_passed = priv->sz_filter_passed;
> + struct folio *folio;
> + pte_t ptent;
> +
> + ptent = ptep_get(pte);
> + if (pte_none(ptent) || !pte_present(ptent))
> + return 0;
> +
> + folio = damon_get_folio(pte_pfn(ptent));
As David suggested, let's use vm_normal_folio() here, and remove below
folio_put().
> + if (!folio)
> + return 0;
As also mentioned above, let's drop above NULL case check, in favor of that in
damon_va_invalid_damos_folio().
> +
> + if (damon_invalid_damos_folio(folio, s))
> + goto update_last_applied;
Again, I don't think we need to update s->last_applied in this case. Let's
do only necessary cleanups and return.
> +
> + if (!damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL)){
> + *sz_filter_passed += folio_size(folio);
> + }
Let's drop braces for single statement[1].
> +
> + folio_put(folio);
> +
> +update_last_applied:
> + s->last_applied = folio;
> + return 0;
> +}
> +
> +static unsigned long damos_va_stat(struct damon_target *target,
> + struct damon_region *r, struct damos *s,
> + unsigned long *sz_filter_passed)
> +{
> +
Let's remove this unnecessary blank line.
> + struct damos_va_stat_private priv;
> + struct mm_struct *mm;
> + struct mm_walk_ops walk_ops = {
> + .pmd_entry = damos_va_stat_pmd_entry,
> + .pte_entry = damos_va_stat_pte_entry,
> + .walk_lock = PGWALK_RDLOCK,
> + };
> +
> + priv.scheme = s;
> + priv.sz_filter_passed = sz_filter_passed;
> +
> + if (!damon_scheme_has_filter(s)){
> + return 0;
> + }
Let's remove braces for single statement[1].
> +
> + mm = damon_get_mm(target);
> + if (!mm)
> + return 0;
> +
> + mmap_read_lock(mm);
> + walk_page_range(mm, r->ar.start, r->ar.end, &walk_ops, &priv);
> + mmap_read_unlock(mm);
> + mmput(mm);
> + pr_debug("Call va_stat: %lu\n", *sz_filter_passed);
I don't think we really need this debug log. Can we remove?
> + return 0;
> +
Yet another unnecessary blank line. Let's remove.
> +}
> +
> static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
> struct damon_target *t, struct damon_region *r,
> struct damos *scheme, unsigned long *sz_filter_passed)
> @@ -916,7 +1027,7 @@ static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
> case DAMOS_MIGRATE_COLD:
> return damos_va_migrate(t, r, scheme, sz_filter_passed);
> case DAMOS_STAT:
> - return 0;
> + return damos_va_stat(t, r, scheme, sz_filter_passed);
> default:
> /*
> * DAMOS actions that are not yet supported by 'vaddr'.
> --
> 2.47.3
[1] https://docs.kernel.org/process/coding-style.html
Thanks,
SJ
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr
2025-07-29 13:53 ` [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
2025-07-29 14:11 ` David Hildenbrand
2025-07-29 18:15 ` SeongJae Park
@ 2025-07-29 18:21 ` SeongJae Park
2 siblings, 0 replies; 10+ messages in thread
From: SeongJae Park @ 2025-07-29 18:21 UTC (permalink / raw)
To: Yueyang Pan
Cc: SeongJae Park, Andrew Morton, Usama Arif, damon, linux-mm,
linux-kernel
On Tue, 29 Jul 2025 06:53:30 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:
> From: PanJason <pyyjason@gmail.com>
>
> This patch adds support for damos_stat in virtual address space.
> It leverages the walk_page_range to walk the page table and gets
> the folio from page table. The last folio scanned is stored in
> damos->last_applied to prevent double counting.
Please add your Singed-off-by: tag[1] here.
[1] https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1
Thanks,
SJ
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/2] mm/damon: Add damos_stat support for vaddr
2025-07-29 13:53 [PATCH v1 0/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
2025-07-29 13:53 ` [PATCH v1 1/2] mm/damon: Move invalid folio and has filter to ops-common Yueyang Pan
2025-07-29 13:53 ` [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
@ 2025-07-29 18:34 ` SeongJae Park
2 siblings, 0 replies; 10+ messages in thread
From: SeongJae Park @ 2025-07-29 18:34 UTC (permalink / raw)
To: Yueyang Pan
Cc: SeongJae Park, Andrew Morton, Usama Arif, damon, linux-mm,
linux-kernel
Hello Pan (Or, should I call you Yueyang or Jason? Please let me know your
preferrence if you have),
On Tue, 29 Jul 2025 06:53:28 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:
> From: PanJason <pyyjason@gmail.com>
>
> Previously damos_stat only supoort paddr. This patch set adds support
> for damos_stat for vaddr. Also all different types of filters are
> supported.
>
> Functionality Test
> ==================
> I wrote a small test program which allocates 10GB of DRAM, use
> madvise(MADV_HUGEPAGE) to convert the base pages to 2MB huge pages
> Then my program does the following things in order:
> 1. Write sequentially to the whole 10GB region
> 2. Read the first 5GB region sequentially for 10 times
> 3. Sleep 5s
> 4. Read the second 5GB region sequentially for 10 times
>
> With a proper damon setting, we are expected to see df-passed to be 10GB
> and hot region move around with the read
>
> $ # Start damon record
> $sudo ./damo/damo record "./my_test/test" --monitoring_intervals 100000\
> 1000000 2000000
You can use 'start' instead of 'record' for this test purpose.
--monitoring_intervals receive more human-friendly format, so you can do
'--monitoring_intervals 100ms 1s 2s'.
>
> $ # damon report
> $sudo ./damo/damo report access --snapshot_damos_filter reject none \
> hugepage_size 2MiB 2MiB
The --snapshot_damos_filter option means you want to make folios that not
having size 2 MiB not passed by the filter. You can ask same thing in more
intuitive way, like below.
--snapshot_damos_filter allow hugepage_size 2MiB 2MiB
> heatmap:
> # min/max temperatures: -900,000,000, 100,002,000, column size: 136.564 MiB
checkpatch.pl gives me below warning:
WARNING: Commit log lines starting with '#' are dropped by git as comments
I'd suggest adding four spaces prefix to quoted command outputs like this.
> intervals: sample 100 ms aggr 1 s (max access hz 10)
> # damos filters (df): reject none hugepage_size [2.000 MiB, 2.000 MiB]
> df-pass:
> # min/max temperatures: -663,075,528, 100,002,000, column size: 128.037 MiB
> 0 addr 86.082 TiB size 682.039 MiB access 0 hz age 9 s df-passed 0 B
> 1 addr 127.225 TiB size 466.039 MiB access 1.000 hz age 0 ns df-passed 468.000 MiB
[...]
> memory bw estimate: 3.615 GiB per second df-passed: 3.615 GiB per second
> total size: 10.669 GiB df-passed 10.000 GiB
> record DAMON intervals: sample 100 ms, aggr 1 s
>
> $ # damon report again
> $sudo ./damo/damo report access --snapshot_damos_filter reject none \
> hugepage_size 2MiB 2MiB
> heatmap:
> # min/max temperatures: -1,100,000,000, 300,001,000, column size: 136.564 MiB
> intervals: sample 100 ms aggr 1 s (max access hz 10)
> # damos filters (df): reject none hugepage_size [2.000 MiB, 2.000 MiB]
> df-pass:
> # min/max temperatures: -800,000,000, 300,001,000, column size: 128.037 MiB
> 0 addr 86.082 TiB size 682.039 MiB access 0 hz age 11 s df-passed 0 B
> 1 addr 127.225 TiB size 10.355 MiB access 1.000 hz age 0 ns df-passed 12.000 MiB
> 2 addr 127.225 TiB size 93.207 MiB access 1.000 hz age 0 ns df-passed 92.000 MiB
> 3 addr 127.225 TiB size 414.262 MiB access 1.000 hz age 0 ns df-passed 414.000 MiB
> 4 addr 127.225 TiB size 706.695 MiB access 1.000 hz age 3 s df-passed 708.000 MiB
> 5 addr 127.226 TiB size 78.523 MiB access 1.000 hz age 3 s df-passed 78.000 MiB
[...]
> total size: 10.669 GiB df-passed 10.000 GiB
> record DAMON intervals: sample 100 ms, aggr 1 s
>
> As you can see the total df-passed region is 10GiB and the hot region
> moves as the seq read keeps going
>
> PanJason (2):
> mm/damon: Move invalid folio and has filter to ops-common
> mm/damon: Add damos_stat support for vaddr
The changes look good overall, though I left a few comments for formatting and
unnecessary folio references that inherited from my original sin. Looking
forward to more discussions and next version of this great patch series!
Thanks,
SJ
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread