All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/4] mm/ksm: Add ksm advisor
@ 2023-12-04 23:49 Stefan Roesch
  2023-12-04 23:49 ` [PATCH v3 1/4] mm/ksm: add " Stefan Roesch
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Stefan Roesch @ 2023-12-04 23:49 UTC (permalink / raw)
  To: kernel-team; +Cc: shr, akpm, david, hannes, riel, linux-kernel, linux-mm

What is the KSM advisor?
=========================
The ksm advisor automatically manages the pages_to_scan setting to
achieve a target scan time. The target scan time defines how many seconds
it should take to scan all the candidate KSM pages. In other words the
pages_to_scan rate is changed by the advisor to achieve the target scan
time.

Why do we need a KSM advisor?
==============================
The number of candidate pages for KSM is dynamic. It can often be observed
that during the startup of an application more candidate pages need to be
processed. Without an advisor the pages_to_scan parameter needs to be
sized for the maximum number of candidate pages. With the scan time
advisor the pages_to_scan parameter based can be changed based on demand.

Algorithm
==========
The algorithm calculates the change value based on the target scan time
and the previous scan time. To avoid pertubations an exponentially
weighted moving average is applied.

The algorithm has a max and min
value to:
- guarantee responsiveness to changes
- to limit CPU resource consumption

Parameters to influence the KSM scan advisor
=============================================
The respective parameters are:
- ksm_advisor_mode
  0: None (default), 1: scan time advisor
- ksm_advisor_target_scan_time
  how many seconds a scan should of all candidate pages take
- ksm_advisor_max_cpu
  upper limit for the cpu usage in percent of the ksmd background thread

The initial value and the max value for the pages_to_scan parameter can
be limited with:
- ksm_advisor_min_pages
  minimum value for pages_to_scan per batch
- ksm_advisor_max_pages
  maximum value for pages_to_scan per batch

The default settings for the above two parameters should be suitable for
most workloads.

The parameters are exposed as knobs in /sys/kernel/mm/ksm. By default the
scan time advisor is disabled.

Currently there are two advisors:
- none and
- scan-time.

Resource savings
=================
Tests with various workloads have shown considerable CPU savings. Most
of the workloads I have investigated have more candidate pages during
startup. Once the workload is stable in terms of memory, the number of
candidate pages is reduced. Without the advisor, the pages_to_scan needs
to be sized for the maximum number of candidate pages. So having this
advisor definitely helps in reducing CPU consumption.

For the instagram workload, the advisor achieves a 25% CPU reduction.
Once the memory is stable, the pages_to_scan parameter gets reduced to
about 40% of its max value.

The new advisor works especially well if the smart scan feature is also
enabled.

How is defining a target scan time better?
===========================================
For an administrator it is more logical to set a target scan time.. The
administrator can determine how many pages are scanned on each scan.
Therefore setting a target scan time makes more sense.

In addition the administrator might have a good idea about the memory
sizing of its respective workloads.

Setting cpu limits is easier than setting The pages_to_scan parameter. The
pages_to_scan parameter is per batch. For the administrator it is difficult
to set the pages_to_scan parameter.

Tracing
=======
A new tracing event has been added for the scan time advisor. The new
trace event is called ksm_advisor. It reports the scan time, the new
pages_to_scan setting and the cpu usage of the ksmd background thread.

Other approaches
=================

Approach 1: Adapt pages_to_scan after processing each batch. If KSM
  merges pages, increase the scan rate, if less KSM pages, reduce the
  the pages_to_scan rate. This doesn't work too well. While it increases
  the pages_to_scan for a short period, but generally it ends up with a
  too low pages_to_scan rate.

Approach 2: Adapt pages_to_scan after each scan. The problem with that
  approach is that the calculated scan rate tends to be high. The more
  aggressive KSM scans, the more pages it can de-duplicate.

There have been earlier attempts at an advisor:
  propose auto-run mode of ksm and its tests
  (https://marc.info/?l=linux-mm&m=166029880214485&w=2)


Changes:
========
V3:
  - Use string parameters for advisor mode
  - Removed min cpu load sysfs knob
  - dropped unused enums in ksm_advisor_type
  - renamed KSM_ADVISOR_LAST to KSM_ADVISOR_COUNT
  - init_advisor() is needed but changed how it is initialized
  - don't allow to change pages_to_scan parameter when scan-time advisor
    is enabled
  - add ksm_advisor_start_scan() and ksm_advisor_stop_scan() functions
    to calculate scan time
  - removed scan time parameter to scan_time_advisor() function

V2:
  - Use functions for long long calculations to support 32 bit platforms
  - Use cpu min and cpu max settings for the advisor instead of the pages
    min and max parameters.
  - pages min and max values are now used for the initial and max values.
    Generally they are not required to be changed.
  - Add cpu percent usage value to tracepoint definition
  - Update documentation for cpu min and cpu max values 
  - Update commit messages for the above changes



*** BLURB HERE ***

Stefan Roesch (4):
  mm/ksm: add ksm advisor
  mm/ksm: add sysfs knobs for advisor
  mm/ksm: add tracepoint for ksm advisor
  mm/ksm: document ksm advisor and its sysfs knobs

 Documentation/admin-guide/mm/ksm.rst |  54 +++++
 include/trace/events/ksm.h           |  33 +++
 mm/ksm.c                             | 303 ++++++++++++++++++++++++++-
 3 files changed, 389 insertions(+), 1 deletion(-)


base-commit: 12d04a7bf0da67321229d2bc8b1a7074d65415a9
-- 
2.39.3


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 1/4] mm/ksm: add ksm advisor
  2023-12-04 23:49 [PATCH v3 0/4] mm/ksm: Add ksm advisor Stefan Roesch
@ 2023-12-04 23:49 ` Stefan Roesch
  2023-12-06 15:17   ` kernel test robot
  2023-12-12 13:44   ` David Hildenbrand
  2023-12-04 23:49 ` [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor Stefan Roesch
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 16+ messages in thread
From: Stefan Roesch @ 2023-12-04 23:49 UTC (permalink / raw)
  To: kernel-team; +Cc: shr, akpm, david, hannes, riel, linux-kernel, linux-mm

This adds the ksm advisor. The ksm advisor automatically manages the
pages_to_scan setting to achieve a target scan time. The target scan
time defines how many seconds it should take to scan all the candidate
KSM pages. In other words the pages_to_scan rate is changed by the
advisor to achieve the target scan time. The algorithm has a max and min
value to:
- guarantee responsiveness to changes
- limit CPU resource consumption

The respective parameters are:
- ksm_advisor_target_scan_time (how many seconds a scan should take)
- ksm_advisor_max_cpu (maximum value for cpu percent usage)

- ksm_advisor_min_pages (minimum value for pages_to_scan per batch)
- ksm_advisor_max_pages (maximum value for pages_to_scan per batch)

The algorithm calculates the change value based on the target scan time
and the previous scan time. To avoid pertubations an exponentially
weighted moving average is applied.

The advisor is managed by two main parameters: target scan time,
cpu max time for the ksmd background thread. These parameters determine
how aggresive ksmd scans.

In addition there are min and max values for the pages_to_scan parameter
to make sure that its initial and max values are not set too low or too
high. This ensures that it is able to react to changes quickly enough.

The default values are:
- target scan time: 200 secs
- max cpu: 70%
- min pages: 500
- max pages: 30000

By default the advisor is disabled. Currently there are two advisors:
none and scan-time.

Tests with various workloads have shown considerable CPU savings. Most
of the workloads I have investigated have more candidate pages during
startup, once the workload is stable in terms of memory, the number of
candidate pages is reduced. Without the advisor, the pages_to_scan needs
to be sized for the maximum number of candidate pages. So having this
advisor definitely helps in reducing CPU consumption.

For the instagram workload, the advisor achieves a 25% CPU reduction.
Once the memory is stable, the pages_to_scan parameter gets reduced to
about 40% of its max value.

Signed-off-by: Stefan Roesch <shr@devkernel.io>
---
 mm/ksm.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 174 insertions(+), 1 deletion(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 7efcc68ccc6ea..b27010fa2e946 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -21,6 +21,7 @@
 #include <linux/sched.h>
 #include <linux/sched/mm.h>
 #include <linux/sched/coredump.h>
+#include <linux/sched/cputime.h>
 #include <linux/rwsem.h>
 #include <linux/pagemap.h>
 #include <linux/rmap.h>
@@ -248,6 +249,9 @@ static struct kmem_cache *rmap_item_cache;
 static struct kmem_cache *stable_node_cache;
 static struct kmem_cache *mm_slot_cache;
 
+/* Default number of pages to scan per batch */
+#define DEFAULT_PAGES_TO_SCAN 100
+
 /* The number of pages scanned */
 static unsigned long ksm_pages_scanned;
 
@@ -276,7 +280,7 @@ static unsigned int ksm_stable_node_chains_prune_millisecs = 2000;
 static int ksm_max_page_sharing = 256;
 
 /* Number of pages ksmd should scan in one batch */
-static unsigned int ksm_thread_pages_to_scan = 100;
+static unsigned int ksm_thread_pages_to_scan = DEFAULT_PAGES_TO_SCAN;
 
 /* Milliseconds ksmd should sleep between batches */
 static unsigned int ksm_thread_sleep_millisecs = 20;
@@ -297,6 +301,168 @@ unsigned long ksm_zero_pages;
 /* The number of pages that have been skipped due to "smart scanning" */
 static unsigned long ksm_pages_skipped;
 
+/* Don't scan more than max pages per batch. */
+static unsigned long ksm_advisor_max_pages = 30000;
+
+/* At least scan this many pages per batch. */
+static unsigned long ksm_advisor_min_pages = 500;
+
+/* Min CPU for scanning pages per scan */
+static unsigned int ksm_advisor_min_cpu =  10;
+
+/* Max CPU for scanning pages per scan */
+static unsigned int ksm_advisor_max_cpu =  70;
+
+/* Target scan time in seconds to analyze all KSM candidate pages. */
+static unsigned long ksm_advisor_target_scan_time = 200;
+
+/* Exponentially weighted moving average. */
+#define EWMA_WEIGHT 30
+
+/**
+ * struct advisor_ctx - metadata for KSM advisor
+ * @start_scan: start time of the current scan
+ * @scan_time: scan time of previous scan
+ * @change: change in percent to pages_to_scan parameter
+ * @cpu_time: cpu time consumed by the ksmd thread in the previous scan
+ */
+struct advisor_ctx {
+	ktime_t start_scan;
+	unsigned long scan_time;
+	unsigned long change;
+	unsigned long long cpu_time;
+};
+static struct advisor_ctx advisor_ctx;
+
+/* Define different advisor's */
+enum ksm_advisor_type {
+	KSM_ADVISOR_NONE,
+	KSM_ADVISOR_SCAN_TIME,
+};
+static enum ksm_advisor_type ksm_advisor;
+
+static void init_advisor(void)
+{
+	advisor_ctx = (const struct advisor_ctx){ 0 };
+}
+
+static void set_advisor_defaults(void)
+{
+	if (ksm_advisor == KSM_ADVISOR_NONE)
+		ksm_thread_pages_to_scan = DEFAULT_PAGES_TO_SCAN;
+	else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
+		ksm_thread_pages_to_scan = ksm_advisor_min_pages;
+}
+
+static inline void advisor_start_scan(void)
+{
+	if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
+		advisor_ctx.start_scan = ktime_get();
+}
+
+static inline s64 advisor_stop_scan(void)
+{
+	return ktime_ms_delta(ktime_get(), advisor_ctx.start_scan);
+}
+
+/*
+ * Use previous scan time if available, otherwise use current scan time as an
+ * approximation for the previous scan time.
+ */
+static inline unsigned long prev_scan_time(struct advisor_ctx *ctx,
+					   unsigned long scan_time)
+{
+	return ctx->scan_time ? ctx->scan_time : scan_time;
+}
+
+/* Calculate exponential weighted moving average */
+static unsigned long ewma(unsigned long prev, unsigned long curr)
+{
+	return ((100 - EWMA_WEIGHT) * prev + EWMA_WEIGHT * curr) / 100;
+}
+
+/*
+ * The scan time advisor is based on the current scan rate and the target
+ * scan rate.
+ *
+ *      new_pages_to_scan = pages_to_scan * (scan_time / target_scan_time)
+ *
+ * To avoid pertubations it calculates a change factor of previous changes.
+ * A new change factor is calculated for each iteration and it uses an
+ * exponentially weighted moving average. The new pages_to_scan value is
+ * multiplied with that change factor:
+ *
+ *      new_pages_to_scan *= change facor
+ *
+ * In addition the new pages_to_scan value is capped by the max and min
+ * limits.
+ */
+static void scan_time_advisor(void)
+{
+	unsigned int cpu_percent;
+	unsigned long cpu_time;
+	unsigned long cpu_time_diff;
+	unsigned long cpu_time_diff_ms;
+	unsigned long pages;
+	unsigned long per_page_cost;
+	unsigned long factor;
+	unsigned long change;
+	unsigned long last_scan_time;
+	s64 scan_time;
+
+	/* Convert scan time to seconds */
+	scan_time = advisor_stop_scan();
+	scan_time = div_s64(scan_time, MSEC_PER_SEC);
+	scan_time = scan_time ? scan_time : 1;
+
+	/* Calculate CPU consumption of ksmd background thread */
+	cpu_time = task_sched_runtime(current);
+	cpu_time_diff = cpu_time - advisor_ctx.cpu_time;
+	cpu_time_diff_ms = cpu_time_diff / 1000 / 1000;
+
+	cpu_percent = (cpu_time_diff_ms * 100) / (scan_time * 1000);
+	cpu_percent = cpu_percent ? cpu_percent : 1;
+	last_scan_time = prev_scan_time(&advisor_ctx, scan_time);
+
+	/* Calculate scan time as percentage of target scan time */
+	factor = ksm_advisor_target_scan_time * 100 / scan_time;
+	factor = factor ? factor : 1;
+
+	/*
+	 * Calculate scan time as percentage of last scan time and use
+	 * exponentially weighted average to smooth it
+	 */
+	change = scan_time * 100 / last_scan_time;
+	change = change ? change : 1;
+	change = ewma(advisor_ctx.change, change);
+
+	/* Calculate new scan rate based on target scan rate. */
+	pages = ksm_thread_pages_to_scan * 100 / factor;
+	/* Update pages_to_scan by weighted change percentage. */
+	pages = pages * change / 100;
+
+	/* Cap new pages_to_scan value */
+	per_page_cost = ksm_thread_pages_to_scan / cpu_percent;
+	per_page_cost = per_page_cost ? per_page_cost : 1;
+
+	pages = min(pages, per_page_cost * ksm_advisor_max_cpu);
+	pages = max(pages, per_page_cost * ksm_advisor_min_cpu);
+	pages = min(pages, ksm_advisor_max_pages);
+
+	/* Update advisor context */
+	advisor_ctx.change = change;
+	advisor_ctx.scan_time = scan_time;
+	advisor_ctx.cpu_time = cpu_time;
+
+	ksm_thread_pages_to_scan = pages;
+}
+
+static void run_advisor(void)
+{
+	if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
+		scan_time_advisor();
+}
+
 #ifdef CONFIG_NUMA
 /* Zeroed when merging across nodes is not allowed */
 static unsigned int ksm_merge_across_nodes = 1;
@@ -2401,6 +2567,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 
 	mm_slot = ksm_scan.mm_slot;
 	if (mm_slot == &ksm_mm_head) {
+		advisor_start_scan();
 		trace_ksm_start_scan(ksm_scan.seqnr, ksm_rmap_items);
 
 		/*
@@ -2558,6 +2725,8 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 	if (mm_slot != &ksm_mm_head)
 		goto next_mm;
 
+	run_advisor();
+
 	trace_ksm_stop_scan(ksm_scan.seqnr, ksm_rmap_items);
 	ksm_scan.seqnr++;
 	return NULL;
@@ -3244,6 +3413,9 @@ static ssize_t pages_to_scan_store(struct kobject *kobj,
 	unsigned int nr_pages;
 	int err;
 
+	if (ksm_advisor != KSM_ADVISOR_NONE)
+		return -EINVAL;
+
 	err = kstrtouint(buf, 10, &nr_pages);
 	if (err)
 		return -EINVAL;
@@ -3603,6 +3775,7 @@ static int __init ksm_init(void)
 	zero_checksum = calc_checksum(ZERO_PAGE(0));
 	/* Default to false for backwards compatibility */
 	ksm_use_zero_pages = false;
+	init_advisor();
 
 	err = ksm_slab_init();
 	if (err)
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor
  2023-12-04 23:49 [PATCH v3 0/4] mm/ksm: Add ksm advisor Stefan Roesch
  2023-12-04 23:49 ` [PATCH v3 1/4] mm/ksm: add " Stefan Roesch
