mm/damon/sysfs: fix wrong empty schemes assumption under online tuning in damon_sysfs_set_schemes()

PATCH Applied SeongJae Park 1 participant 3 messages Nov 22, 2022

Subsystems (3)

Stable branch Memory management DAMON

lore

Share & tools

lore

SeongJae Park

author maintainer

2022-11-22 19:48 UTC #0 lore raw

PATCH 1/1 mm/damon/sysfs: fix wrong empty schemes assumption under online tuning in damon_sysfs_set_schemes()

Commit da87878010e5 ("mm/damon/sysfs: support online inputs update") made 'damon_sysfs_set_schemes()' to be called for running DAMON context, which could have schemes. In the case, DAMON sysfs interface is supposed to update, remove, or add schemes to reflect the sysfs files. However, the code is assuming the DAMON context wouldn't have schemes at all, and therefore creates and adds new schemes. As a result, the code doesn't work as intended for online schemes tuning and could have more
than expected memory footprint. The schemes are all in the DAMON
context, so it doesn't leak the memory, though.

Remove the wrong asssumption (the DAMON context wouldn't have schemes) in 'damon_sysfs_set_schemes()' to fix the bug.

Fixes da87878010e5 ("mm/damon/sysfs: support online inputs update")

Cc <[email protected]> # 5.19.y

Signed-off-by SeongJae Park <[email protected]>

---
 mm/damon/sysfs.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

mm/damon/sysfs.c +44 -2

index 5ce403378c20..07e5f1bdf025 100644
@@ -2283,12 +2283,54 @@ static struct damos *damon_sysfs_mk_scheme(
 			&wmarks);
 }
 
