mm/damon: fix DAMOS bugs in core, paddr and vaddr

Fix misc bugs of DAMOS. Patch 1 makes DAMOS less stress memory allocator under extreme situation. Patches 2 and 3 fix wrong folios walking in DAMON_PADDR. Patches 4 and 5 fix wrong folios walking in DAMON_VADDR. Patches 6-8 handle extreme and unlikely memory situations that can cause divide by zero and underflow.

All bugs are discovered by Sashiko.

The bugs are not very critical, but better to be merged sooner than later. Since mm.git is closed for urgent changes, I aim for 7.3-rcX. I will drop the RFC tag after 7.3-rc1.

Changes from RFC v1.1
- RFC v1.1: https://lore.kernel.org/[email protected]
- Fix wrong commit message (patch 7).
- Return 100% and 0% for used and free memory (patches 7 and 8).
- Drop incomplete last_psi_total initialization (patch 2).
- Rebase to latest mm-new.
Changes from RFC
- RFC: https://lore.kernel.org/[email protected]
- Use cached pte content (patch 5 and 6).
- Handle totalram < freeram case (patch 7).
- Fix wrong function name in commit message (patch 9).

SJ Park (8):
mm/damon/core: skip applying scheme if region split for quota fails mm/damon/paddr: respect folio end for DAMOS_STAT mm/damon/paddr: respect folio end for DAMOS actions except STAT mm/damon/vaddr: respect folio end for DAMOS_STAT mm/damon/vaddr: respect folio end for DAMOS_MIGRATE_{HOT,COLD} mm/damon/core: handle extreme memory state in damon_get_node_mem_bp() mm/damon/core: handle extreme memory state in get_node_memcg_used_bp() mm/damon/core: handle extreme memory state in get_in_active_mem_bp()

 mm/damon/core.c  | 18 ++++++++++++++++--
 mm/damon/paddr.c |  8 ++++----
 mm/damon/vaddr.c | 10 ++++++++--
 3 files changed, 28 insertions(+), 8 deletions(-)

base-commit: 59a8d789d9478797978640a7a640b086ad400b37

2.47.3

damos_apply_scheme() splits a region and apply the action to the subregion if it is needed for not violating the quota. The split operation (damon_split_region_at()) could fail for allocation failure. In the case, the quota could be violated. From the user's perspective, DAMOS becomes more aggressive than expected under the extreme situation. Handle the failure.

The user impact is not critical. The failure of damon_split_region_at() is unlikely since it is arguably too small to fail. Also DAMOS being aggressive is limited to the single region. Users can set min_nr_regions to set the maximum size of each region. If it is reasonably set, the transient overhead shouldn't be critical.

The issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/[email protected]

---
 mm/damon/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 644daf5a16560..e2900d0c984c9 100644
@@ -2613,7 +2613,8 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 					c->min_region_sz);
 			if (!sz)
 				goto update_stat;
-			damon_split_region_at(t, r, sz);
+			if (damon_split_region_at(t, r, sz))
+				goto update_stat;
 		}
 		if (damos_core_filter_out(c, t, r, s))
 			return;

2.47.3

The function for applying DAMOS_STAT in DAMON physical address space operation set (paddr), namely damon_pa_stat(), applies DAMOS filters to folios of the given region. For that, it gets folios of addresses in the region. It starts from the region start address and advances the address by the size of the folio of the address until it goes out of the region. If the start address is in the middle of a large folio, and if the next folios are small, some of the next folios could be skipped. Fix the issue by advancing the address to exactly the start address of the next folio.

The user impact is that the DAMOS_STAT-based page level monitoring results become inaccurate. Since the page level monitoring is supposed to provide relatively high precision, this is definitely a problem. It is arguably not critical since it is only monitoring quality degradation.

---
 mm/damon/paddr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5c6c3a597fd0b..2ab7b3842701e 100644
@@ -379,7 +379,7 @@ static unsigned long damon_pa_stat(struct damon_region *r,
 
 		if (!damos_pa_filter_out(s, folio))
 			*sz_filter_passed += folio_size(folio) / addr_unit;
-		addr += folio_size(folio);
+		addr = PFN_PHYS(folio_pfn(folio)) + folio_size(folio);
 		folio_put(folio);
 	}
 	s->last_applied = folio;

2.47.3