@ 2023-12-04 23:49 ` Stefan Roesch
  2023-12-12 13:45   ` David Hildenbrand
  2023-12-04 23:49 ` [PATCH v3 3/4] mm/ksm: add tracepoint for ksm advisor Stefan Roesch
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Stefan Roesch @ 2023-12-04 23:49 UTC (permalink / raw)
  To: kernel-team; +Cc: shr, akpm, david, hannes, riel, linux-kernel, linux-mm

This adds four new knobs for the KSM advisor to influence its behaviour.

The knobs are:
- advisor_mode:
    none:      no advisor (default)
    scan-time: scan time advisor
- advisor_max_cpu: 70 (default, cpu usage percent)
- advisor_min_pages: 500 (default)
- advisor_max_pages: 30000 (default)
- advisor_target_scan_time: 200 (default in seconds)

The new values will take effect on the next scan round.

Signed-off-by: Stefan Roesch <shr@devkernel.io>
---
 mm/ksm.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/mm/ksm.c b/mm/ksm.c
index b27010fa2e946..18b7185bbc65b 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -3735,6 +3735,128 @@ static ssize_t smart_scan_store(struct kobject *kobj,
 }
 KSM_ATTR(smart_scan);
 
+static ssize_t advisor_mode_show(struct kobject *kobj,
+				 struct kobj_attribute *attr, char *buf)
+{
+	const char *output;
+
+	if (ksm_advisor == KSM_ADVISOR_NONE)
+		output = "[none] scan-time";
+	else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
+		output = "none [scan-time]";
+
+	return sysfs_emit(buf, "%s\n", output);
+}
+
+static ssize_t advisor_mode_store(struct kobject *kobj,
+				  struct kobj_attribute *attr, const char *buf,
+				  size_t count)
+{
+	if (sysfs_streq("scan-time", buf))
+		ksm_advisor = KSM_ADVISOR_SCAN_TIME;
+	else if (sysfs_streq("none", buf))
+		ksm_advisor = KSM_ADVISOR_NONE;
+	else
+		return -EINVAL;
+
+	/* Set advisor default values */
+	init_advisor();
+	set_advisor_defaults();
+
+	return count;
+}
+KSM_ATTR(advisor_mode);
+
+static ssize_t advisor_max_cpu_show(struct kobject *kobj,
+				    struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", ksm_advisor_max_cpu);
+}
+
+static ssize_t advisor_max_cpu_store(struct kobject *kobj,
+				     struct kobj_attribute *attr,
+				     const char *buf, size_t count)
+{
+	int err;
+	unsigned long value;
+
+	err = kstrtoul(buf, 10, &value);
+	if (err)
+		return -EINVAL;
+
+	ksm_advisor_max_cpu = value;
+	return count;
+}
+KSM_ATTR(advisor_max_cpu);
+
+static ssize_t advisor_min_pages_show(struct kobject *kobj,
+				      struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%lu\n", ksm_advisor_min_pages);
+}
+
+static ssize_t advisor_min_pages_store(struct kobject *kobj,
+				       struct kobj_attribute *attr,
+				       const char *buf, size_t count)
+{
+	int err;
+	unsigned long value;
+
+	err = kstrtoul(buf, 10, &value);
+	if (err)
+		return -EINVAL;
+
+	ksm_advisor_min_pages = value;
+	return count;
+}
+KSM_ATTR(advisor_min_pages);
+
+static ssize_t advisor_max_pages_show(struct kobject *kobj,
+				      struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%lu\n", ksm_advisor_max_pages);
+}
+
+static ssize_t advisor_max_pages_store(struct kobject *kobj,
+				       struct kobj_attribute *attr,
+				       const char *buf, size_t count)
+{
+	int err;
+	unsigned long value;
+
+	err = kstrtoul(buf, 10, &value);
+	if (err)
+		return -EINVAL;
+
+	ksm_advisor_max_pages = value;
+	return count;
+}
+KSM_ATTR(advisor_max_pages);
+
+static ssize_t advisor_target_scan_time_show(struct kobject *kobj,
+					     struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%lu\n", ksm_advisor_target_scan_time);
+}
+
+static ssize_t advisor_target_scan_time_store(struct kobject *kobj,
+					      struct kobj_attribute *attr,
+					      const char *buf, size_t count)
+{
+	int err;
+	unsigned long value;
+
+	err = kstrtoul(buf, 10, &value);
+	if (err)
+		return -EINVAL;
+	if (value < 1)
+		return -EINVAL;
+
+	ksm_advisor_target_scan_time = value;
+	return count;
+}
+KSM_ATTR(advisor_target_scan_time);
+
 static struct attribute *ksm_attrs[] = {
 	&sleep_millisecs_attr.attr,
 	&pages_to_scan_attr.attr,
@@ -3757,6 +3879,11 @@ static struct attribute *ksm_attrs[] = {
 	&use_zero_pages_attr.attr,
 	&general_profit_attr.attr,
 	&smart_scan_attr.attr,
+	&advisor_mode_attr.attr,
+	&advisor_max_cpu_attr.attr,
+	&advisor_min_pages_attr.attr,
+	&advisor_max_pages_attr.attr,
+	&advisor_target_scan_time_attr.attr,
 	NULL,
 };
 
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 3/4] mm/ksm: add tracepoint for ksm advisor
  2023-12-04 23:49 [PATCH v3 0/4] mm/ksm: Add ksm advisor Stefan Roesch
  2023-12-04 23:49 ` [PATCH v3 1/4] mm/ksm: add " Stefan Roesch
  2023-12-04 23:49 ` [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor Stefan Roesch
@ 2023-12-04 23:49 ` Stefan Roesch
  2023-12-12 13:45   ` David Hildenbrand
  2023-12-04 23:49 ` [PATCH v3 4/4] mm/ksm: document ksm advisor and its sysfs knobs Stefan Roesch
  2023-12-05 20:46 ` [PATCH v3 0/4] mm/ksm: Add ksm advisor Andrew Morton
  4 siblings, 1 reply; 16+ messages in thread
From: Stefan Roesch @ 2023-12-04 23:49 UTC (permalink / raw)
  To: kernel-team; +Cc: shr, akpm, david, hannes, riel, linux-kernel, linux-mm

This adds a new tracepoint for the ksm advisor. It reports the last scan
time, the new setting of the pages_to_scan parameter and the average cpu
percent usage of the ksmd background thread for the last scan.

Signed-off-by: Stefan Roesch <shr@devkernel.io>
---
 include/trace/events/ksm.h | 33 +++++++++++++++++++++++++++++++++
 mm/ksm.c                   |  1 +
 2 files changed, 34 insertions(+)

diff --git a/include/trace/events/ksm.h b/include/trace/events/ksm.h
index b5ac35c1d0e88..e728647b5d268 100644
--- a/include/trace/events/ksm.h
+++ b/include/trace/events/ksm.h
@@ -245,6 +245,39 @@ TRACE_EVENT(ksm_remove_rmap_item,
 			__entry->pfn, __entry->rmap_item, __entry->mm)
 );
 
+/**
+ * ksm_advisor - called after the advisor has run
+ *
+ * @scan_time:		scan time in seconds
+ * @pages_to_scan:	new pages_to_scan value
+ * @cpu_percent:	cpu usage in percent
+ *
+ * Allows to trace the ksm advisor.
+ */
+TRACE_EVENT(ksm_advisor,
+
+	TP_PROTO(s64 scan_time, unsigned long pages_to_scan,
+		 unsigned int cpu_percent),
+
+	TP_ARGS(scan_time, pages_to_scan, cpu_percent),
+
+	TP_STRUCT__entry(
+		__field(s64,		scan_time)
+		__field(unsigned long,	pages_to_scan)
+		__field(unsigned int,	cpu_percent)
+	),
+
+	TP_fast_assign(
+		__entry->scan_time	= scan_time;
+		__entry->pages_to_scan	= pages_to_scan;
+		__entry->cpu_percent	= cpu_percent;
+	),
+
+	TP_printk("ksm scan time %lld pages_to_scan %lu cpu percent %u",
+			__entry->scan_time, __entry->pages_to_scan,
+			__entry->cpu_percent)
+);
+
 #endif /* _TRACE_KSM_H */
 
 /* This part must be outside protection */
diff --git a/mm/ksm.c b/mm/ksm.c
index 18b7185bbc65b..b8065c5d5f833 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -455,6 +455,7 @@ static void scan_time_advisor(void)
 	advisor_ctx.cpu_time = cpu_time;
 
 	ksm_thread_pages_to_scan = pages;
+	trace_ksm_advisor(scan_time, pages, cpu_percent);
 }
 
 static void run_advisor(void)
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 4/4] mm/ksm: document ksm advisor and its sysfs knobs
  2023-12-04 23:49 [PATCH v3 0/4] mm/ksm: Add ksm advisor Stefan Roesch
                   ` (2 preceding siblings ...)
  2023-12-04 23:49 ` [PATCH v3 3/4] mm/ksm: add tracepoint for ksm advisor Stefan Roesch
@ 2023-12-04 23:49 ` Stefan Roesch
  2023-12-12 13:46   ` David Hildenbrand
  2023-12-05 20:46 ` [PATCH v3 0/4] mm/ksm: Add ksm advisor Andrew Morton
  4 siblings, 1 reply; 16+ messages in thread
