[RFC 0/2] Damos filter cleanup code and implement workingset

[RFC 0/2] Damos filter cleanup code and implement workingset

[Huan Yang]

Now damos support filter contains two type.
The first is scheme filter which will invoke each scheme apply,
second is scheme action filter, which will filter out unwanted action.

But this implement is scattered in different implementations and hard
to reuse or extend.

This patchset clean up those filter code, move into filter.c and add header
to expose filter func.(patch 2) And add a new filter "workingset" to 
protect workingset page.

Do we need this and cleanup it?

Huan Yang (2):
  mm/damos/filter: Add workingset page filter
  mm/damos/filter: Damos filter cleanup

 include/linux/damon.h    |  62 +-----------------
 mm/damon/Makefile        |   2 +-
 mm/damon/core-test.h     |   7 ++
 mm/damon/core.c          |  93 ++++-----------------------
 mm/damon/filter.c        | 135 +++++++++++++++++++++++++++++++++++++++
 mm/damon/filter.h        | 119 ++++++++++++++++++++++++++++++++++
 mm/damon/paddr.c         |  29 +++------
 mm/damon/reclaim.c       |  48 +++++++++++---
 mm/damon/sysfs-schemes.c |   1 +
 9 files changed, 325 insertions(+), 171 deletions(-)
 create mode 100644 mm/damon/filter.c
 create mode 100644 mm/damon/filter.h

-- 
2.34.1

[RFC 1/2] mm/damos/filter: Add workingset page filter

[Huan Yang]

Some page if marked as workingset, we'd better not touch it.
So, for damon pa scheme, this patch just add an new filter
DAMOS_FILTER_TYPE_WORKINGSET, which will filter page if it
marked as workingset.

Like skip anon, add a control skip_workingset.

To make code clean, fold reclaim's filter create logic into
__damon_reclaim_create_filters.