+static void damon_sysfs_update_scheme(struct damos *scheme,
+		struct damon_sysfs_scheme *sysfs_scheme)
+{
+	struct damon_sysfs_access_pattern *access_pattern =
+		sysfs_scheme->access_pattern;
+	struct damon_sysfs_quotas *sysfs_quotas = sysfs_scheme->quotas;
+	struct damon_sysfs_weights *sysfs_weights = sysfs_quotas->weights;
+	struct damon_sysfs_watermarks *sysfs_wmarks = sysfs_scheme->watermarks;
+
+	scheme->pattern.min_sz_region = access_pattern->sz->min;
+	scheme->pattern.max_sz_region = access_pattern->sz->max;
+	scheme->pattern.min_nr_accesses = access_pattern->nr_accesses->min;
+	scheme->pattern.max_nr_accesses = access_pattern->nr_accesses->max;
+	scheme->pattern.min_age_region = access_pattern->age->min;
+	scheme->pattern.max_age_region = access_pattern->age->max;
+
+	scheme->action = sysfs_scheme->action;
+
+	scheme->quota.ms = sysfs_quotas->ms;
+	scheme->quota.sz = sysfs_quotas->sz;
+	scheme->quota.reset_interval = sysfs_quotas->reset_interval_ms;
+	scheme->quota.weight_sz = sysfs_weights->sz;
+	scheme->quota.weight_nr_accesses = sysfs_weights->nr_accesses;
+	scheme->quota.weight_age = sysfs_weights->age;
+
+	scheme->wmarks.metric = sysfs_wmarks->metric;
+	scheme->wmarks.interval = sysfs_wmarks->interval_us;
+	scheme->wmarks.high = sysfs_wmarks->high;
+	scheme->wmarks.mid = sysfs_wmarks->mid;
+	scheme->wmarks.low = sysfs_wmarks->low;
+}
+
 static int damon_sysfs_set_schemes(struct damon_ctx *ctx,
 		struct damon_sysfs_schemes *sysfs_schemes)
 {
-	int i;
+	struct damos *scheme, *next;
+	int i = 0;
+
+	damon_for_each_scheme_safe(scheme, next, ctx) {
+		if (i < sysfs_schemes->nr)
+			damon_sysfs_update_scheme(scheme,
+					sysfs_schemes->schemes_arr[i]);
+		else
+			damon_destroy_scheme(scheme);
+		i++;
+	}
 
-	for (i = 0; i < sysfs_schemes->nr; i++) {
+	for (; i < sysfs_schemes->nr; i++) {
 		struct damos *scheme, *next;
 
 		scheme = damon_sysfs_mk_scheme(sysfs_schemes->schemes_arr[i]);

Signature · 1 line

2.25.1

SeongJae Park

author maintainer

2022-11-22 19:53 UTC #1 lore raw

Hi Andrew,

Quoted 10 lines · On Tue, 22 Nov 2022 19:48:31 +0000 SeongJae Park <[email protected]> wrote:

Commit da87878010e5 ("mm/damon/sysfs: support online inputs update") made 'damon_sysfs_set_schemes()' to be called for running DAMON context, which could have schemes. In the case, DAMON sysfs interface is supposed to update, remove, or add schemes to reflect the sysfs files. However, the code is assuming the DAMON context wouldn't have schemes at all, and therefore creates and adds new schemes. As a result, the code doesn't work as intended for online schemes tuning and could have more
than expected memory footprint. The schemes are all in the DAMON
context, so it doesn't leak the memory, though.

Quoted 6 lines

Remove the wrong asssumption (the DAMON context wouldn't have schemes) in 'damon_sysfs_set_schemes()' to fix the bug.
Fixes da87878010e5 ("mm/damon/sysfs: support online inputs update")
Cc <[email protected]> # 5.19.y
Signed-off-by SeongJae Park <[email protected]>

This patch is for a hotfix, so based on mm-hotfixes-unstable. And this makes a following sysfs refactoring patch, "mm/damon/sysfs: split out schemes directory implementation to separate file" conflicts. Below is the conflict resolved version of the patch. Please replace it with below one. Sorry for the inconvenience and thank you for your help in advance!

================================ 8< ===========================================

From bfb5e978a34626f5b19ee777eeb3187323e6a546 Mon Sep 17 00:00:00 2001
From: SeongJae Park <[email protected]>
Date: Wed, 26 Oct 2022 22:59:41 +0000
Subject: [PATCH mm-unstable] mm/damon/sysfs: split out schemes directory implementation to
separate file

DAMON sysfs interface for 'schemes' directory is implemented using about one thousand lines of code. It has no strong dependency with other parts of its file, so split it out to another file for better code management.

Link https://lkml.kernel.org/r/[email protected] open here

Signed-off-by SeongJae Park <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---
 mm/damon/Makefile        |    2 +-
 mm/damon/sysfs-common.h  |   22 +
 mm/damon/sysfs-schemes.c | 1068 ++++++++++++++++++++++++++++++++++++++
 mm/damon/sysfs.c         | 1064 -------------------------------------
 4 files changed, 1091 insertions(+), 1065 deletions(-)
 create mode 100644 mm/damon/sysfs-schemes.c

mm/damon/Makefile +1 -1

index f8d535a6253b..1e86f5253d7f 100644
@@ -3,7 +3,7 @@
 obj-y				:= core.o
 obj-$(CONFIG_DAMON_VADDR)	+= ops-common.o vaddr.o
 obj-$(CONFIG_DAMON_PADDR)	+= ops-common.o paddr.o
-obj-$(CONFIG_DAMON_SYSFS)	+= sysfs-common.o sysfs.o
+obj-$(CONFIG_DAMON_SYSFS)	+= sysfs-common.o sysfs-schemes.o sysfs.o
 obj-$(CONFIG_DAMON_DBGFS)	+= dbgfs.o
 obj-$(CONFIG_DAMON_RECLAIM)	+= reclaim.o
 obj-$(CONFIG_DAMON_LRU_SORT)	+= lru_sort.o

mm/damon/sysfs-common.h +22 -0

index 56e6a99e353b..4626b2784404 100644
@@ -22,3 +22,25 @@ struct damon_sysfs_ul_range *damon_sysfs_ul_range_alloc(
 void damon_sysfs_ul_range_release(struct kobject *kobj);
 
 extern struct kobj_type damon_sysfs_ul_range_ktype;
+
+/*
+ * schemes directory
+ */
+
+struct damon_sysfs_schemes {
+	struct kobject kobj;
+	struct damon_sysfs_scheme **schemes_arr;
+	int nr;
+};
+
+struct damon_sysfs_schemes *damon_sysfs_schemes_alloc(void);
+void damon_sysfs_schemes_rm_dirs(struct damon_sysfs_schemes *schemes);
+
+extern struct kobj_type damon_sysfs_schemes_ktype;
+
+int damon_sysfs_set_schemes(struct damon_ctx *ctx,
+		struct damon_sysfs_schemes *sysfs_schemes);
+
+void damon_sysfs_schemes_update_stats(
+		struct damon_sysfs_schemes *sysfs_schemes,
+		struct damon_ctx *ctx);

mm/damon/sysfs-schemes.c new file +1068 -0

1,068 lines not on this page · raw message

mm/damon/sysfs.c +0 ~~-1064~~

1,082 lines not on this page · raw message

Signature · 1 line

2.25.1

SeongJae Park

author maintainer

2022-11-22 22:32 UTC #2 lore raw

Quoted 51 lines · On Tue, 22 Nov 2022 19:53:33 +0000 SeongJae Park <[email protected]> wrote:

Hi Andrew,
Quoted 10 lines · On Tue, 22 Nov 2022 19:48:31 +0000 SeongJae Park <[email protected]> wrote:
Commit da87878010e5 ("mm/damon/sysfs: support online inputs update") made 'damon_sysfs_set_schemes()' to be called for running DAMON context, which could have schemes. In the case, DAMON sysfs interface is supposed to update, remove, or add schemes to reflect the sysfs files. However, the code is assuming the DAMON context wouldn't have schemes at all, and therefore creates and adds new schemes. As a result, the code doesn't work as intended for online schemes tuning and could have more
than expected memory footprint. The schemes are all in the DAMON
context, so it doesn't leak the memory, though.
Quoted 6 lines
Remove the wrong asssumption (the DAMON context wouldn't have schemes) in 'damon_sysfs_set_schemes()' to fix the bug.
Fixes da87878010e5 ("mm/damon/sysfs: support online inputs update")
Cc <[email protected]> # 5.19.y
Signed-off-by SeongJae Park <[email protected]>
This patch is for a hotfix, so based on mm-hotfixes-unstable. And this makes a following sysfs refactoring patch, "mm/damon/sysfs: split out schemes directory implementation to separate file" conflicts. Below is the conflict resolved version of the patch. Please replace it with below one. Sorry for the inconvenience and thank you for your help in advance!
================================ 8< ===========================================
From bfb5e978a34626f5b19ee777eeb3187323e6a546 Mon Sep 17 00:00:00 2001
From: SeongJae Park <[email protected]>
Date: Wed, 26 Oct 2022 22:59:41 +0000
Subject: [PATCH mm-unstable] mm/damon/sysfs: split out schemes directory implementation to
separate file
DAMON sysfs interface for 'schemes' directory is implemented using about one thousand lines of code. It has no strong dependency with other parts of its file, so split it out to another file for better code management.
Link https://lkml.kernel.org/r/[email protected] open here
Signed-off-by SeongJae Park <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
---
 mm/damon/Makefile        |    2 +-
 mm/damon/sysfs-common.h  |   22 +
 mm/damon/sysfs-schemes.c | 1068 ++++++++++++++++++++++++++++++++++++++
 mm/damon/sysfs.c         | 1064 -------------------------------------
 4 files changed, 1091 insertions(+), 1065 deletions(-)
 create mode 100644 mm/damon/sysfs-schemes.c

[...]

Quoted 5 lines

mm/damon/sysfs-schemes.c new file +0 -0
new file mode 100644
index 000000000000..731a2314623c

[...]

mm/damon/sysfs-schemes.c

Quoted 2 lines

mm/damon/sysfs-schemes.c new file +2 -0

+static int damon_sysfs_set_schemes(struct damon_ctx *ctx,
+		struct damon_sysfs_schemes *sysfs_schemes)

Sorry, I made a mistake. This should be not static. Below is the fixed version of the patch.

================================= 8< ==========================================

From b1e13a4ebe12485d8c54c36e9ddaec32c34e31ea Mon Sep 17 00:00:00 2001
From: SeongJae Park <[email protected]>
Date: Wed, 26 Oct 2022 22:59:41 +0000
Subject: [PATCH] mm/damon/sysfs: split out schemes directory implementation to
separate file

Link https://lkml.kernel.org/r/[email protected] open here

Signed-off-by SeongJae Park <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---
 mm/damon/Makefile        |    2 +-
 mm/damon/sysfs-common.h  |   22 +
 mm/damon/sysfs-schemes.c | 1068 ++++++++++++++++++++++++++++++++++++++
 mm/damon/sysfs.c         | 1064 -------------------------------------
 4 files changed, 1091 insertions(+), 1065 deletions(-)
 create mode 100644 mm/damon/sysfs-schemes.c

mm/damon/Makefile +1 -1

index f8d535a6253b..1e86f5253d7f 100644
@@ -3,7 +3,7 @@
 obj-y				:= core.o
 obj-$(CONFIG_DAMON_VADDR)	+= ops-common.o vaddr.o
 obj-$(CONFIG_DAMON_PADDR)	+= ops-common.o paddr.o
-obj-$(CONFIG_DAMON_SYSFS)	+= sysfs-common.o sysfs.o
+obj-$(CONFIG_DAMON_SYSFS)	+= sysfs-common.o sysfs-schemes.o sysfs.o
 obj-$(CONFIG_DAMON_DBGFS)	+= dbgfs.o
 obj-$(CONFIG_DAMON_RECLAIM)	+= reclaim.o
 obj-$(CONFIG_DAMON_LRU_SORT)	+= lru_sort.o

mm/damon/sysfs-common.h +22 -0

index 56e6a99e353b..4626b2784404 100644
@@ -22,3 +22,25 @@ struct damon_sysfs_ul_range *damon_sysfs_ul_range_alloc(
 void damon_sysfs_ul_range_release(struct kobject *kobj);
 
 extern struct kobj_type damon_sysfs_ul_range_ktype;
+
+/*
+ * schemes directory
+ */
+
+struct damon_sysfs_schemes {
+	struct kobject kobj;
+	struct damon_sysfs_scheme **schemes_arr;
+	int nr;
+};
+
+struct damon_sysfs_schemes *damon_sysfs_schemes_alloc(void);
+void damon_sysfs_schemes_rm_dirs(struct damon_sysfs_schemes *schemes);
+
+extern struct kobj_type damon_sysfs_schemes_ktype;
+
+int damon_sysfs_set_schemes(struct damon_ctx *ctx,
+		struct damon_sysfs_schemes *sysfs_schemes);
+
+void damon_sysfs_schemes_update_stats(
+		struct damon_sysfs_schemes *sysfs_schemes,
+		struct damon_ctx *ctx);

mm/damon/sysfs-schemes.c new file +1068 -0

1,068 lines not on this page · raw message

mm/damon/sysfs.c +0 ~~-1064~~

1,082 lines not on this page · raw message

Signature · 1 line

2.25.1