From: Stefan Roesch @ 2023-12-04 23:49 UTC (permalink / raw)
  To: kernel-team; +Cc: shr, akpm, david, hannes, riel, linux-kernel, linux-mm

This documents the KSM advisor and its new knobs in /sys/fs/kernel/mm.

Signed-off-by: Stefan Roesch <shr@devkernel.io>
---
 Documentation/admin-guide/mm/ksm.rst | 54 ++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/Documentation/admin-guide/mm/ksm.rst b/Documentation/admin-guide/mm/ksm.rst
index e59231ac6bb71..297c4a0d04dee 100644
--- a/Documentation/admin-guide/mm/ksm.rst
+++ b/Documentation/admin-guide/mm/ksm.rst
@@ -80,6 +80,9 @@ pages_to_scan
         how many pages to scan before ksmd goes to sleep
         e.g. ``echo 100 > /sys/kernel/mm/ksm/pages_to_scan``.
 
+        The pages_to_scan value cannot be changed if ``advisor_mode`` has
+        been set to scan-time.
+
         Default: 100 (chosen for demonstration purposes)
 
 sleep_millisecs
@@ -164,6 +167,29 @@ smart_scan
         optimization is enabled.  The ``pages_skipped`` metric shows how
         effective the setting is.
 
+advisor_mode
+        The ``advisor_mode`` selects the current advisor. Two modes are
+        supported: none and scan-time. The default is none. By setting
+        ``advisor_mode`` to scan-time, the scan time advisor is enabled.
+        The section about ``advisor`` explains in detail how the scan time
+        advisor works.
+
+adivsor_max_cpu
+        specifies the upper limit of the cpu percent usage of the ksmd
+        background thread. The default is 70.
+
+advisor_target_scan_time
+        specifies the target scan time in seconds to scan all the candidate
+        pages. The default value is 200 seconds.
+
+advisor_min_pages
+        specifies the lower limit of the ``pages_to_scan`` parameter of the
+        scan time advisor. The default is 500.
+
+adivsor_max_pages
+        specifies the upper limit of the ``pages_to_scan`` parameter of the
+        scan time advisor. The default is 30000.
+
 The effectiveness of KSM and MADV_MERGEABLE is shown in ``/sys/kernel/mm/ksm/``:
 
 general_profit