A few functions for applying DAMOS actions including pageout, lru_[de]prio and migrate_{hot,cold} in DAMON physical address space operation set (paddr) collect folios of the given region by getting the folios of region-internal addresses. Then, those functions apply the action to the collected folios at once. The collection starts from the region start address and advances the address by the size of the folio of the address until it goes out of the region. If the start address is in the middle of a large folio, and if the next folios are small, some of the next folios could be skipped. Fix the issue by advancing the address to exactly the start address of the next folio.

The user impact is that DAMOS action is applied to less than expected amount of memory. Given the best effort nature of DAMON, it is no big problem, but it is clearly a bug that is better to be fixed.

The issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/[email protected]

---
 mm/damon/paddr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 2ab7b3842701e..9ddd1ec8202b7 100644
@@ -264,7 +264,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r,
 		else
 			list_add(&folio->lru, &folio_list);
 put_folio:
-		addr += folio_size(folio);
+		addr = PFN_PHYS(folio_pfn(folio)) + folio_size(folio);
 		folio_put(folio);
 	}
 	if (install_young_filter)

@@ -302,7 +302,7 @@ static inline unsigned long damon_pa_de_activate(
 			folio_deactivate(folio);
 		applied += folio_nr_pages(folio);
 put_folio:
-		addr += folio_size(folio);
+		addr = PFN_PHYS(folio_pfn(folio)) + folio_size(folio);
 		folio_put(folio);
 	}
 	s->last_applied = folio;

@@ -350,7 +350,7 @@ static unsigned long damon_pa_migrate(struct damon_region *r,
 				folio_is_file_lru(folio));
 		list_add(&folio->lru, &folio_list);
 put_folio:
-		addr += folio_size(folio);
+		addr = PFN_PHYS(folio_pfn(folio)) + folio_size(folio);
 		folio_put(folio);
 	}
 	applied = damon_migrate_pages(&folio_list, s->target_nid);

2.47.3

For applying DAMOS_STAT action to a region, DAMON virtual address space operation set (vaddr) calls walk_page_range[_vma]() for the region. The pmd walk entry function, namely damon_va_stat_pmd_entry(), applies DAMOS filters to folios of addresses of the region in the pmd. It starts from the walking address and advances the address by the size of the folio of the address until it goes out of the pmd or the region.

Let's suppose it is for the first pmd of the region, and the region start address is in the middle of a large folio. Also, the next folios are small. Then, some of the next folios could be skipped. Fix the issue by advancing the address to exactly the start address of the next folio.

The user impact is that the DAMOS_STAT-based page level monitoring results become inaccurate. Since the page level monitoring is supposed to provide relatively high precision, this is definitely a problem. It is arguably not critical since it is only monitoring quality degradation.

The issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/[email protected]

---
 mm/damon/vaddr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 0648400b2d65b..4b0b5edf67952 100644
@@ -831,6 +831,8 @@ static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
 		return 0;
 
 	for (; addr < next; pte += nr, addr += nr * PAGE_SIZE) {
+		unsigned long page_idx;
+
 		nr = 1;
 		ptent = ptep_get(pte);
 

@@ -844,7 +846,8 @@ static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
 
 		if (!damos_va_filter_out(s, folio, vma, addr, pte, NULL))
 			*sz_filter_passed += folio_size(folio);
-		nr = folio_nr_pages(folio);
+		page_idx = folio_page_idx(folio, pte_page(ptent));
+		nr = folio_nr_pages(folio) - page_idx;
 		s->last_applied = folio;
 	}
 	pte_unmap_unlock(start_pte, ptl);

2.47.3

For applying DAMOS_MIGRATE_{HOT,COLD} actions to a region, DAMON virtual address space operation set (vaddr) calls walk_page_range[_vma]() for the region. The pmd walk entry function, namely damon_va_migrate_pmd_entry(), collects folios of addresses of the region in the pmd. It starts from the walking address and advances the address by the size of the folio of the address until it goes out of the pmd or the region.

Let's suppose it is for the first pmd of the region, and the region start address is in the middle of a large folio. Also, the next folios are small. Then, some of the next folios could be skipped. Fix the issue by advancing the address to exactly the start address of the next folio.

The user impact is that DAMOS action is applied to less than expected amount of memory. Given the best effort nature of DAMON, it is no big problem, but it is clearly a bug that is better to be fixed.

The issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/[email protected]

---
 mm/damon/vaddr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 4b0b5edf67952..c8c32b2ae0402 100644
@@ -669,6 +669,8 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
 		return 0;
 
 	for (; addr < next; pte += nr, addr += nr * PAGE_SIZE) {
+		unsigned long page_idx;
+
 		nr = 1;
 		ptent = ptep_get(pte);
 

@@ -681,7 +683,8 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
 			continue;
 		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
 				migration_lists);
-		nr = folio_nr_pages(folio);
+		page_idx = folio_page_idx(folio, pte_page(ptent));
+		nr = folio_nr_pages(folio) - page_idx;
 	}
 	pte_unmap_unlock(start_pte, ptl);
 	return 0;

2.47.3

In an extreme and unlikely situation, si_meminfo_node() might let the caller show zero total ram. That could cause a divide by zero in damon_get_node_mem_bp(). It could also show free memory larger than the total memory. This could cause underflow and make DAMOS temporarily make unexpected behavior. Thanks to safety guards in the auto-tuning feedback loop, that should not be a real problem, though. Fix the problems by respectively returning 100% and 0% for used and free memory queries in the corner cases.

The issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/[email protected]

---
 mm/damon/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

index e2900d0c984c9..75d71ac09515e 100644
@@ -2816,6 +2816,13 @@ static __kernel_ulong_t damos_get_node_mem_bp(
 	}
 
 	si_meminfo_node(&i, goal->nid);
+	if (!i.totalram || i.totalram < i.freeram) {
+		if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
+			return 10000;
+		else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
+			return 0;
+	}
+
 	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
 		numerator = i.totalram - i.freeram;
 	else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */

2.47.3

In extreme unlikely situations, total memory might be zero. In less extreme but still very unlikely situations, lruvec_page_state() calls might let the caller show used memory larger than total memory. In the two cases, damos_get_node_memcg_used_bp() could cause division by zero, or return underflowed value, respectively. Handle the cases by respectively returning 100% and 0% for used and free memory queries in the corner cases.

This issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/[email protected]

---
 mm/damon/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 75d71ac09515e..3779a04753061 100644
@@ -2863,6 +2863,12 @@ static unsigned long damos_get_node_memcg_used_bp(
 	mem_cgroup_put(memcg);
 
 	si_meminfo_node(&i, goal->nid);
+	if (!i.totalram || i.totalram < used_pages) {
+		if (goal->metric == DAMOS_QUOTA_NODE_MEMCG_USED_BP)
+			return 10000;
+		else	/* DAMOS_QUOTA_NODE_MEMCG_FREE_BP */
+			return 0;
+	}
 	if (goal->metric == DAMOS_QUOTA_NODE_MEMCG_USED_BP)
 		numerator = used_pages;
 	else	/* DAMOS_QUOTA_NODE_MEMCG_FREE_BP */

2.47.3

There could be different perspective, but I think that's fine.

Anyway it is a corner case. And even in this case, quota will be changed only up to 2x. So no real problem in my opinion. We can revisit if we find a real issue from this in real usage.

Thanks,
SJ

damos_get_in_active_mem_bp() uses the sum of the active and inactive memory amount as a denominator. In an extreme and unlikely environment, active and inactive memory might be zero. In this case, hence, it results in a divide by zero problem. Avoid it by changing the denominator to one if it is zero, before it is being used.

The issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/[email protected]

---
 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 3779a04753061..f8dddbff74a77 100644
@@ -3015,7 +3015,7 @@ static unsigned int damos_get_in_active_mem_bp(bool active_ratio)
 		global_node_page_state(NR_LRU_BASE + LRU_ACTIVE_FILE);
 	inactive = global_node_page_state(NR_LRU_BASE + LRU_INACTIVE_ANON) +
 		global_node_page_state(NR_LRU_BASE + LRU_INACTIVE_FILE);
-	total = active + inactive;
+	total = max(active + inactive, 1);
 	if (active_ratio)
 		return mult_frac(active, 10000, total);
 	return mult_frac(inactive, 10000, total);

2.47.3

Known low priority issue.

Thanks,
SJ

Sashiko found no blocker for this series. Sashiko sent findings to damon@ mailing list [1], and I replied to all the comments. Please read those for details.

[1] https://lore.kernel.org/damon/

Thanks,
SJ

[...]