Signed-off-by: Huan Yang <link@vivo.com>
---
 include/linux/damon.h | 17 +++++++++-------
 mm/damon/core-test.h  |  7 +++++++
 mm/damon/paddr.c      |  4 ++++
 mm/damon/reclaim.c    | 47 ++++++++++++++++++++++++++++++++++---------
 4 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index ae2664d1d5f1..8e8f35df6a5e 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -228,22 +228,25 @@ struct damos_stat {
  * @DAMOS_FILTER_TYPE_MEMCG:	Specific memcg's pages.
  * @DAMOS_FILTER_TYPE_ADDR:	Address range.
  * @DAMOS_FILTER_TYPE_TARGET:	Data Access Monitoring target.
+ * @DAMOS_FILTER_TYPE_WORKINGSET: Workingset pages, need protect.
  * @NR_DAMOS_FILTER_TYPES:	Number of filter types.
  *
- * The anon pages type and memcg type filters are handled by underlying
- * &struct damon_operations as a part of scheme action trying, and therefore
- * accounted as 'tried'.  In contrast, other types are handled by core layer
- * before trying of the action and therefore not accounted as 'tried'.
+ * The anon pages type, memcg type, workingset page type filters are handled
+ * by underlying &struct damon_operations as a part of scheme action trying,
+ * and therefore accounted as 'tried'.  In contrast, other types are handled
+ * by core layer before trying of the action and therefore not accounted as
+ * 'tried'.
  *
  * The support of the filters that handled by &struct damon_operations depend
  * on the running &struct damon_operations.
- * &enum DAMON_OPS_PADDR supports both anon pages type and memcg type filters,
- * while &enum DAMON_OPS_VADDR and &enum DAMON_OPS_FVADDR don't support any of
- * the two types.
+ * &enum DAMON_OPS_PADDR supports both anon pages type, memcg type and
+ * workingset page type filters, while &enum DAMON_OPS_VADDR and &enum
+ * DAMON_OPS_FVADDR don't support any of the two types.
  */
 enum damos_filter_type {
 	DAMOS_FILTER_TYPE_ANON,
 	DAMOS_FILTER_TYPE_MEMCG,
+	DAMOS_FILTER_TYPE_WORKINGSET,
 	DAMOS_FILTER_TYPE_ADDR,
 	DAMOS_FILTER_TYPE_TARGET,
 	NR_DAMOS_FILTER_TYPES,
diff --git a/mm/damon/core-test.h b/mm/damon/core-test.h
index 6cc8b245586d..c752e6e3cf3e 100644
--- a/mm/damon/core-test.h
+++ b/mm/damon/core-test.h
@@ -351,6 +351,13 @@ static void damos_test_new_filter(struct kunit *test)
 	KUNIT_EXPECT_PTR_EQ(test, filter->list.prev, &filter->list);
 	KUNIT_EXPECT_PTR_EQ(test, filter->list.next, &filter->list);
 	damos_destroy_filter(filter);
+
+	filter = damos_new_filter(DAMOS_FILTER_TYPE_WORKINGSET, true);
+	KUNIT_EXPECT_EQ(test, filter->type, DAMOS_FILTER_TYPE_WORKINGSET);
+	KUNIT_EXPECT_EQ(test, filter->matching, true);
+	KUNIT_EXPECT_PTR_EQ(test, filter->list.prev, &filter->list);
+	KUNIT_EXPECT_PTR_EQ(test, filter->list.next, &filter->list);
+	damos_destroy_filter(filter);
 }
 
 static void damos_test_filter_out(struct kunit *test)
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 909db25efb35..8a690505e033 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -12,6 +12,7 @@
 #include <linux/pagemap.h>
 #include <linux/rmap.h>
 #include <linux/swap.h>
+#include <linux/mm_inline.h>
 
 #include "../internal.h"
 #include "ops-common.h"
@@ -204,6 +205,9 @@ static bool __damos_pa_filter_out(struct damos_filter *filter,
 			matched = filter->memcg_id == mem_cgroup_id(memcg);
 		rcu_read_unlock();
 		break;
+	case DAMOS_FILTER_TYPE_WORKINGSET:
+		matched = folio_test_workingset(folio);
+		break;
 	default:
 		break;
 	}
diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 648d2a85523a..26ae8fa5d088 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -107,6 +107,15 @@ module_param(monitor_region_end, ulong, 0600);
 static bool skip_anon __read_mostly;
 module_param(skip_anon, bool, 0600);
 
+/*
+ * Skip workingset pages reclamation.
+ *
+ * If this parameter is set as ``Y``, DAMON_RECLAIM does not reclaim workingset
+ * pages.  By default, ``N``.
+ */
+static bool skip_workingset __read_mostly;
+module_param(skip_workingset, bool, 0600);
+
 /*
  * PID of the DAMON thread
  *
@@ -148,10 +157,25 @@ static struct damos *damon_reclaim_new_scheme(void)
 			&damon_reclaim_wmarks);
 }
 
+static int __damon_reclaim_create_filters(struct damos *scheme,
+					  enum damos_filter_type type,
+					  bool matching)
+{
+	struct damos_filter *filter = damos_new_filter(type, matching);
+
+	if (unlikely(!filter)) {
+		/* Will be freed by next 'damon_set_schemes()' below */
+		damon_destroy_scheme(scheme);
+		return -ENOMEM;
+	}
+
+	damos_add_filter(scheme, filter);
+	return 0;
+}
+
 static int damon_reclaim_apply_parameters(void)
 {
 	struct damos *scheme;
-	struct damos_filter *filter;
 	int err = 0;
 
 	err = damon_set_attrs(ctx, &damon_reclaim_mon_attrs);
@@ -162,15 +186,18 @@ static int damon_reclaim_apply_parameters(void)
 	scheme = damon_reclaim_new_scheme();
 	if (!scheme)
 		return -ENOMEM;
-	if (skip_anon) {
-		filter = damos_new_filter(DAMOS_FILTER_TYPE_ANON, true);
-		if (!filter) {
-			/* Will be freed by next 'damon_set_schemes()' below */
-			damon_destroy_scheme(scheme);
-			return -ENOMEM;
-		}
-		damos_add_filter(scheme, filter);
-	}
+
+	err = skip_anon && __damon_reclaim_create_filters(
+				   scheme, DAMOS_FILTER_TYPE_ANON, true);
+	if (err)
+		return err;
+
+	err = skip_workingset &&
+	      __damon_reclaim_create_filters(
+		      scheme, DAMOS_FILTER_TYPE_WORKINGSET, true);
+	if (err)
+		return err;
+
 	damon_set_schemes(ctx, &scheme, 1);
 
 	return damon_set_region_biggest_system_ram_default(target,
-- 
2.34.1

[RFC 2/2] mm/damos/filter: Damos filter cleanup

[Huan Yang]

Now damos support filter contains two type.
The first is scheme filter which will invoke each scheme apply,
second is scheme action filter, which will filter out unwanted action.

But this implement is scattered in different implementations and hard
to reuse or extend.

This patch clean up those filter code, move into filter.c and add header
to expose filter func.

Each filter implement register itself into filter_fn array, and user
can invoke this by there inited type in filter.

Signed-off-by: Huan Yang <link@vivo.com>
---
 include/linux/damon.h    |  65 +------------------
 mm/damon/Makefile        |   2 +-
 mm/damon/core.c          |  93 ++++-----------------------
 mm/damon/filter.c        | 135 +++++++++++++++++++++++++++++++++++++++
 mm/damon/filter.h        | 119 ++++++++++++++++++++++++++++++++++
 mm/damon/paddr.c         |  31 +++------
 mm/damon/reclaim.c       |   1 +
 mm/damon/sysfs-schemes.c |   1 +
 8 files changed, 280 insertions(+), 167 deletions(-)
 create mode 100644 mm/damon/filter.c
 create mode 100644 mm/damon/filter.h

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 8e8f35df6a5e..03c718b30bfe 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -222,63 +222,6 @@ struct damos_stat {
 	unsigned long qt_exceeds;
 };
 
-/**
- * enum damos_filter_type - Type of memory for &struct damos_filter
- * @DAMOS_FILTER_TYPE_ANON:	Anonymous pages.
- * @DAMOS_FILTER_TYPE_MEMCG:	Specific memcg's pages.
- * @DAMOS_FILTER_TYPE_ADDR:	Address range.
- * @DAMOS_FILTER_TYPE_TARGET:	Data Access Monitoring target.
- * @DAMOS_FILTER_TYPE_WORKINGSET: Workingset pages, need protect.
- * @NR_DAMOS_FILTER_TYPES:	Number of filter types.
- *
- * The anon pages type, memcg type, workingset page type filters are handled
- * by underlying &struct damon_operations as a part of scheme action trying,
- * and therefore accounted as 'tried'.  In contrast, other types are handled
- * by core layer before trying of the action and therefore not accounted as
- * 'tried'.
- *
- * The support of the filters that handled by &struct damon_operations depend
- * on the running &struct damon_operations.
- * &enum DAMON_OPS_PADDR supports both anon pages type, memcg type and
- * workingset page type filters, while &enum DAMON_OPS_VADDR and &enum
- * DAMON_OPS_FVADDR don't support any of the two types.
- */
-enum damos_filter_type {
-	DAMOS_FILTER_TYPE_ANON,
-	DAMOS_FILTER_TYPE_MEMCG,
-	DAMOS_FILTER_TYPE_WORKINGSET,
-	DAMOS_FILTER_TYPE_ADDR,
-	DAMOS_FILTER_TYPE_TARGET,
-	NR_DAMOS_FILTER_TYPES,
-};
-
-/**
- * struct damos_filter - DAMOS action target memory filter.
- * @type:	Type of the page.
- * @matching:	If the matching page should filtered out or in.
- * @memcg_id:	Memcg id of the question if @type is DAMOS_FILTER_MEMCG.
- * @addr_range:	Address range if @type is DAMOS_FILTER_TYPE_ADDR.
- * @target_idx:	Index of the &struct damon_target of
- *		&damon_ctx->adaptive_targets if @type is
- *		DAMOS_FILTER_TYPE_TARGET.
- * @list:	List head for siblings.
- *
- * Before applying the &damos->action to a memory region, DAMOS checks if each
- * page of the region matches to this and avoid applying the action if so.
- * Support of each filter type depends on the running &struct damon_operations
- * and the type.  Refer to &enum damos_filter_type for more detai.
- */
-struct damos_filter {
-	enum damos_filter_type type;
-	bool matching;
-	union {
-		unsigned short memcg_id;
-		struct damon_addr_range addr_range;
-		int target_idx;
-	};
-	struct list_head list;
-};
-
 /**
  * struct damos_access_pattern - Target access pattern of the given scheme.
  * @min_sz_region:	Minimum size of target regions.
@@ -607,16 +550,14 @@ static inline void damon_insert_region(struct damon_region *r,
 	t->nr_regions++;
 }
 
+void damon_split_region_at(struct damon_target *t,
+				  struct damon_region *r, unsigned long sz_r);
+
 void damon_add_region(struct damon_region *r, struct damon_target *t);
 void damon_destroy_region(struct damon_region *r, struct damon_target *t);
 int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
 		unsigned int nr_ranges);
 
-struct damos_filter *damos_new_filter(enum damos_filter_type type,
-		bool matching);
-void damos_add_filter(struct damos *s, struct damos_filter *f);
-void damos_destroy_filter(struct damos_filter *f);
-
 struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
 			enum damos_action action, struct damos_quota *quota,
 			struct damos_watermarks *wmarks);
diff --git a/mm/damon/Makefile b/mm/damon/Makefile
index f7add3f4aa79..789ab0a9d9c9 100644
--- a/mm/damon/Makefile
+++ b/mm/damon/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
-obj-y				:= core.o
+obj-y				:= core.o filter.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-schemes.o sysfs.o
diff --git a/mm/damon/core.c b/mm/damon/core.c
index bcd2bd9d6c10..a74932fcdb11 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -22,6 +22,8 @@
 #define DAMON_MIN_REGION 1
 #endif
 
+#include "filter.h"
+
 static DEFINE_MUTEX(damon_lock);
 static int nr_running_ctxs;
 static bool running_exclusive_ctxs;
@@ -263,41 +265,6 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
 	return 0;
 }
 
-struct damos_filter *damos_new_filter(enum damos_filter_type type,
-		bool matching)
-{
-	struct damos_filter *filter;
-
-	filter = kmalloc(sizeof(*filter), GFP_KERNEL);
-	if (!filter)
-		return NULL;
-	filter->type = type;
-	filter->matching = matching;
-	INIT_LIST_HEAD(&filter->list);
-	return filter;
-}
-
-void damos_add_filter(struct damos *s, struct damos_filter *f)
-{
-	list_add_tail(&f->list, &s->filters);
-}
-
-static void damos_del_filter(struct damos_filter *f)
-{
-	list_del(&f->list);
-}
-
-static void damos_free_filter(struct damos_filter *f)
-{
-	kfree(f);
-}
-
-void damos_destroy_filter(struct damos_filter *f)
-{
-	damos_del_filter(f);
-	damos_free_filter(f);
-}
-
 /* initialize private fields of damos_quota and return the pointer */
 static struct damos_quota *damos_quota_init_priv(struct damos_quota *quota)
 {
@@ -780,9 +747,6 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
 	}
 }
 
-static void damon_split_region_at(struct damon_target *t,
-				  struct damon_region *r, unsigned long sz_r);
-
 static bool __damos_valid_target(struct damon_region *r, struct damos *s)
 {
 	unsigned long sz;
@@ -881,49 +845,16 @@ static void damos_update_stat(struct damos *s,
 static bool __damos_filter_out(struct damon_ctx *ctx, struct damon_target *t,
 		struct damon_region *r, struct damos_filter *filter)
 {
-	bool matched = false;
-	struct damon_target *ti;
-	int target_idx = 0;
-	unsigned long start, end;
-
-	switch (filter->type) {
-	case DAMOS_FILTER_TYPE_TARGET:
-		damon_for_each_target(ti, ctx) {
-			if (ti == t)
-				break;
-			target_idx++;
-		}
-		matched = target_idx == filter->target_idx;
-		break;
-	case DAMOS_FILTER_TYPE_ADDR:
-		start = ALIGN_DOWN(filter->addr_range.start, DAMON_MIN_REGION);
-		end = ALIGN_DOWN(filter->addr_range.end, DAMON_MIN_REGION);
-
-		/* inside the range */
-		if (start <= r->ar.start && r->ar.end <= end) {
-			matched = true;
-			break;
-		}
-		/* outside of the range */
-		if (r->ar.end <= start || end <= r->ar.start) {
-			matched = false;
-			break;
-		}
-		/* start before the range and overlap */
-		if (r->ar.start < start) {
-			damon_split_region_at(t, r, start - r->ar.start);
-			matched = false;
-			break;
-		}
-		/* start inside the range */
-		damon_split_region_at(t, r, end - r->ar.start);
-		matched = true;
-		break;
-	default:
-		break;
-	}
+	struct damos_filter_ctx dfctx = {
+		.scheme = {
+			.ctx = ctx,
+			.target = t,
+			.region = r,
+		},
+		.filter = filter,
+	};
 
-	return matched == filter->matching;
+	return damon_filter_invoke(&dfctx);
 }
 
 static bool damos_filter_out(struct damon_ctx *ctx, struct damon_target *t,
@@ -1161,7 +1092,7 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
  * r		the region to be split
  * sz_r		size of the first sub-region that will be made
  */
-static void damon_split_region_at(struct damon_target *t,
+void damon_split_region_at(struct damon_target *t,
 				  struct damon_region *r, unsigned long sz_r)
 {
 	struct damon_region *new;
diff --git a/mm/damon/filter.c b/mm/damon/filter.c
new file mode 100644
index 000000000000..a451d5428fe2
--- /dev/null
+++ b/mm/damon/filter.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Data Access Monitor Filter
+ *
+ * This filter contains the scheme filter, which will be called
+ * for each scheme apply check. action filter will be called when
+ * scheme action invoke.
+ *
+ * Author: Huan Yang <link@vivo.com>
+ */
+#include <linux/damon.h>
+
+#include "filter.h"
+
+typedef bool (*DAMON_FILTER_FN) (struct damos_filter_ctx *dfctx);
+
+/* Scheme action filter */
+
+static bool damos_anon_filter(struct damos_filter_ctx *dfctx)
+{
+	return folio_test_anon(dfctx->action.folio) == dfctx->filter->matching;
+}
+
+static bool damos_memcg_filter(struct damos_filter_ctx *dfctx)
+{
+	struct damos_filter *filter = dfctx->filter;
+	struct mem_cgroup *memcg;
+	bool matched;
+
+	rcu_read_lock();
+	memcg = folio_memcg_check(dfctx->action.folio);
+	if (!memcg)
+		matched = false;
+	else
+		matched = filter->memcg_id == mem_cgroup_id(memcg);
+	rcu_read_unlock();
+
+	return matched == filter->matching;
+}
+
+static bool damos_workingset_filter(struct damos_filter_ctx *dfctx)
+{
+	return folio_test_workingset(dfctx->action.folio) ==
+	       dfctx->filter->matching;
+}
+
+/* Scheme filter */
+
+static bool damos_addr_filter(struct damos_filter_ctx *dfctx)
+{
+	bool matched = false;
+	struct damos_filter *filter = dfctx->filter;
+	struct damon_target *t = dfctx->scheme.target;
+	struct damon_region *r = dfctx->scheme.region;
+	unsigned long start, end;
+
+	start = ALIGN_DOWN(filter->addr_range.start, DAMON_MIN_REGION);
+	end = ALIGN_DOWN(filter->addr_range.end, DAMON_MIN_REGION);
+
+	/* inside the range */
+	if (start <= r->ar.start && r->ar.end <= end) {
+		matched = true;
+		goto got_result;
+	}
+	/* outside of the range */
+	if (r->ar.end <= start || end <= r->ar.start) {
+		matched = false;
+		goto got_result;
+	}
+	/* start before the range and overlap */
+	if (r->ar.start < start) {
+		damon_split_region_at(t, r, start - r->ar.start);
+		matched = false;
+		goto got_result;
+	}
+	/* start inside the range */
+	damon_split_region_at(t, r, end - r->ar.start);
+	matched = true;
+
+got_result:
+	return matched == filter->matching;
+}
+
+static bool damos_target_filter(struct damos_filter_ctx *dfctx)
+{
+	bool matched = false;
+	struct damon_target *ti, *t = dfctx->scheme.target;
+	struct damon_ctx *ctx = dfctx->scheme.ctx;
+	struct damos_filter *filter = dfctx->filter;
+	int target_idx = 0;
+
+	damon_for_each_target(ti, ctx) {
+		if (ti == t)
+			break;
+		target_idx++;
+	}
+	matched = target_idx == filter->target_idx;
+	return matched == filter->matching;
+}
+
+static DAMON_FILTER_FN filter_fn[NR_DAMOS_FILTER_TYPES] = {
+	/* Damos scheme action filter */
+	damos_anon_filter,
+	damos_memcg_filter,
+	damos_workingset_filter,
+	/* Damos scheme filter */
+	damos_addr_filter,
+	damos_target_filter,
+};
+
+
+bool damon_filter_invoke(struct damos_filter_ctx *dfctx)
+{
+	return filter_fn[dfctx->filter->type](dfctx);
+}
+
+struct damos_filter *damos_new_filter(enum damos_filter_type type,
+		bool matching)
+{
+	struct damos_filter *filter;
+
+	filter = kmalloc(sizeof(*filter), GFP_KERNEL);
+	if (!filter)
+		return NULL;
+	filter->type = type;
+	filter->matching = matching;
+	INIT_LIST_HEAD(&filter->list);
+	return filter;
+}
+
+void damos_destroy_filter(struct damos_filter *f)
+{
+	damos_del_filter(f);
+	damos_free_filter(f);
+}
diff --git a/mm/damon/filter.h b/mm/damon/filter.h
new file mode 100644
index 000000000000..5d724e8276fc
--- /dev/null
+++ b/mm/damon/filter.h
@@ -0,0 +1,119 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Data Access Monitor Filter
+ *
+ * Author: Huan Yang <link@vivo.com>
+ */
+#ifndef __DAMON_FILTER_H__
+#define __DAMON_FILTER_H__
+
+/**
+ * enum damos_filter_type - Type of memory for &struct damos_filter
+ * @DAMOS_FILTER_TYPE_ANON:	Anonymous pages.
+ * @DAMOS_FILTER_TYPE_MEMCG:	Specific memcg's pages.
+ * @DAMOS_FILTER_TYPE_ADDR:	Address range.
+ * @DAMOS_FILTER_TYPE_TARGET:	Data Access Monitoring target.
+ * @DAMOS_FILTER_TYPE_WORKINGSET: Workingset pages, need protect.
+ * @NR_DAMOS_FILTER_TYPES:	Number of filter types.
+ *
+ * The anon pages type, memcg type, workingset page type filters are handled
+ * by underlying &struct damon_operations as a part of scheme action trying,
+ * and therefore accounted as 'tried'.  In contrast, other types are handled
+ * by core layer before trying of the action and therefore not accounted as
+ * 'tried'.
+ *
+ * The support of the filters that handled by &struct damon_operations depend
+ * on the running &struct damon_operations.
+ * &enum DAMON_OPS_PADDR supports both anon pages type, memcg type and
+ * workingset page type filters, while &enum DAMON_OPS_VADDR and &enum
+ * DAMON_OPS_FVADDR don't support any of the two types.
+ */
+enum damos_filter_type {
+	/* Damos action filter, tried */
+	DAMOS_FILTER_TYPE_ANON,
+	DAMOS_FILTER_TYPE_MEMCG,
+	DAMOS_FILTER_TYPE_WORKINGSET,
+	/* Damos scheme filter */
+	DAMOS_FILTER_TYPE_ADDR,
+	DAMOS_FILTER_TYPE_TARGET,
+	NR_DAMOS_FILTER_TYPES,
+};
+
+/**
+ * struct damos_filter - DAMOS action target memory filter.
+ * @type:	Type of the page.
+ * @matching:	If the matching page should filtered out or in.
+ * @memcg_id:	Memcg id of the question if @type is DAMOS_FILTER_MEMCG.
+ * @addr_range:	Address range if @type is DAMOS_FILTER_TYPE_ADDR.
+ * @target_idx:	Index of the &struct damon_target of
+ *		&damon_ctx->adaptive_targets if @type is
+ *		DAMOS_FILTER_TYPE_TARGET.
+ * @list:	List head for siblings.
+ *
+ * Before applying the &damos->action to a memory region, DAMOS checks if each
+ * page of the region matches to this and avoid applying the action if so.
+ * Support of each filter type depends on the running &struct damon_operations
+ * and the type.  Refer to &enum damos_filter_type for more detai.
+ */
+struct damos_filter {
+	enum damos_filter_type type;
+	bool matching;
+	union {
+		unsigned short memcg_id;
+		struct damon_addr_range addr_range;
+		int target_idx;
+	};
+	struct list_head list;
+};
+
+/**
+ * struct damos_filter_ctx - Represents a context for each filter
+ * @scheme:     Each scheme filter context
+ * @action:     Each scheme action filter context
+ * @filter:     DAMOS action target memory filter instance
+ *
+ * User need pass this for each invoke filter, and each filter
+ * will invoke a specified filter function by filter type which
+ * user already target it when register filter.
+ */
+struct damos_filter_ctx {
+	union {
+		/* Use by scheme filter */
+		struct {
+			struct damon_ctx *ctx;
+			struct damon_target *target;
+			struct damon_region *region;
+		} scheme;
+
+		/* Use by action filter */
+		struct {
+			struct folio *folio;
+		} action;
+	};
+
+	struct damos_filter *filter;
+};
+
+bool damon_filter_invoke(struct damos_filter_ctx *dfctx);
+
+struct damos_filter *damos_new_filter(enum damos_filter_type type,
+				      bool matching);
+
+static inline void damos_add_filter(struct damos *s, struct damos_filter *f)
+{
+	list_add_tail(&f->list, &s->filters);
+}
+
+static inline void damos_del_filter(struct damos_filter *f)
+{
+	list_del(&f->list);
+}
+
+static inline void damos_free_filter(struct damos_filter *f)
+{
+	kfree(f);
+}
+
+void damos_destroy_filter(struct damos_filter *f);
+
+#endif //__DAMON_FILTER_H__
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 8a690505e033..748300655ba4 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -16,6 +16,7 @@
 
 #include "../internal.h"
 #include "ops-common.h"
+#include "filter.h"
 
 static bool __damon_pa_mkold(struct folio *folio, struct vm_area_struct *vma,
 		unsigned long addr, void *arg)
@@ -189,30 +190,14 @@ static unsigned int damon_pa_check_accesses(struct damon_ctx *ctx)
 static bool __damos_pa_filter_out(struct damos_filter *filter,
 		struct folio *folio)
 {
-	bool matched = false;
-	struct mem_cgroup *memcg;
-
-	switch (filter->type) {
-	case DAMOS_FILTER_TYPE_ANON:
-		matched = folio_test_anon(folio);
-		break;
-	case DAMOS_FILTER_TYPE_MEMCG:
-		rcu_read_lock();
-		memcg = folio_memcg_check(folio);
-		if (!memcg)
-			matched = false;
-		else
-			matched = filter->memcg_id == mem_cgroup_id(memcg);
-		rcu_read_unlock();
-		break;
-	case DAMOS_FILTER_TYPE_WORKINGSET:
-		matched = folio_test_workingset(folio);
-		break;
-	default:
-		break;
-	}
+	struct damos_filter_ctx dfctx = {
+		.action = {
+			.folio = folio,
+		},
+		.filter = filter,
+	};
 
-	return matched == filter->matching;
+	return damon_filter_invoke(&dfctx);
 }
 
 /*
diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 26ae8fa5d088..1fdd55c03f1f 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 
 #include "modules-common.h"
+#include "filter.h"
 
 #ifdef MODULE_PARAM_PREFIX
 #undef MODULE_PARAM_PREFIX
diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 527e7d17eb3b..98cd93b28b21 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -8,6 +8,7 @@
 #include <linux/slab.h>
 
 #include "sysfs-common.h"
+#include "filter.h"
 
 /*
  * scheme region directory
-- 
2.34.1

Re: [RFC 0/2] Damos filter cleanup code and implement workingset

[SeongJae Park]

Hi Huan,


First of all, thank you for this patch.  I have some high level comments and
questions as below.

On Tue, 19 Sep 2023 17:52:33 +0800 Huan Yang <link@vivo.com> wrote:

> Now damos support filter contains two type.
> The first is scheme filter which will invoke each scheme apply,
> second is scheme action filter, which will filter out unwanted action.

IMHO, that's not clear definition of the types.  Conceptually, all the filters
are same.  Nonetheless, they are having different behaviors because they are
handled in different layer depending on which layer is more efficient to handle
those[1].

I agree this is complicated and a bit verbose explanation, but I don't feel
your scheme vs action definition is making it easier to understand.

> 
> But this implement is scattered in different implementations

Indeed the implementation is scattered in core layer and the ops layer
depending on the filter types.  But I think this is needed for efficient
handling of those.

> and hard to reuse or extend.

From your first patch, which extending the filter, the essential change for the
extension is adding another enum to 'enum damos_filter_type' (1 line) and
addition of switch case in '__damos_pa_filter_out()' (3 lines).  I don't think
it looks too complicated.  If you're saying it was hard to understand which
part really need to be modifed, I think that makes sense.  But in the case,
we might need more comments rather than structural change.

> 
> This patchset clean up those filter code, move into filter.c and add header
> to expose filter func.(patch 2)

Again, I agree more code cleanup is needed.  But I'm not sure if this is the
right way.  Especially, I'm unsure if it really need to have a dedicated file.
To my humble eyes, it doesn't look like making code clearly easier to read
compared to the current version.  This is probably because I don't feel your
scheme vs action definition is clear to understand.  Neither it is
deduplicating code.  The patch adds lines more that deleted ones, according to
its diffstat.  For the reason, I don't see clear benefit of this change.  I
might be biased, having NIH (Not Invented Here) sindrome, or missing something.
Please let me know if so.

> And add a new filter "workingset" to protect workingset page.

Can you explain expected use cases of this new filter type in more detail?
Specifically, for what scheme action and under what situation this new filter
type will be needed?  If you have some test data for the use case and could
share it, it would be very helpful for me to understand why it is needed.

> 
> Do we need this and cleanup it?

I think I cannot answer for now, and your further clarification and patient
explanation could be helpful.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/Documentation/mm/damon/design.rst?h=mm-everything-2023-09-20-19-38#n400


Thanks,
SJ

> 
> Huan Yang (2):
>   mm/damos/filter: Add workingset page filter
>   mm/damos/filter: Damos filter cleanup
> 
>  include/linux/damon.h    |  62 +-----------------
>  mm/damon/Makefile        |   2 +-
>  mm/damon/core-test.h     |   7 ++
>  mm/damon/core.c          |  93 ++++-----------------------
>  mm/damon/filter.c        | 135 +++++++++++++++++++++++++++++++++++++++
>  mm/damon/filter.h        | 119 ++++++++++++++++++++++++++++++++++
>  mm/damon/paddr.c         |  29 +++------
>  mm/damon/reclaim.c       |  48 +++++++++++---
>  mm/damon/sysfs-schemes.c |   1 +
>  9 files changed, 325 insertions(+), 171 deletions(-)
>  create mode 100644 mm/damon/filter.c
>  create mode 100644 mm/damon/filter.h
> 
> -- 
> 2.34.1
> 
> 

Re: [RFC 0/2] Damos filter cleanup code and implement workingset

[杨欢]

HI SJ,

Thanks for your patient response.
> [Some people who received this message don't often get email from sj@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Huan,
>
>
> First of all, thank you for this patch.  I have some high level comments and
> questions as below.
>
> On Tue, 19 Sep 2023 17:52:33 +0800 Huan Yang <link@vivo.com> wrote:
>
>> Now damos support filter contains two type.
>> The first is scheme filter which will invoke each scheme apply,
>> second is scheme action filter, which will filter out unwanted action.
> IMHO, that's not clear definition of the types.  Conceptually, all the filters
> are same.  Nonetheless, they are having different behaviors because they are
> handled in different layer depending on which layer is more efficient to handle
Thanks for these instructions to help me understand the design idea of 
damos filter.
> those[1].
>
> I agree this is complicated and a bit verbose explanation, but I don't feel
> your scheme vs action definition is making it easier to understand.
>
>> But this implement is scattered in different implementations
> Indeed the implementation is scattered in core layer and the ops layer
> depending on the filter types.  But I think this is needed for efficient
> handling of those.

IMO, some simple filter can have a common implement, like anon filter? And,

for some special type, each layer offer their own?

>
>> and hard to reuse or extend.
>  From your first patch, which extending the filter, the essential change for the
> extension is adding another enum to 'enum damos_filter_type' (1 line) and
> addition of switch case in '__damos_pa_filter_out()' (3 lines).  I don't think
> it looks too complicated.  If you're saying it was hard to understand which
Yes, indeed.
> part really need to be modifed, I think that makes sense.  But in the case,
> we might need more comments rather than structural change.
>
>> This patchset clean up those filter code, move into filter.c and add header
>> to expose filter func.(patch 2)
> Again, I agree more code cleanup is needed.  But I'm not sure if this is the
> right way.  Especially, I'm unsure if it really need to have a dedicated file.

I think the filter code scatter into each layer may make code hard to 
reuse, other ops

may need anon filter or something. So, make code into a dedicated file 
may good?

(just my opinion.)

> To my humble eyes, it doesn't look like making code clearly easier to read
> compared to the current version.  This is probably because I don't feel your
> scheme vs action definition is clear to understand.  Neither it is
Yes, indeed, current code not clean, the idea didn't take shape.
> deduplicating code.  The patch adds lines more that deleted ones, according to
> its diffstat.  For the reason, I don't see clear benefit of this change.
In this code, maybe just a little benefit when other ops need to filter 
anon page? :)
>    I
> might be biased, having NIH (Not Invented Here) sindrome, or missing something.
> Please let me know if so.
>
>> And add a new filter "workingset" to protect workingset page.
> Can you explain expected use cases of this new filter type in more detail?
> Specifically, for what scheme action and under what situation this new filter

IMO, page if marked as workingset, mean page in task's core 
workload(maybe have

seen the refault, and trigger refault protect). So, for lru sort or reclaim,

we'd better not touch it?(If any wrong, please let me know.)

> type will be needed?  If you have some test data for the use case and could
> share it, it would be very helpful for me to understand why it is needed.

Sorry, this type just from my knowledge of MM, have no test data.

For futher learn of DAMON, I'll try it.

>
>> Do we need this and cleanup it?
> I think I cannot answer for now, and your further clarification and patient
> explanation could be helpful.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/Documentation/mm/damon/design.rst?h=mm-everything-2023-09-20-19-38#n400
>
>
> Thanks,
> SJ

Thanks,

Huan

>
>> Huan Yang (2):
>>    mm/damos/filter: Add workingset page filter
>>    mm/damos/filter: Damos filter cleanup
>>
>>   include/linux/damon.h    |  62 +-----------------
>>   mm/damon/Makefile        |   2 +-
>>   mm/damon/core-test.h     |   7 ++
>>   mm/damon/core.c          |  93 ++++-----------------------
>>   mm/damon/filter.c        | 135 +++++++++++++++++++++++++++++++++++++++
>>   mm/damon/filter.h        | 119 ++++++++++++++++++++++++++++++++++
>>   mm/damon/paddr.c         |  29 +++------
>>   mm/damon/reclaim.c       |  48 +++++++++++---
>>   mm/damon/sysfs-schemes.c |   1 +
>>   9 files changed, 325 insertions(+), 171 deletions(-)
>>   create mode 100644 mm/damon/filter.c
>>   create mode 100644 mm/damon/filter.h
>>
>> --
>> 2.34.1
>>
>>

Re: [RFC 0/2] Damos filter cleanup code and implement workingset

[SeongJae Park]

Hi Huan,

On Fri, 22 Sep 2023 02:54:58 +0000 杨欢 <link@vivo.com> wrote:

> HI SJ,
> 
> Thanks for your patient response.

Happy to hear this kind acknowledgement :)

> > [Some people who received this message don't often get email from sj@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > Hi Huan,
> >
> >
> > First of all, thank you for this patch.  I have some high level comments and
> > questions as below.
> >
> > On Tue, 19 Sep 2023 17:52:33 +0800 Huan Yang <link@vivo.com> wrote:
> >
> >> Now damos support filter contains two type.
> >> The first is scheme filter which will invoke each scheme apply,
> >> second is scheme action filter, which will filter out unwanted action.
> > IMHO, that's not clear definition of the types.  Conceptually, all the filters
> > are same.  Nonetheless, they are having different behaviors because they are
> > handled in different layer depending on which layer is more efficient to handle
> Thanks for these instructions to help me understand the design idea of 
> damos filter.
> > those[1].
> >
> > I agree this is complicated and a bit verbose explanation, but I don't feel
> > your scheme vs action definition is making it easier to understand.
> >
> >> But this implement is scattered in different implementations
> > Indeed the implementation is scattered in core layer and the ops layer
> > depending on the filter types.  But I think this is needed for efficient
> > handling of those.
> 
> IMO, some simple filter can have a common implement, like anon filter? And,
> for some special type, each layer offer their own?

Do you mean there could be two filter types that better to be handled in
different layer for efficiency, but share common implementation?  Could you
please give me a more specific example?

> 
> >
> >> and hard to reuse or extend.
> >  From your first patch, which extending the filter, the essential change for the
> > extension is adding another enum to 'enum damos_filter_type' (1 line) and
> > addition of switch case in '__damos_pa_filter_out()' (3 lines).  I don't think
> > it looks too complicated.  If you're saying it was hard to understand which
> Yes, indeed.
> > part really need to be modifed, I think that makes sense.  But in the case,
> > we might need more comments rather than structural change.
> >
> >> This patchset clean up those filter code, move into filter.c and add header
> >> to expose filter func.(patch 2)
> > Again, I agree more code cleanup is needed.  But I'm not sure if this is the
> > right way.  Especially, I'm unsure if it really need to have a dedicated file.
> 
> I think the filter code scatter into each layer may make code hard to 
> reuse, other ops
> 
> may need anon filter or something. So, make code into a dedicated file 
> may good?
> 
> (just my opinion.)

Again, I'm not super confident about my understanding.  But sounds like you're
partly worrying about duplication of some code in each ops implementation
(modules in same layer).  Please correct me if I'm wrong, with specific,
detailed and realistic examples.

If my guess is not that wrong, I can agree to the concern.  Nevertheless, we
already have a dedicated source file for such common code between ops
implementaions, namely ops-common.c.

That said, we don't have duplicated DAMOS filters implementation code in
multipe ops implementation at the moment.  It could have such duplication in
the future, but I think it's not too late to make such cleanup after or just
before such duplication heppen.  IOW, I'd suggest to not make changes for
something that _might_ happen in future.

> 
> > To my humble eyes, it doesn't look like making code clearly easier to read
> > compared to the current version.  This is probably because I don't feel your
> > scheme vs action definition is clear to understand.  Neither it is
> Yes, indeed, current code not clean, the idea didn't take shape.
> > deduplicating code.  The patch adds lines more that deleted ones, according to
> > its diffstat.  For the reason, I don't see clear benefit of this change.
> In this code, maybe just a little benefit when other ops need to filter 
> anon page? :)

As mentioned above, it's unclear when, how, and for what benefit we will
support anon pages filter from vaddr.  I'd again suggest to not make change
only for future change that not clear if we really want to make for now.

> >    I
> > might be biased, having NIH (Not Invented Here) sindrome, or missing something.
> > Please let me know if so.
> >
> >> And add a new filter "workingset" to protect workingset page.
> > Can you explain expected use cases of this new filter type in more detail?
> > Specifically, for what scheme action and under what situation this new filter
> 
> IMO, page if marked as workingset, mean page in task's core 
> workload(maybe have
> 
> seen the refault, and trigger refault protect). So, for lru sort or reclaim,
> 
> we'd better not touch it?(If any wrong, please let me know.)

Still unclear to me.  Could I ask you more specific and detailed case?

> 
> > type will be needed?  If you have some test data for the use case and could
> > share it, it would be very helpful for me to understand why it is needed.
> 
> Sorry, this type just from my knowledge of MM, have no test data.
> 
> For futher learn of DAMON, I'll try it.

Yes, that will be very helpful.

And from this point, I'm getting an impression that the purpose of this RFC is
not for making a real change for specific use case that assumed to make real
benefits, but just for getting opinions about some imaginable changes that
_might_ be helpful, and _might_ be made in future.  If so, making the point
more clear would be helpful for me to give you opinion earlier.  If that's the
case, my opinion is this.  I agree DAMON code has many rooms of improvement in
terms of readability, so cleanup patches are welcome.  Nevertheless, I'd prefer
to make changes only when it is reasonable to expect it's providing _real_
improvement just after be applied, or at least very near and specific future.


Thanks,
SJ

> 
> >
> >> Do we need this and cleanup it?
> > I think I cannot answer for now, and your further clarification and patient
> > explanation could be helpful.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/Documentation/mm/damon/design.rst?h=mm-everything-2023-09-20-19-38#n400
> >
> >
> > Thanks,
> > SJ
> 
> Thanks,
> 
> Huan
> 
> >
> >> Huan Yang (2):
> >>    mm/damos/filter: Add workingset page filter
> >>    mm/damos/filter: Damos filter cleanup
> >>
> >>   include/linux/damon.h    |  62 +-----------------
> >>   mm/damon/Makefile        |   2 +-
> >>   mm/damon/core-test.h     |   7 ++
> >>   mm/damon/core.c          |  93 ++++-----------------------
> >>   mm/damon/filter.c        | 135 +++++++++++++++++++++++++++++++++++++++
> >>   mm/damon/filter.h        | 119 ++++++++++++++++++++++++++++++++++
> >>   mm/damon/paddr.c         |  29 +++------
> >>   mm/damon/reclaim.c       |  48 +++++++++++---
> >>   mm/damon/sysfs-schemes.c |   1 +
> >>   9 files changed, 325 insertions(+), 171 deletions(-)
> >>   create mode 100644 mm/damon/filter.c
> >>   create mode 100644 mm/damon/filter.h
> >>
> >> --
> >> 2.34.1
> >>
> >>
> 

Re: [RFC 0/2] Damos filter cleanup code and implement workingset

[杨欢]

HI SJ,
> Hi Huan,
>
> On Fri, 22 Sep 2023 02:54:58 +0000 杨欢 <link@vivo.com> wrote:
>
>> HI SJ,
>>
>> Thanks for your patient response.
> Happy to hear this kind acknowledgement :)
>
>>> [Some people who received this message don't often get email from sj@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> Hi Huan,
>>>
>>>
>>> First of all, thank you for this patch.  I have some high level comments and
>>> questions as below.
>>>
>>> On Tue, 19 Sep 2023 17:52:33 +0800 Huan Yang <link@vivo.com> wrote:
>>>
>>>> Now damos support filter contains two type.
>>>> The first is scheme filter which will invoke each scheme apply,
>>>> second is scheme action filter, which will filter out unwanted action.
>>> IMHO, that's not clear definition of the types.  Conceptually, all the filters
>>> are same.  Nonetheless, they are having different behaviors because they are
>>> handled in different layer depending on which layer is more efficient to handle
>> Thanks for these instructions to help me understand the design idea of
>> damos filter.
>>> those[1].
>>>
>>> I agree this is complicated and a bit verbose explanation, but I don't feel
>>> your scheme vs action definition is making it easier to understand.
>>>
>>>> But this implement is scattered in different implementations
>>> Indeed the implementation is scattered in core layer and the ops layer
>>> depending on the filter types.  But I think this is needed for efficient
>>> handling of those.
>> IMO, some simple filter can have a common implement, like anon filter? And,
>> for some special type, each layer offer their own?
> Do you mean there could be two filter types that better to be handled in
> different layer for efficiency, but share common implementation?  Could you
> please give me a more specific example?

It's hard to offer a specific example due to current ops implement is so
great to cover

much situation. Maybe Heterogeneous memory may add a new ops(just
examples of

imprudence, like intel's or other network memory?) . And offer a common
ops filter code

can may some simple type be reused.

>
>>>> and hard to reuse or extend.
>>>   From your first patch, which extending the filter, the essential change for the
>>> extension is adding another enum to 'enum damos_filter_type' (1 line) and
>>> addition of switch case in '__damos_pa_filter_out()' (3 lines).  I don't think
>>> it looks too complicated.  If you're saying it was hard to understand which
>> Yes, indeed.
>>> part really need to be modifed, I think that makes sense.  But in the case,
>>> we might need more comments rather than structural change.
>>>
>>>> This patchset clean up those filter code, move into filter.c and add header
>>>> to expose filter func.(patch 2)
>>> Again, I agree more code cleanup is needed.  But I'm not sure if this is the
>>> right way.  Especially, I'm unsure if it really need to have a dedicated file.
>> I think the filter code scatter into each layer may make code hard to
>> reuse, other ops
>>
>> may need anon filter or something. So, make code into a dedicated file
>> may good?
>>
>> (just my opinion.)
> Again, I'm not super confident about my understanding.  But sounds like you're
> partly worrying about duplication of some code in each ops implementation
> (modules in same layer).  Please correct me if I'm wrong, with specific,
> detailed and realistic examples.
>
> If my guess is not that wrong, I can agree to the concern.  Nevertheless, we
> already have a dedicated source file for such common code between ops
> implementaions, namely ops-common.c.
Yes, no need to split a single file to drive filter.
>
> That said, we don't have duplicated DAMOS filters implementation code in
> multipe ops implementation at the moment.  It could have such duplication in
> the future, but I think it's not too late to make such cleanup after or just
> before such duplication heppen.  IOW, I'd suggest to not make changes for
> something that _might_ happen in future.

Yes, indeed. We needn't to make this right now.(That's the why RFC,
meanwhile, my code is

not good.)

>
>>> To my humble eyes, it doesn't look like making code clearly easier to read
>>> compared to the current version.  This is probably because I don't feel your
>>> scheme vs action definition is clear to understand.  Neither it is
>> Yes, indeed, current code not clean, the idea didn't take shape.
>>> deduplicating code.  The patch adds lines more that deleted ones, according to
>>> its diffstat.  For the reason, I don't see clear benefit of this change.
>> In this code, maybe just a little benefit when other ops need to filter
>> anon page? :)
> As mentioned above, it's unclear when, how, and for what benefit we will
> support anon pages filter from vaddr.  I'd again suggest to not make change
> only for future change that not clear if we really want to make for now.
>
>>>     I
>>> might be biased, having NIH (Not Invented Here) sindrome, or missing something.
>>> Please let me know if so.
>>>
>>>> And add a new filter "workingset" to protect workingset page.
>>> Can you explain expected use cases of this new filter type in more detail?
>>> Specifically, for what scheme action and under what situation this new filter
>> IMO, page if marked as workingset, mean page in task's core
>> workload(maybe have
>>
>> seen the refault, and trigger refault protect). So, for lru sort or reclaim,
>>
>> we'd better not touch it?(If any wrong, please let me know.)
> Still unclear to me.  Could I ask you more specific and detailed case?

I may have misunderstood, I suggest you just look:

Page workingset flag will mark to page when page need to deactive or
refault and

find it already marked. In some memory path(migrate, reclaim or else.),
touch

workingset page require special attention.(Enter psi mm stall or else)

So, I think help filter out this is helpful.(Of course, just thought
experiment, no helpful data).

As, above and below. This RFC patch. :). I will share when get valuable
data.

>
>>> type will be needed?  If you have some test data for the use case and could
>>> share it, it would be very helpful for me to understand why it is needed.
>> Sorry, this type just from my knowledge of MM, have no test data.
>>
>> For futher learn of DAMON, I'll try it.
> Yes, that will be very helpful.
>
> And from this point, I'm getting an impression that the purpose of this RFC is
> not for making a real change for specific use case that assumed to make real
> benefits, but just for getting opinions about some imaginable changes that
> _might_ be helpful, and _might_ be made in future.  If so, making the point
Yes, I just learn DAMON a little time, and offer some thinking for this.
> more clear would be helpful for me to give you opinion earlier.  If that's the
> case, my opinion is this.  I agree DAMON code has many rooms of improvement in
> terms of readability, so cleanup patches are welcome.  Nevertheless, I'd prefer
> to make changes only when it is reasonable to expect it's providing _real_
> improvement just after be applied, or at least very near and specific future.
Yes, keep this and change when we need indeed. :)
>
>
> Thanks,
> SJ

Thans,

Huan

>>>> Do we need this and cleanup it?
>>> I think I cannot answer for now, and your further clarification and patient
>>> explanation could be helpful.
>>>
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/Documentation/mm/damon/design.rst?h=mm-everything-2023-09-20-19-38#n400
>>>
>>>
>>> Thanks,
>>> SJ
>> Thanks,
>>
>> Huan
>>
>>>> Huan Yang (2):
>>>>     mm/damos/filter: Add workingset page filter
>>>>     mm/damos/filter: Damos filter cleanup
>>>>
>>>>    include/linux/damon.h    |  62 +-----------------
>>>>    mm/damon/Makefile        |   2 +-
>>>>    mm/damon/core-test.h     |   7 ++
>>>>    mm/damon/core.c          |  93 ++++-----------------------
>>>>    mm/damon/filter.c        | 135 +++++++++++++++++++++++++++++++++++++++
>>>>    mm/damon/filter.h        | 119 ++++++++++++++++++++++++++++++++++
>>>>    mm/damon/paddr.c         |  29 +++------
>>>>    mm/damon/reclaim.c       |  48 +++++++++++---
>>>>    mm/damon/sysfs-schemes.c |   1 +
>>>>    9 files changed, 325 insertions(+), 171 deletions(-)
>>>>    create mode 100644 mm/damon/filter.c
>>>>    create mode 100644 mm/damon/filter.h
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>>>

Re: [RFC 0/2] Damos filter cleanup code and implement workingset

[SeongJae Park]

Hi Huan,

On Mon, 25 Sep 2023 02:10:58 +0000 杨欢 <link@vivo.com> wrote:

> HI SJ,
> > Hi Huan,
> >
> > On Fri, 22 Sep 2023 02:54:58 +0000 杨欢 <link@vivo.com> wrote:
> >
> >> HI SJ,
> >>
> >> Thanks for your patient response.
> > Happy to hear this kind acknowledgement :)
> >
> >>> [Some people who received this message don't often get email from sj@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >>>
> >>> Hi Huan,
> >>>
> >>>
> >>> First of all, thank you for this patch.  I have some high level comments and
> >>> questions as below.
> >>>
> >>> On Tue, 19 Sep 2023 17:52:33 +0800 Huan Yang <link@vivo.com> wrote:
> >>>
> >>>> Now damos support filter contains two type.
> >>>> The first is scheme filter which will invoke each scheme apply,
> >>>> second is scheme action filter, which will filter out unwanted action.
> >>> IMHO, that's not clear definition of the types.  Conceptually, all the filters
> >>> are same.  Nonetheless, they are having different behaviors because they are
> >>> handled in different layer depending on which layer is more efficient to handle
> >> Thanks for these instructions to help me understand the design idea of
> >> damos filter.
> >>> those[1].
> >>>
> >>> I agree this is complicated and a bit verbose explanation, but I don't feel
> >>> your scheme vs action definition is making it easier to understand.
> >>>
> >>>> But this implement is scattered in different implementations
> >>> Indeed the implementation is scattered in core layer and the ops layer
> >>> depending on the filter types.  But I think this is needed for efficient
> >>> handling of those.
> >> IMO, some simple filter can have a common implement, like anon filter? And,
> >> for some special type, each layer offer their own?
> > Do you mean there could be two filter types that better to be handled in
> > different layer for efficiency, but share common implementation?  Could you
> > please give me a more specific example?
> 
> It's hard to offer a specific example due to current ops implement is so
> great to cover
> 
> much situation. Maybe Heterogeneous memory may add a new ops(just
> examples of
> 
> imprudence, like intel's or other network memory?) . And offer a common
> ops filter code
> 
> can may some simple type be reused.
> 
> >
> >>>> and hard to reuse or extend.
> >>>   From your first patch, which extending the filter, the essential change for the
> >>> extension is adding another enum to 'enum damos_filter_type' (1 line) and
> >>> addition of switch case in '__damos_pa_filter_out()' (3 lines).  I don't think
> >>> it looks too complicated.  If you're saying it was hard to understand which
> >> Yes, indeed.
> >>> part really need to be modifed, I think that makes sense.  But in the case,
> >>> we might need more comments rather than structural change.
> >>>
> >>>> This patchset clean up those filter code, move into filter.c and add header
> >>>> to expose filter func.(patch 2)
> >>> Again, I agree more code cleanup is needed.  But I'm not sure if this is the
> >>> right way.  Especially, I'm unsure if it really need to have a dedicated file.
> >> I think the filter code scatter into each layer may make code hard to
> >> reuse, other ops
> >>
> >> may need anon filter or something. So, make code into a dedicated file
> >> may good?
> >>
> >> (just my opinion.)
> > Again, I'm not super confident about my understanding.  But sounds like you're
> > partly worrying about duplication of some code in each ops implementation
> > (modules in same layer).  Please correct me if I'm wrong, with specific,
> > detailed and realistic examples.
> >
> > If my guess is not that wrong, I can agree to the concern.  Nevertheless, we
> > already have a dedicated source file for such common code between ops
> > implementaions, namely ops-common.c.
> Yes, no need to split a single file to drive filter.
> >
> > That said, we don't have duplicated DAMOS filters implementation code in
> > multipe ops implementation at the moment.  It could have such duplication in
> > the future, but I think it's not too late to make such cleanup after or just
> > before such duplication heppen.  IOW, I'd suggest to not make changes for
> > something that _might_ happen in future.
> 
> Yes, indeed. We needn't to make this right now.(That's the why RFC,
> meanwhile, my code is
> 
> not good.)
> 
> >
> >>> To my humble eyes, it doesn't look like making code clearly easier to read
> >>> compared to the current version.  This is probably because I don't feel your
> >>> scheme vs action definition is clear to understand.  Neither it is
> >> Yes, indeed, current code not clean, the idea didn't take shape.
> >>> deduplicating code.  The patch adds lines more that deleted ones, according to
> >>> its diffstat.  For the reason, I don't see clear benefit of this change.
> >> In this code, maybe just a little benefit when other ops need to filter
> >> anon page? :)
> > As mentioned above, it's unclear when, how, and for what benefit we will
> > support anon pages filter from vaddr.  I'd again suggest to not make change
> > only for future change that not clear if we really want to make for now.
> >
> >>>     I
> >>> might be biased, having NIH (Not Invented Here) sindrome, or missing something.
> >>> Please let me know if so.
> >>>
> >>>> And add a new filter "workingset" to protect workingset page.
> >>> Can you explain expected use cases of this new filter type in more detail?
> >>> Specifically, for what scheme action and under what situation this new filter
> >> IMO, page if marked as workingset, mean page in task's core
> >> workload(maybe have
> >>
> >> seen the refault, and trigger refault protect). So, for lru sort or reclaim,
> >>
> >> we'd better not touch it?(If any wrong, please let me know.)
> > Still unclear to me.  Could I ask you more specific and detailed case?
> 
> I may have misunderstood, I suggest you just look:
> 
> Page workingset flag will mark to page when page need to deactive or
> refault and
> 
> find it already marked. In some memory path(migrate, reclaim or else.),
> touch
> 
> workingset page require special attention.(Enter psi mm stall or else)
> 
> So, I think help filter out this is helpful.(Of course, just thought
> experiment, no helpful data).
> 
> As, above and below. This RFC patch. :). I will share when get valuable
> data.
> 
> >
> >>> type will be needed?  If you have some test data for the use case and could
> >>> share it, it would be very helpful for me to understand why it is needed.
> >> Sorry, this type just from my knowledge of MM, have no test data.
> >>
> >> For futher learn of DAMON, I'll try it.
> > Yes, that will be very helpful.
> >
> > And from this point, I'm getting an impression that the purpose of this RFC is
> > not for making a real change for specific use case that assumed to make real
> > benefits, but just for getting opinions about some imaginable changes that
> > _might_ be helpful, and _might_ be made in future.  If so, making the point
> Yes, I just learn DAMON a little time, and offer some thinking for this.
> > more clear would be helpful for me to give you opinion earlier.  If that's the
> > case, my opinion is this.  I agree DAMON code has many rooms of improvement in
> > terms of readability, so cleanup patches are welcome.  Nevertheless, I'd prefer
> > to make changes only when it is reasonable to expect it's providing _real_
> > improvement just after be applied, or at least very near and specific future.
> Yes, keep this and change when we need indeed. :)

Ok, makes sense.  Let's do further discussion after some more data and
realistic detailed example be available. :)  Looking forward to!


Thanks,
SJ

> >
> >
> > Thanks,
> > SJ
> 
> Thans,
> 
> Huan
> 
> >>>> Do we need this and cleanup it?
> >>> I think I cannot answer for now, and your further clarification and patient
> >>> explanation could be helpful.
> >>>
> >>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/Documentation/mm/damon/design.rst?h=mm-everything-2023-09-20-19-38#n400
> >>>
> >>>
> >>> Thanks,
> >>> SJ
> >> Thanks,
> >>
> >> Huan
> >>
> >>>> Huan Yang (2):
> >>>>     mm/damos/filter: Add workingset page filter
> >>>>     mm/damos/filter: Damos filter cleanup
> >>>>
> >>>>    include/linux/damon.h    |  62 +-----------------
> >>>>    mm/damon/Makefile        |   2 +-
> >>>>    mm/damon/core-test.h     |   7 ++
> >>>>    mm/damon/core.c          |  93 ++++-----------------------
> >>>>    mm/damon/filter.c        | 135 +++++++++++++++++++++++++++++++++++++++
> >>>>    mm/damon/filter.h        | 119 ++++++++++++++++++++++++++++++++++
> >>>>    mm/damon/paddr.c         |  29 +++------
> >>>>    mm/damon/reclaim.c       |  48 +++++++++++---
> >>>>    mm/damon/sysfs-schemes.c |   1 +
> >>>>    9 files changed, 325 insertions(+), 171 deletions(-)
> >>>>    create mode 100644 mm/damon/filter.c
> >>>>    create mode 100644 mm/damon/filter.h
> >>>>
> >>>> --
> >>>> 2.34.1
> >>>>
> >>>>