@@ -263,6 +289,34 @@ ksm_swpin_copy
 	note that KSM page might be copied when swapping in because do_swap_page()
 	cannot do all the locking needed to reconstitute a cross-anon_vma KSM page.
 
+Advisor
+=======
+
+The number of candidate pages for KSM is dynamic. It can be often observed
+that during the startup of an application more candidate pages need to be
+processed. Without an advisor the ``pages_to_scan`` parameter needs to be
+sized for the maximum number of candidate pages. The scan time advisor can
+changes the ``pages_to_scan`` parameter based on demand.
+
+The advisor can be enabled, so KSM can automatically adapt to changes in the
+number of candidate pages to scan. Two advisors are implemented: none and
+scan-time. With none, no advisor is enabled. The default is none.
+
+The scan time advisor changes the ``pages_to_scan`` parameter based on the
+observed scan times. The possible values for the ``pages_to_scan`` parameter is
+limited by the ``advisor_max_cpu`` parameter. In addition there is also the
+``advisor_target_scan_time`` parameter. This parameter sets the target time to
+scan all the KSM candidate pages. The parameter ``advisor_target_scan_time``
+decides how aggressive the scan time advisor scans candidate pages. Lower
+values make the scan time advisor to scan more aggresively. This is the most
+important parameter for the configuration of the scan time advisor.
+
+The initial value and the maximum value can be changed with ``advisor_min_pages``
+and ``advisor_max_pages``. The default values are sufficient for most workloads.
+
+The ``pages_to_scan`` parameter is re-calculated after a scan has been completed.
+
+
 --
 Izik Eidus,
 Hugh Dickins, 17 Nov 2009
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/4] mm/ksm: Add ksm advisor
  2023-12-04 23:49 [PATCH v3 0/4] mm/ksm: Add ksm advisor Stefan Roesch
                   ` (3 preceding siblings ...)
  2023-12-04 23:49 ` [PATCH v3 4/4] mm/ksm: document ksm advisor and its sysfs knobs Stefan Roesch
@ 2023-12-05 20:46 ` Andrew Morton
  2023-12-07 23:18   ` Stefan Roesch
  4 siblings, 1 reply; 16+ messages in thread
From: Andrew Morton @ 2023-12-05 20:46 UTC (permalink / raw)
  To: Stefan Roesch; +Cc: kernel-team, david, hannes, riel, linux-kernel, linux-mm

On Mon,  4 Dec 2023 15:49:02 -0800 Stefan Roesch <shr@devkernel.io> wrote:

> What is the KSM advisor?
> =========================
> The ksm advisor automatically manages the pages_to_scan setting to
> achieve a target scan time. The target scan time defines how many seconds
> it should take to scan all the candidate KSM pages. In other words the
> pages_to_scan rate is changed by the advisor to achieve the target scan
> time.

Dumb question time.  Can this be done in userspace?  Presumably this
will require exposing some additional kernel state to userspace.

> Why do we need a KSM advisor?
> ==============================
> The number of candidate pages for KSM is dynamic. It can often be observed
> that during the startup of an application more candidate pages need to be
> processed. Without an advisor the pages_to_scan parameter needs to be
> sized for the maximum number of candidate pages. With the scan time
> advisor the pages_to_scan parameter based can be changed based on demand.
> 
> Algorithm
> ==========
> The algorithm calculates the change value based on the target scan time
> and the previous scan time. To avoid pertubations an exponentially
> weighted moving average is applied.
> 
> The algorithm has a max and min
> value to:
> - guarantee responsiveness to changes
> - to limit CPU resource consumption
> 
> Parameters to influence the KSM scan advisor
> =============================================
> The respective parameters are:
> - ksm_advisor_mode
>   0: None (default), 1: scan time advisor
> - ksm_advisor_target_scan_time
>   how many seconds a scan should of all candidate pages take
> - ksm_advisor_max_cpu
>   upper limit for the cpu usage in percent of the ksmd background thread
> 
> The initial value and the max value for the pages_to_scan parameter can
> be limited with:
> - ksm_advisor_min_pages
>   minimum value for pages_to_scan per batch
> - ksm_advisor_max_pages
>   maximum value for pages_to_scan per batch

Should these be called ksm_advisor_min_pages_to_scan?

> The default settings for the above two parameters should be suitable for
> most workloads.
> 
> The parameters are exposed as knobs in /sys/kernel/mm/ksm. By default the
> scan time advisor is disabled.

Disabling it will reduce the effectiveness of testing.  What are the
risks of defaulting to "on"?

> Currently there are two advisors:
> - none and
> - scan-time.
> 
> Resource savings
> =================
> Tests with various workloads have shown considerable CPU savings. Most
> of the workloads I have investigated have more candidate pages during
> startup. Once the workload is stable in terms of memory, the number of
> candidate pages is reduced. Without the advisor, the pages_to_scan needs
> to be sized for the maximum number of candidate pages. So having this
> advisor definitely helps in reducing CPU consumption.
> 
> For the instagram workload, the advisor achieves a 25% CPU reduction.

25% of what?  What is the overall affect on machine resource
consumption?

