diff mbox series

[v4,2/6] mm/damon/paddr: use damon_get_folio_in_region to obtain folio

Message ID 20250203225604.44742-3-usamaarif642@gmail.com (mailing list archive)
State New
Headers show
Series mm/damon: add support for hugepages | expand

Commit Message

Usama Arif Feb. 3, 2025, 10:55 p.m. UTC 
This is introduced for larger folios. If a large folio has subpages
present in multiple regions, it will be considered multiple times.
This can be when checking access or applying DAMOS schemes. For e.g.
in pa_stat, folios split across N regions will be counted N times,
giving inaccurate results. Hence, only consider a page for access
check/DAMOS scheme only if the head page is part of that region as
well.

Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
 mm/damon/paddr.c | 44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

Comments

SeongJae Park Feb. 4, 2025, 11:06 p.m. UTC | #1 
On Mon,  3 Feb 2025 22:55:29 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> This is introduced for larger folios. If a large folio has subpages
> present in multiple regions, it will be considered multiple times.
> This can be when checking access

I think this behavior makes sense.  If a 2 MiB address range is known to be
accessed or not, treating DAMON regions in the address range as all accessed or
not aligns with the DAMON's region definition and makes sense in general, to
me.  Note that DAMON's adaptive regions adjustment mechanism will make the
DAMON regions in a large folio to be merged in near future.

> or applying DAMOS schemes. For e.g.
> in pa_stat, folios split across N regions will be counted N times,
> giving inaccurate results.

Nice catch!  I agree this could be a problem.

> Hence, only consider a page for access check/DAMOS scheme only if the head
> page is part of that region as well.

For access checking, this seems wrong to me.  This change will unnecessarily
mark some DAMON regions as not accessed.

Even for DAMOS, this seems not very optimum.

1. DAMOS will unnecessarily repeat PAGE_SIZE skippings on second and later
   DAMON regions of a same large folio.
2. If only a small part of the large folio belongs to the first DAMON region,
   and the DAMON region is not eligible to the scheme, while the second region
   is, the scheme action will not applied to the second region.

Also, I think this problem can happen on vaddr case.  Hence, making the change
on core layer makes sense to me.

That is, let damon_ops->apply_scheme() inform the caller (damos_apply_scheme())
how much bytes of the next region should be ignored at applying the action.
Due to the complicated implementation of DAMOS core logic, this might be not
very simple.

I think the additional complexity might outweigh the benefit for following
reasons.  First, adaptive regions adjustment mechanism will make DAMON regions
in same large folios to be merged soon.  So the problem will be not significant
in common case.  Second, any threads could collapse any parts of memory into
large folios while DAMOS is traversing DAMON regions.  So this problem cannot
perfectly be solved unless we make DAMOS' DAMON regions traversal exclusive
with any large folios collapsing.  Which would add more complexity.

And given DAMON's best-effort nature, keeping the issue until we get a simple
and effective solution is ok to me, unless a real significant issue from this
is confirmed.

Usama, what do you think?


Thanks,
SJ

[...]
diff mbox series

Patch

diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 0fb61f6ddb8d..3f59a3fdc391 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -19,6 +19,30 @@ 
 #include "../internal.h"
 #include "ops-common.h"
 
+/*
+ * Get an online page for a paddr if it's in the LRU list and if the head page is
+ * before region_start. Otherwise, returns NULL.
+ */
+static struct folio *damon_get_folio_in_region(unsigned long addr, unsigned long region_start)
+{
+	struct page *page = pfn_to_online_page(PHYS_PFN(addr));
+	struct folio *folio;
+
+	if (!page)
+		return NULL;
+
+	folio = page_folio(page);
+	if (addr - folio_page_idx(folio, page) * PAGE_SIZE < region_start)
+		return NULL;
+	if (!folio_test_lru(folio) || !folio_try_get(folio))
+		return NULL;
+	if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
+		folio_put(folio);
+		folio = NULL;
+	}
+	return folio;
+}
+
 static bool damon_folio_mkold_one(struct folio *folio,
 		struct vm_area_struct *vma, unsigned long addr, void *arg)
 {
@@ -58,9 +82,9 @@  static void damon_folio_mkold(struct folio *folio)
 
 }
 
-static void damon_pa_mkold(unsigned long paddr)
+static void damon_pa_mkold(unsigned long paddr, unsigned long region_start)
 {
-	struct folio *folio = damon_get_folio(PHYS_PFN(paddr));
+	struct folio *folio = damon_get_folio_in_region(paddr, region_start);
 
 	if (!folio)
 		return;
@@ -73,7 +97,7 @@  static void __damon_pa_prepare_access_check(struct damon_region *r)
 {
 	r->sampling_addr = damon_rand(r->ar.start, r->ar.end);
 
-	damon_pa_mkold(r->sampling_addr);
+	damon_pa_mkold(r->sampling_addr, r->ar.start);
 }
 
 static void damon_pa_prepare_access_checks(struct damon_ctx *ctx)
@@ -148,9 +172,9 @@  static bool damon_folio_young(struct folio *folio)
 	return accessed;
 }
 
-static bool damon_pa_young(unsigned long paddr, unsigned long *folio_sz)
+static bool damon_pa_young(unsigned long paddr, unsigned long *folio_sz, unsigned long region_start)
 {
-	struct folio *folio = damon_get_folio(PHYS_PFN(paddr));
+	struct folio *folio = damon_get_folio_in_region(paddr, region_start);
 	bool accessed;
 
 	if (!folio)
@@ -176,7 +200,7 @@  static void __damon_pa_check_access(struct damon_region *r,
 		return;
 	}
 
-	last_accessed = damon_pa_young(r->sampling_addr, &last_folio_sz);
+	last_accessed = damon_pa_young(r->sampling_addr, &last_folio_sz, r->ar.start);
 	damon_update_region_access_rate(r, last_accessed, attrs);
 
 	last_addr = r->sampling_addr;
@@ -268,7 +292,7 @@  static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
-		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+		struct folio *folio = damon_get_folio_in_region(addr, r->ar.start);
 
 		if (!folio) {
 			addr += PAGE_SIZE;
@@ -307,7 +331,7 @@  static inline unsigned long damon_pa_mark_accessed_or_deactivate(
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
-		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+		struct folio *folio = damon_get_folio_in_region(addr, r->ar.start);
 
 		if (!folio) {
 			addr += PAGE_SIZE;
@@ -474,7 +498,7 @@  static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
-		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+		struct folio *folio = damon_get_folio_in_region(addr, r->ar.start);
 
 		if (!folio) {
 			addr += PAGE_SIZE;
@@ -518,7 +542,7 @@  static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s,
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
-		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+		struct folio *folio = damon_get_folio_in_region(addr, r->ar.start);
 
 		if (!folio) {
 			addr += PAGE_SIZE;