> Once the memory is stable, the pages_to_scan parameter gets reduced to
> about 40% of its max value.
> 
> The new advisor works especially well if the smart scan feature is also
> enabled.
> 
> How is defining a target scan time better?
> ===========================================
> For an administrator it is more logical to set a target scan time.. The
> administrator can determine how many pages are scanned on each scan.
> Therefore setting a target scan time makes more sense.
> 
> In addition the administrator might have a good idea about the memory
> sizing of its respective workloads.
> 
> Setting cpu limits is easier than setting The pages_to_scan parameter. The
> pages_to_scan parameter is per batch. For the administrator it is difficult
> to set the pages_to_scan parameter.
> 
> Tracing
> =======
> A new tracing event has been added for the scan time advisor. The new
> trace event is called ksm_advisor. It reports the scan time, the new
> pages_to_scan setting and the cpu usage of the ksmd background thread.
> 
> Other approaches
> =================
> 
> Approach 1: Adapt pages_to_scan after processing each batch. If KSM
>   merges pages, increase the scan rate, if less KSM pages, reduce the
>   the pages_to_scan rate. This doesn't work too well. While it increases
>   the pages_to_scan for a short period, but generally it ends up with a
>   too low pages_to_scan rate.
> 
> Approach 2: Adapt pages_to_scan after each scan. The problem with that
>   approach is that the calculated scan rate tends to be high. The more
>   aggressive KSM scans, the more pages it can de-duplicate.
> 
> There have been earlier attempts at an advisor:
>   propose auto-run mode of ksm and its tests
>   (https://marc.info/?l=linux-mm&m=166029880214485&w=2)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/4] mm/ksm: add ksm advisor
  2023-12-04 23:49 ` [PATCH v3 1/4] mm/ksm: add " Stefan Roesch
@ 2023-12-06 15:17   ` kernel test robot
  2023-12-12 13:44   ` David Hildenbrand
  1 sibling, 0 replies; 16+ messages in thread
From: kernel test robot @ 2023-12-06 15:17 UTC (permalink / raw)
  To: Stefan Roesch, kernel-team
  Cc: oe-kbuild-all, shr, akpm, david, hannes, riel, linux-kernel, linux-mm

Hi Stefan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 12d04a7bf0da67321229d2bc8b1a7074d65415a9]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Roesch/mm-ksm-add-ksm-advisor/20231205-075118
base:   12d04a7bf0da67321229d2bc8b1a7074d65415a9
patch link:    https://lore.kernel.org/r/20231204234906.1237478-2-shr%40devkernel.io
patch subject: [PATCH v3 1/4] mm/ksm: add ksm advisor
config: i386-randconfig-011-20231206 (https://download.01.org/0day-ci/archive/20231206/202312062353.lBYSA7Eb-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/202312062353.lBYSA7Eb-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312062353.lBYSA7Eb-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: mm/ksm.o: in function `scan_time_advisor':
>> mm/ksm.c:423: undefined reference to `__divdi3'
>> ld: mm/ksm.c:435: undefined reference to `__divdi3'
   ld: mm/ksm.c:428: undefined reference to `__divdi3'


vim +423 mm/ksm.c

   383	
   384	/*
   385	 * The scan time advisor is based on the current scan rate and the target
   386	 * scan rate.
   387	 *
   388	 *      new_pages_to_scan = pages_to_scan * (scan_time / target_scan_time)
   389	 *
   390	 * To avoid pertubations it calculates a change factor of previous changes.
   391	 * A new change factor is calculated for each iteration and it uses an
   392	 * exponentially weighted moving average. The new pages_to_scan value is
   393	 * multiplied with that change factor:
   394	 *
   395	 *      new_pages_to_scan *= change facor
   396	 *
   397	 * In addition the new pages_to_scan value is capped by the max and min
   398	 * limits.
   399	 */
   400	static void scan_time_advisor(void)
   401	{
   402		unsigned int cpu_percent;
   403		unsigned long cpu_time;
   404		unsigned long cpu_time_diff;
   405		unsigned long cpu_time_diff_ms;
   406		unsigned long pages;
   407		unsigned long per_page_cost;
   408		unsigned long factor;
   409		unsigned long change;
   410		unsigned long last_scan_time;
   411		s64 scan_time;
   412	
   413		/* Convert scan time to seconds */
   414		scan_time = advisor_stop_scan();
   415		scan_time = div_s64(scan_time, MSEC_PER_SEC);
   416		scan_time = scan_time ? scan_time : 1;
   417	
   418		/* Calculate CPU consumption of ksmd background thread */
   419		cpu_time = task_sched_runtime(current);
   420		cpu_time_diff = cpu_time - advisor_ctx.cpu_time;
   421		cpu_time_diff_ms = cpu_time_diff / 1000 / 1000;
   422	
 > 423		cpu_percent = (cpu_time_diff_ms * 100) / (scan_time * 1000);
   424		cpu_percent = cpu_percent ? cpu_percent : 1;
   425		last_scan_time = prev_scan_time(&advisor_ctx, scan_time);
   426	
   427		/* Calculate scan time as percentage of target scan time */
   428		factor = ksm_advisor_target_scan_time * 100 / scan_time;
   429		factor = factor ? factor : 1;
   430	
   431		/*
   432		 * Calculate scan time as percentage of last scan time and use
   433		 * exponentially weighted average to smooth it
   434		 */
 > 435		change = scan_time * 100 / last_scan_time;
   436		change = change ? change : 1;
   437		change = ewma(advisor_ctx.change, change);
   438	
   439		/* Calculate new scan rate based on target scan rate. */
   440		pages = ksm_thread_pages_to_scan * 100 / factor;
   441		/* Update pages_to_scan by weighted change percentage. */
   442		pages = pages * change / 100;
   443	
   444		/* Cap new pages_to_scan value */
   445		per_page_cost = ksm_thread_pages_to_scan / cpu_percent;
   446		per_page_cost = per_page_cost ? per_page_cost : 1;
   447	
   448		pages = min(pages, per_page_cost * ksm_advisor_max_cpu);
   449		pages = max(pages, per_page_cost * ksm_advisor_min_cpu);
   450		pages = min(pages, ksm_advisor_max_pages);
   451	
   452		/* Update advisor context */
   453		advisor_ctx.change = change;
   454		advisor_ctx.scan_time = scan_time;
   455		advisor_ctx.cpu_time = cpu_time;
   456	
   457		ksm_thread_pages_to_scan = pages;
   458	}
   459	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/4] mm/ksm: Add ksm advisor
  2023-12-05 20:46 ` [PATCH v3 0/4] mm/ksm: Add ksm advisor Andrew Morton
@ 2023-12-07 23:18   ` Stefan Roesch
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Roesch @ 2023-12-07 23:18 UTC (permalink / raw)
  To: Andrew Morton; +Cc: kernel-team, david, hannes, riel, linux-kernel, linux-mm


Andrew Morton <akpm@linux-foundation.org> writes:

> On Mon,  4 Dec 2023 15:49:02 -0800 Stefan Roesch <shr@devkernel.io> wrote:
>
>> What is the KSM advisor?
>> =========================
>> The ksm advisor automatically manages the pages_to_scan setting to
>> achieve a target scan time. The target scan time defines how many seconds
>> it should take to scan all the candidate KSM pages. In other words the
>> pages_to_scan rate is changed by the advisor to achieve the target scan
>> time.
>
> Dumb question time.  Can this be done in userspace?  Presumably this
> will require exposing some additional kernel state to userspace.
>

Userspace doesn't look like a good fit
- We need access to how much cpu the ksmd kernel thread spent during the
last scan
- We want to recompute this after each scan completes, so the new
parameter is in effect for the next scan and to avoid we have to
partially deal with new values in the middle
- When the advisor is active we want to disable that other users can
manually set the scan rate.

The scan advisor might be augmented with per page cost in the future.

>> Why do we need a KSM advisor?
>> ==============================
>> The number of candidate pages for KSM is dynamic. It can often be observed
>> that during the startup of an application more candidate pages need to be
>> processed. Without an advisor the pages_to_scan parameter needs to be
>> sized for the maximum number of candidate pages. With the scan time
>> advisor the pages_to_scan parameter based can be changed based on demand.
>>
>> Algorithm
>> ==========
>> The algorithm calculates the change value based on the target scan time
>> and the previous scan time. To avoid pertubations an exponentially
>> weighted moving average is applied.
>>
>> The algorithm has a max and min
>> value to:
>> - guarantee responsiveness to changes
>> - to limit CPU resource consumption
>>
>> Parameters to influence the KSM scan advisor
>> =============================================
>> The respective parameters are:
>> - ksm_advisor_mode
>>   0: None (default), 1: scan time advisor
>> - ksm_advisor_target_scan_time
>>   how many seconds a scan should of all candidate pages take
>> - ksm_advisor_max_cpu
>>   upper limit for the cpu usage in percent of the ksmd background thread
>>
>> The initial value and the max value for the pages_to_scan parameter can
>> be limited with:
>> - ksm_advisor_min_pages
>>   minimum value for pages_to_scan per batch
>> - ksm_advisor_max_pages
>>   maximum value for pages_to_scan per batch
>
> Should these be called ksm_advisor_min_pages_to_scan?
>

That's a good recommendation. I'll make that change and the
corresponding change for max.

>> The default settings for the above two parameters should be suitable for
>> most workloads.
>>
>> The parameters are exposed as knobs in /sys/kernel/mm/ksm. By default the
>> scan time advisor is disabled.
>
> Disabling it will reduce the effectiveness of testing.  What are the
> risks of defaulting to "on"?
>

Some users might have already tuned the values for pages_to_scan. So
generally turning it on might not be good. However we could turn it on,
it the default value of 100 for pages_to_scan hasn't been changed.

Any thoughts?

>> Currently there are two advisors:
>> - none and
>> - scan-time.
>>
>> Resource savings
>> =================
>> Tests with various workloads have shown considerable CPU savings. Most
>> of the workloads I have investigated have more candidate pages during
>> startup. Once the workload is stable in terms of memory, the number of
>> candidate pages is reduced. Without the advisor, the pages_to_scan needs
>> to be sized for the maximum number of candidate pages. So having this
>> advisor definitely helps in reducing CPU consumption.
>>
>> For the instagram workload, the advisor achieves a 25% CPU reduction.
>
> 25% of what?  What is the overall affect on machine resource
> consumption?
>

25% of the cpu consumption of the ksmd background thread. On a 32 cpu
machine this translates to 1 to 2% cpu savings alltogether.

However this is highly workload dependent.

>> Once the memory is stable, the pages_to_scan parameter gets reduced to
>> about 40% of its max value.
>>
>> The new advisor works especially well if the smart scan feature is also
>> enabled.
>>
>> How is defining a target scan time better?
>> ===========================================
>> For an administrator it is more logical to set a target scan time.. The
>> administrator can determine how many pages are scanned on each scan.
>> Therefore setting a target scan time makes more sense.
>>
>> In addition the administrator might have a good idea about the memory
>> sizing of its respective workloads.
>>
>> Setting cpu limits is easier than setting The pages_to_scan parameter. The
>> pages_to_scan parameter is per batch. For the administrator it is difficult
>> to set the pages_to_scan parameter.
>>
>> Tracing
>> =======
>> A new tracing event has been added for the scan time advisor. The new
>> trace event is called ksm_advisor. It reports the scan time, the new
>> pages_to_scan setting and the cpu usage of the ksmd background thread.
>>
>> Other approaches
>> =================
>>
>> Approach 1: Adapt pages_to_scan after processing each batch. If KSM
>>   merges pages, increase the scan rate, if less KSM pages, reduce the
>>   the pages_to_scan rate. This doesn't work too well. While it increases
>>   the pages_to_scan for a short period, but generally it ends up with a
>>   too low pages_to_scan rate.
>>
>> Approach 2: Adapt pages_to_scan after each scan. The problem with that
>>   approach is that the calculated scan rate tends to be high. The more
>>   aggressive KSM scans, the more pages it can de-duplicate.
>>
>> There have been earlier attempts at an advisor:
>>   propose auto-run mode of ksm and its tests
>>   (https://marc.info/?l=linux-mm&m=166029880214485&w=2)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/4] mm/ksm: add ksm advisor
  2023-12-04 23:49 ` [PATCH v3 1/4] mm/ksm: add " Stefan Roesch
  2023-12-06 15:17   ` kernel test robot
@ 2023-12-12 13:44   ` David Hildenbrand
  2023-12-12 18:32     ` Stefan Roesch
  1 sibling, 1 reply; 16+ messages in thread
From: David Hildenbrand @ 2023-12-12 13:44 UTC (permalink / raw)
  To: Stefan Roesch, kernel-team; +Cc: akpm, hannes, riel, linux-kernel, linux-mm

[...]

> +
> +/**
> + * struct advisor_ctx - metadata for KSM advisor
> + * @start_scan: start time of the current scan
> + * @scan_time: scan time of previous scan
> + * @change: change in percent to pages_to_scan parameter
> + * @cpu_time: cpu time consumed by the ksmd thread in the previous scan
> + */
> +struct advisor_ctx {
> +	ktime_t start_scan;
> +	unsigned long scan_time;
> +	unsigned long change;
> +	unsigned long long cpu_time;
> +};
> +static struct advisor_ctx advisor_ctx;
> +
> +/* Define different advisor's */
> +enum ksm_advisor_type {
> +	KSM_ADVISOR_NONE,
> +	KSM_ADVISOR_SCAN_TIME,
> +};
> +static enum ksm_advisor_type ksm_advisor;
> +
> +static void init_advisor(void)
> +{
> +	advisor_ctx = (const struct advisor_ctx){ 0 };
> +}

Again, you can drop this completely. The static values are already 
initialized to 0.

Or is there any reason to initialize to 0 explicitly?

> +
> +static void set_advisor_defaults(void)
> +{
> +	if (ksm_advisor == KSM_ADVISOR_NONE)
> +		ksm_thread_pages_to_scan = DEFAULT_PAGES_TO_SCAN;
> +	else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
> +		ksm_thread_pages_to_scan = ksm_advisor_min_pages;
> +}

That function is unused?

> +
> +static inline void advisor_start_scan(void)
> +{
> +	if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
> +		advisor_ctx.start_scan = ktime_get();
> +}
> +
> +static inline s64 advisor_stop_scan(void)
> +{
> +	return ktime_ms_delta(ktime_get(), advisor_ctx.start_scan);
> +}

Just inline that into the caller. Then rename run_advisor() into 
advisor_stop_scan(). So in scan_get_next_rmap_item)( you have paired 
start+stop hooks.

> +
> +/*
> + * Use previous scan time if available, otherwise use current scan time as an
> + * approximation for the previous scan time.
> + */
> +static inline unsigned long prev_scan_time(struct advisor_ctx *ctx,
> +					   unsigned long scan_time)
> +{
> +	return ctx->scan_time ? ctx->scan_time : scan_time;
> +}
> +
> +/* Calculate exponential weighted moving average */
> +static unsigned long ewma(unsigned long prev, unsigned long curr)
> +{
> +	return ((100 - EWMA_WEIGHT) * prev + EWMA_WEIGHT * curr) / 100;
> +}
> +
> +/*
> + * The scan time advisor is based on the current scan rate and the target
> + * scan rate.
> + *
> + *      new_pages_to_scan = pages_to_scan * (scan_time / target_scan_time)
> + *
> + * To avoid pertubations it calculates a change factor of previous changes.

s/pertubations/perturbations/

Do you also want to describe how min/max CPU comes into play?

> + * A new change factor is calculated for each iteration and it uses an
> + * exponentially weighted moving average. The new pages_to_scan value is
> + * multiplied with that change factor:
> + *
> + *      new_pages_to_scan *= change facor
> + *
> + * In addition the new pages_to_scan value is capped by the max and min
> + * limits.
> + */



With that, LGTM

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor
  2023-12-04 23:49 ` [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor Stefan Roesch
@ 2023-12-12 13:45   ` David Hildenbrand
  2023-12-12 18:02     ` Stefan Roesch
  0 siblings, 1 reply; 16+ messages in thread
From: David Hildenbrand @ 2023-12-12 13:45 UTC (permalink / raw)
  To: Stefan Roesch, kernel-team; +Cc: akpm, hannes, riel, linux-kernel, linux-mm

On 05.12.23 00:49, Stefan Roesch wrote:
> This adds four new knobs for the KSM advisor to influence its behaviour.
> 
> The knobs are:
> - advisor_mode:
>      none:      no advisor (default)
>      scan-time: scan time advisor
> - advisor_max_cpu: 70 (default, cpu usage percent)
> - advisor_min_pages: 500 (default)
> - advisor_max_pages: 30000 (default)
> - advisor_target_scan_time: 200 (default in seconds)
> 
> The new values will take effect on the next scan round.
> 
> Signed-off-by: Stefan Roesch <shr@devkernel.io>
> ---
>   mm/ksm.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 127 insertions(+)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index b27010fa2e946..18b7185bbc65b 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -3735,6 +3735,128 @@ static ssize_t smart_scan_store(struct kobject *kobj,
>   }
>   KSM_ATTR(smart_scan);
>   
> +static ssize_t advisor_mode_show(struct kobject *kobj,
> +				 struct kobj_attribute *attr, char *buf)
> +{
> +	const char *output;
> +
> +	if (ksm_advisor == KSM_ADVISOR_NONE)
> +		output = "[none] scan-time";
> +	else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
> +		output = "none [scan-time]";
> +
> +	return sysfs_emit(buf, "%s\n", output);
> +}
> +
> +static ssize_t advisor_mode_store(struct kobject *kobj,
> +				  struct kobj_attribute *attr, const char *buf,
> +				  size_t count)
> +{
> +	if (sysfs_streq("scan-time", buf))
> +		ksm_advisor = KSM_ADVISOR_SCAN_TIME;
> +	else if (sysfs_streq("none", buf))
> +		ksm_advisor = KSM_ADVISOR_NONE;
> +	else
> +		return -EINVAL;
> +
> +	/* Set advisor default values */
> +	init_advisor();

Is the "init_advisor()" really required?

> +	set_advisor_defaults();

That function should go to this patch.



-- 
Cheers,

David / dhildenb


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/4] mm/ksm: add tracepoint for ksm advisor
  2023-12-04 23:49 ` [PATCH v3 3/4] mm/ksm: add tracepoint for ksm advisor Stefan Roesch
@ 2023-12-12 13:45   ` David Hildenbrand
  0 siblings, 0 replies; 16+ messages in thread
From: David Hildenbrand @ 2023-12-12 13:45 UTC (permalink / raw)
  To: Stefan Roesch, kernel-team; +Cc: akpm, hannes, riel, linux-kernel, linux-mm

On 05.12.23 00:49, Stefan Roesch wrote:
> This adds a new tracepoint for the ksm advisor. It reports the last scan
> time, the new setting of the pages_to_scan parameter and the average cpu
> percent usage of the ksmd background thread for the last scan.
> 
> Signed-off-by: Stefan Roesch <shr@devkernel.io>
> ---
>   include/trace/events/ksm.h | 33 +++++++++++++++++++++++++++++++++
>   mm/ksm.c                   |  1 +
>   2 files changed, 34 insertions(+)
> 
> diff --git a/include/trace/events/ksm.h b/include/trace/events/ksm.h
> index b5ac35c1d0e88..e728647b5d268 100644
> --- a/include/trace/events/ksm.h
> +++ b/include/trace/events/ksm.h
> @@ -245,6 +245,39 @@ TRACE_EVENT(ksm_remove_rmap_item,
>   			__entry->pfn, __entry->rmap_item, __entry->mm)
>   );
>   
> +/**
> + * ksm_advisor - called after the advisor has run
> + *
> + * @scan_time:		scan time in seconds
> + * @pages_to_scan:	new pages_to_scan value
> + * @cpu_percent:	cpu usage in percent
> + *
> + * Allows to trace the ksm advisor.
> + */
> +TRACE_EVENT(ksm_advisor,
> +
> +	TP_PROTO(s64 scan_time, unsigned long pages_to_scan,
> +		 unsigned int cpu_percent),
> +
> +	TP_ARGS(scan_time, pages_to_scan, cpu_percent),
> +
> +	TP_STRUCT__entry(
> +		__field(s64,		scan_time)
> +		__field(unsigned long,	pages_to_scan)
> +		__field(unsigned int,	cpu_percent)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->scan_time	= scan_time;
> +		__entry->pages_to_scan	= pages_to_scan;
> +		__entry->cpu_percent	= cpu_percent;
> +	),
> +
> +	TP_printk("ksm scan time %lld pages_to_scan %lu cpu percent %u",
> +			__entry->scan_time, __entry->pages_to_scan,
> +			__entry->cpu_percent)
> +);
> +
>   #endif /* _TRACE_KSM_H */
>   
>   /* This part must be outside protection */
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 18b7185bbc65b..b8065c5d5f833 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -455,6 +455,7 @@ static void scan_time_advisor(void)
>   	advisor_ctx.cpu_time = cpu_time;
>   
>   	ksm_thread_pages_to_scan = pages;
> +	trace_ksm_advisor(scan_time, pages, cpu_percent);
>   }
>   
>   static void run_advisor(void)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 4/4] mm/ksm: document ksm advisor and its sysfs knobs
  2023-12-04 23:49 ` [PATCH v3 4/4] mm/ksm: document ksm advisor and its sysfs knobs Stefan Roesch
@ 2023-12-12 13:46   ` David Hildenbrand
  0 siblings, 0 replies; 16+ messages in thread
From: David Hildenbrand @ 2023-12-12 13:46 UTC (permalink / raw)
  To: Stefan Roesch, kernel-team; +Cc: akpm, hannes, riel, linux-kernel, linux-mm

On 05.12.23 00:49, Stefan Roesch wrote:
> This documents the KSM advisor and its new knobs in /sys/fs/kernel/mm.
> 
> Signed-off-by: Stefan Roesch <shr@devkernel.io>
> ---
>   Documentation/admin-guide/mm/ksm.rst | 54 ++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
> 
> diff --git a/Documentation/admin-guide/mm/ksm.rst b/Documentation/admin-guide/mm/ksm.rst
> index e59231ac6bb71..297c4a0d04dee 100644
> --- a/Documentation/admin-guide/mm/ksm.rst
> +++ b/Documentation/admin-guide/mm/ksm.rst
> @@ -80,6 +80,9 @@ pages_to_scan
>           how many pages to scan before ksmd goes to sleep
>           e.g. ``echo 100 > /sys/kernel/mm/ksm/pages_to_scan``.
>   
> +        The pages_to_scan value cannot be changed if ``advisor_mode`` has
> +        been set to scan-time.
> +
>           Default: 100 (chosen for demonstration purposes)
>   
>   sleep_millisecs
> @@ -164,6 +167,29 @@ smart_scan
>           optimization is enabled.  The ``pages_skipped`` metric shows how
>           effective the setting is.
>   
> +advisor_mode
> +        The ``advisor_mode`` selects the current advisor. Two modes are
> +        supported: none and scan-time. The default is none. By setting
> +        ``advisor_mode`` to scan-time, the scan time advisor is enabled.
> +        The section about ``advisor`` explains in detail how the scan time
> +        advisor works.
> +
> +adivsor_max_cpu
> +        specifies the upper limit of the cpu percent usage of the ksmd
> +        background thread. The default is 70.
> +
> +advisor_target_scan_time
> +        specifies the target scan time in seconds to scan all the candidate
> +        pages. The default value is 200 seconds.
> +
> +advisor_min_pages
> +        specifies the lower limit of the ``pages_to_scan`` parameter of the
> +        scan time advisor. The default is 500.
> +
> +adivsor_max_pages
> +        specifies the upper limit of the ``pages_to_scan`` parameter of the
> +        scan time advisor. The default is 30000.
> +
>   The effectiveness of KSM and MADV_MERGEABLE is shown in ``/sys/kernel/mm/ksm/``:
>   
>   general_profit
> @@ -263,6 +289,34 @@ ksm_swpin_copy
>   	note that KSM page might be copied when swapping in because do_swap_page()
>   	cannot do all the locking needed to reconstitute a cross-anon_vma KSM page.
>   
> +Advisor
> +=======
> +
> +The number of candidate pages for KSM is dynamic. It can be often observed
> +that during the startup of an application more candidate pages need to be
> +processed. Without an advisor the ``pages_to_scan`` parameter needs to be
> +sized for the maximum number of candidate pages. The scan time advisor can
> +changes the ``pages_to_scan`` parameter based on demand.
> +
> +The advisor can be enabled, so KSM can automatically adapt to changes in the
> +number of candidate pages to scan. Two advisors are implemented: none and
> +scan-time. With none, no advisor is enabled. The default is none.
> +
> +The scan time advisor changes the ``pages_to_scan`` parameter based on the
> +observed scan times. The possible values for the ``pages_to_scan`` parameter is
> +limited by the ``advisor_max_cpu`` parameter. In addition there is also the
> +``advisor_target_scan_time`` parameter. This parameter sets the target time to
> +scan all the KSM candidate pages. The parameter ``advisor_target_scan_time``
> +decides how aggressive the scan time advisor scans candidate pages. Lower
> +values make the scan time advisor to scan more aggresively. This is the most
> +important parameter for the configuration of the scan time advisor.
> +
> +The initial value and the maximum value can be changed with ``advisor_min_pages``
> +and ``advisor_max_pages``. The default values are sufficient for most workloads.
> +
> +The ``pages_to_scan`` parameter is re-calculated after a scan has been completed.


Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor
  2023-12-12 13:45   ` David Hildenbrand
@ 2023-12-12 18:02     ` Stefan Roesch
  2023-12-12 18:07       ` David Hildenbrand
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Roesch @ 2023-12-12 18:02 UTC (permalink / raw)
  To: David Hildenbrand, kernel-team
  Cc: Andrew Morton, hannes, riel, linux-kernel, linux-mm



On Tue, Dec 12, 2023, at 5:45 AM, David Hildenbrand wrote:
> On 05.12.23 00:49, Stefan Roesch wrote:
>> This adds four new knobs for the KSM advisor to influence its behaviour.
>> 
>> The knobs are:
>> - advisor_mode:
>>      none:      no advisor (default)
>>      scan-time: scan time advisor
>> - advisor_max_cpu: 70 (default, cpu usage percent)
>> - advisor_min_pages: 500 (default)
>> - advisor_max_pages: 30000 (default)
>> - advisor_target_scan_time: 200 (default in seconds)
>> 
>> The new values will take effect on the next scan round.
>> 
>> Signed-off-by: Stefan Roesch <shr@devkernel.io>
>> ---
>>   mm/ksm.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 127 insertions(+)
>> 
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index b27010fa2e946..18b7185bbc65b 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -3735,6 +3735,128 @@ static ssize_t smart_scan_store(struct kobject *kobj,
>>   }
>>   KSM_ATTR(smart_scan);
>>   
>> +static ssize_t advisor_mode_show(struct kobject *kobj,
>> +				 struct kobj_attribute *attr, char *buf)
>> +{
>> +	const char *output;
>> +
>> +	if (ksm_advisor == KSM_ADVISOR_NONE)
>> +		output = "[none] scan-time";
>> +	else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
>> +		output = "none [scan-time]";
>> +
>> +	return sysfs_emit(buf, "%s\n", output);
>> +}
>> +
>> +static ssize_t advisor_mode_store(struct kobject *kobj,
>> +				  struct kobj_attribute *attr, const char *buf,
>> +				  size_t count)
>> +{
>> +	if (sysfs_streq("scan-time", buf))
>> +		ksm_advisor = KSM_ADVISOR_SCAN_TIME;
>> +	else if (sysfs_streq("none", buf))
>> +		ksm_advisor = KSM_ADVISOR_NONE;
>> +	else
>> +		return -EINVAL;
>> +
>> +	/* Set advisor default values */
>> +	init_advisor();
>
> Is the "init_advisor()" really required?
>

The init_advisor is required for the following scenario:
- advisor is used
- advisor is turned off
- pages_to_scan is used and run for some time
- advisor is turned on again
   ==> Advisor would start with two high values for the first calculation

>> +	set_advisor_defaults();
>
> That function should go to this patch.
>
>
>

I'll move it to this patch. I'll also look if I can should move init_advisor() to this
patch.

> -- 
> Cheers,
>
> David / dhildenb

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor
  2023-12-12 18:02     ` Stefan Roesch
@ 2023-12-12 18:07       ` David Hildenbrand
  2023-12-12 18:27         ` Stefan Roesch
  0 siblings, 1 reply; 16+ messages in thread
From: David Hildenbrand @ 2023-12-12 18:07 UTC (permalink / raw)
  To: Stefan Roesch, kernel-team
  Cc: Andrew Morton, hannes, riel, linux-kernel, linux-mm

On 12.12.23 19:02, Stefan Roesch wrote:
> 
> 
> On Tue, Dec 12, 2023, at 5:45 AM, David Hildenbrand wrote:
>> On 05.12.23 00:49, Stefan Roesch wrote:
>>> This adds four new knobs for the KSM advisor to influence its behaviour.
>>>
>>> The knobs are:
>>> - advisor_mode:
>>>       none:      no advisor (default)
>>>       scan-time: scan time advisor
>>> - advisor_max_cpu: 70 (default, cpu usage percent)
>>> - advisor_min_pages: 500 (default)
>>> - advisor_max_pages: 30000 (default)
>>> - advisor_target_scan_time: 200 (default in seconds)
>>>
>>> The new values will take effect on the next scan round.
>>>
>>> Signed-off-by: Stefan Roesch <shr@devkernel.io>
>>> ---
>>>    mm/ksm.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 127 insertions(+)
>>>
>>> diff --git a/mm/ksm.c b/mm/ksm.c
>>> index b27010fa2e946..18b7185bbc65b 100644
>>> --- a/mm/ksm.c
>>> +++ b/mm/ksm.c
>>> @@ -3735,6 +3735,128 @@ static ssize_t smart_scan_store(struct kobject *kobj,
>>>    }
>>>    KSM_ATTR(smart_scan);
>>>    
>>> +static ssize_t advisor_mode_show(struct kobject *kobj,
>>> +				 struct kobj_attribute *attr, char *buf)
>>> +{
>>> +	const char *output;
>>> +
>>> +	if (ksm_advisor == KSM_ADVISOR_NONE)
>>> +		output = "[none] scan-time";
>>> +	else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
>>> +		output = "none [scan-time]";
>>> +
>>> +	return sysfs_emit(buf, "%s\n", output);
>>> +}
>>> +
>>> +static ssize_t advisor_mode_store(struct kobject *kobj,
>>> +				  struct kobj_attribute *attr, const char *buf,
>>> +				  size_t count)
>>> +{
>>> +	if (sysfs_streq("scan-time", buf))
>>> +		ksm_advisor = KSM_ADVISOR_SCAN_TIME;
>>> +	else if (sysfs_streq("none", buf))
>>> +		ksm_advisor = KSM_ADVISOR_NONE;
>>> +	else
>>> +		return -EINVAL;
>>> +
>>> +	/* Set advisor default values */
>>> +	init_advisor();
>>
>> Is the "init_advisor()" really required?
>>
> 
> The init_advisor is required for the following scenario:
> - advisor is used
> - advisor is turned off
> - pages_to_scan is used and run for some time
> - advisor is turned on again
>     ==> Advisor would start with two high values for the first calculation

Can't set_advisor_defaults() handle that?

-- 
Cheers,

David / dhildenb


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor
  2023-12-12 18:07       ` David Hildenbrand
@ 2023-12-12 18:27         ` Stefan Roesch
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Roesch @ 2023-12-12 18:27 UTC (permalink / raw)
  To: David Hildenbrand, kernel-team
  Cc: Andrew Morton, hannes, riel, linux-kernel, linux-mm



On Tue, Dec 12, 2023, at 10:07 AM, David Hildenbrand wrote:
> On 12.12.23 19:02, Stefan Roesch wrote:
>> 
>> 
>> On Tue, Dec 12, 2023, at 5:45 AM, David Hildenbrand wrote:
>>> On 05.12.23 00:49, Stefan Roesch wrote:
>>>> This adds four new knobs for the KSM advisor to influence its behaviour.
>>>>
>>>> The knobs are:
>>>> - advisor_mode:
>>>>       none:      no advisor (default)
>>>>       scan-time: scan time advisor
>>>> - advisor_max_cpu: 70 (default, cpu usage percent)
>>>> - advisor_min_pages: 500 (default)
>>>> - advisor_max_pages: 30000 (default)
>>>> - advisor_target_scan_time: 200 (default in seconds)
>>>>
>>>> The new values will take effect on the next scan round.
>>>>
>>>> Signed-off-by: Stefan Roesch <shr@devkernel.io>
>>>> ---
>>>>    mm/ksm.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 127 insertions(+)
>>>>
>>>> diff --git a/mm/ksm.c b/mm/ksm.c
>>>> index b27010fa2e946..18b7185bbc65b 100644
>>>> --- a/mm/ksm.c
>>>> +++ b/mm/ksm.c
>>>> @@ -3735,6 +3735,128 @@ static ssize_t smart_scan_store(struct kobject *kobj,
>>>>    }
>>>>    KSM_ATTR(smart_scan);
>>>>    
>>>> +static ssize_t advisor_mode_show(struct kobject *kobj,
>>>> +				 struct kobj_attribute *attr, char *buf)
>>>> +{
>>>> +	const char *output;
>>>> +
>>>> +	if (ksm_advisor == KSM_ADVISOR_NONE)
>>>> +		output = "[none] scan-time";
>>>> +	else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
>>>> +		output = "none [scan-time]";
>>>> +
>>>> +	return sysfs_emit(buf, "%s\n", output);
>>>> +}
>>>> +
>>>> +static ssize_t advisor_mode_store(struct kobject *kobj,
>>>> +				  struct kobj_attribute *attr, const char *buf,
>>>> +				  size_t count)
>>>> +{
>>>> +	if (sysfs_streq("scan-time", buf))
>>>> +		ksm_advisor = KSM_ADVISOR_SCAN_TIME;
>>>> +	else if (sysfs_streq("none", buf))
>>>> +		ksm_advisor = KSM_ADVISOR_NONE;
>>>> +	else
>>>> +		return -EINVAL;
>>>> +
>>>> +	/* Set advisor default values */
>>>> +	init_advisor();
>>>
>>> Is the "init_advisor()" really required?
>>>
>> 
>> The init_advisor is required for the following scenario:
>> - advisor is used
>> - advisor is turned off
>> - pages_to_scan is used and run for some time
>> - advisor is turned on again
>>     ==> Advisor would start with two high values for the first calculation
>
> Can't set_advisor_defaults() handle that?
>

Yes, I can move the

advisor_ctx = (const struct advisor_ctx){ 0 };

into set_advisor_defaults().

> -- 
> Cheers,
>
> David / dhildenb

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/4] mm/ksm: add ksm advisor
  2023-12-12 13:44   ` David Hildenbrand
@ 2023-12-12 18:32     ` Stefan Roesch
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Roesch @ 2023-12-12 18:32 UTC (permalink / raw)
  To: David Hildenbrand, kernel-team
  Cc: Andrew Morton, hannes, riel, linux-kernel, linux-mm



On Tue, Dec 12, 2023, at 5:44 AM, David Hildenbrand wrote:
> [...]
>
>> +
>> +/**
>> + * struct advisor_ctx - metadata for KSM advisor
>> + * @start_scan: start time of the current scan
>> + * @scan_time: scan time of previous scan
>> + * @change: change in percent to pages_to_scan parameter
>> + * @cpu_time: cpu time consumed by the ksmd thread in the previous scan
>> + */
>> +struct advisor_ctx {
>> +	ktime_t start_scan;
>> +	unsigned long scan_time;
>> +	unsigned long change;
>> +	unsigned long long cpu_time;
>> +};
>> +static struct advisor_ctx advisor_ctx;
>> +
>> +/* Define different advisor's */
>> +enum ksm_advisor_type {
>> +	KSM_ADVISOR_NONE,
>> +	KSM_ADVISOR_SCAN_TIME,
>> +};
>> +static enum ksm_advisor_type ksm_advisor;
>> +
>> +static void init_advisor(void)
>> +{
>> +	advisor_ctx = (const struct advisor_ctx){ 0 };
>> +}
>
> Again, you can drop this completely. The static values are already 
> initialized to 0.
>

It is needed for patch 2, I folded it into set_advisor_defaults

> Or is there any reason to initialize to 0 explicitly?
>
>> +
>> +static void set_advisor_defaults(void)
>> +{
>> +	if (ksm_advisor == KSM_ADVISOR_NONE)
>> +		ksm_thread_pages_to_scan = DEFAULT_PAGES_TO_SCAN;
>> +	else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
>> +		ksm_thread_pages_to_scan = ksm_advisor_min_pages;
>> +}
>
> That function is unused?
>

I think you already saw it, it is used in patch 2, moving the function to patch 2.

>> +
>> +static inline void advisor_start_scan(void)
>> +{
>> +	if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
>> +		advisor_ctx.start_scan = ktime_get();
>> +}
>> +
>> +static inline s64 advisor_stop_scan(void)
>> +{
>> +	return ktime_ms_delta(ktime_get(), advisor_ctx.start_scan);
>> +}
>
> Just inline that into the caller. Then rename run_advisor() into 
> advisor_stop_scan(). So in scan_get_next_rmap_item)( you have paired 
> start+stop hooks.
>

The next version has this change.

>> +
>> +/*
>> + * Use previous scan time if available, otherwise use current scan time as an
>> + * approximation for the previous scan time.
>> + */
>> +static inline unsigned long prev_scan_time(struct advisor_ctx *ctx,
>> +					   unsigned long scan_time)
>> +{
>> +	return ctx->scan_time ? ctx->scan_time : scan_time;
>> +}
>> +
>> +/* Calculate exponential weighted moving average */
>> +static unsigned long ewma(unsigned long prev, unsigned long curr)
>> +{
>> +	return ((100 - EWMA_WEIGHT) * prev + EWMA_WEIGHT * curr) / 100;
>> +}
>> +
>> +/*
>> + * The scan time advisor is based on the current scan rate and the target
>> + * scan rate.
>> + *
>> + *      new_pages_to_scan = pages_to_scan * (scan_time / target_scan_time)
>> + *
>> + * To avoid pertubations it calculates a change factor of previous changes.
>
> s/pertubations/perturbations/

Fixed.

>
> Do you also want to describe how min/max CPU comes into play?
>

I added additional documentation for it in the next version of the patch.

>> + * A new change factor is calculated for each iteration and it uses an
>> + * exponentially weighted moving average. The new pages_to_scan value is
>> + * multiplied with that change factor:
>> + *
>> + *      new_pages_to_scan *= change facor
>> + *
>> + * In addition the new pages_to_scan value is capped by the max and min
>> + * limits.
>> + */
>
>
>
> With that, LGTM
>
> Acked-by: David Hildenbrand <david@redhat.com>
>
> -- 
> Cheers,
>
> David / dhildenb

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2023-12-12 18:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-04 23:49 [PATCH v3 0/4] mm/ksm: Add ksm advisor Stefan Roesch
2023-12-04 23:49 ` [PATCH v3 1/4] mm/ksm: add " Stefan Roesch
2023-12-06 15:17   ` kernel test robot
2023-12-12 13:44   ` David Hildenbrand
2023-12-12 18:32     ` Stefan Roesch
2023-12-04 23:49 ` [PATCH v3 2/4] mm/ksm: add sysfs knobs for advisor Stefan Roesch
2023-12-12 13:45   ` David Hildenbrand
2023-12-12 18:02     ` Stefan Roesch
2023-12-12 18:07       ` David Hildenbrand
2023-12-12 18:27         ` Stefan Roesch
2023-12-04 23:49 ` [PATCH v3 3/4] mm/ksm: add tracepoint for ksm advisor Stefan Roesch
2023-12-12 13:45   ` David Hildenbrand
2023-12-04 23:49 ` [PATCH v3 4/4] mm/ksm: document ksm advisor and its sysfs knobs Stefan Roesch
2023-12-12 13:46   ` David Hildenbrand
2023-12-05 20:46 ` [PATCH v3 0/4] mm/ksm: Add ksm advisor Andrew Morton
2023-12-07 23:18   ` Stefan Roesch

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.