mm/compaction: Disable compact_unevictable_allowed on RT

Since commit
5bbe3547aa3ba ("mm: allow compaction of unevictable pages")

it is allowed to examine mlocked pages and compact them by default. On -RT even minor pagefaults are problematic because it may take a few 100us to resolve them and until then the task is blocked.

Make compact_unevictable_allowed = 0 default and remove it from /proc on RT.

Link https://lore.kernel.org/linux-mm/20190710144138.qyn4tuttdq6h7kqx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org/ open here

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 kernel/sysctl.c | 3 ++-
 mm/compaction.c | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

kernel/sysctl.c +2 -1

index 70665934d53e2..d08bd51a0fbc3 100644
@@ -1488,6 +1488,7 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &min_extfrag_threshold,
 		.extra2		= &max_extfrag_threshold,
 	},
+#ifndef CONFIG_PREEMPT_RT
 	{
 		.procname	= "compact_unevictable_allowed",
 		.data		= &sysctl_compact_unevictable_allowed,

@@ -1497,7 +1498,7 @@ static struct ctl_table vm_table[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE,
 	},
-
+#endif
 #endif /* CONFIG_COMPACTION */
 	{
 		.procname	= "min_free_kbytes",

mm/compaction.c +4 -0

index 672d3c78c6abf..b2c804c35ae56 100644
@@ -1590,7 +1590,11 @@ typedef enum {
  * Allow userspace to control policy on scanning the unevictable LRU for
  * compactable pages.
  */
+#ifdef CONFIG_PREEMPT_RT
+#define sysctl_compact_unevictable_allowed 0
+#else
 int sysctl_compact_unevictable_allowed __read_mostly = 1;
+#endif
 
 static inline void
 update_fast_start_pfn(struct compact_control *cc, unsigned long pfn)

Quoted 6 lines · Sebastian Andrzej Siewior

Since commit
5bbe3547aa3ba ("mm: allow compaction of unevictable pages")

it is allowed to examine mlocked pages and compact them by default. On -RT even minor pagefaults are problematic because it may take a few 100us to resolve them and until then the task is blocked.

Fine, this makes sense on RT I guess. There might be some trade-off for high-order allocation latencies though. We could perhaps migrate such mlocked pages to pages allocated without __GFP_MOVABLE during the mlock() to at least somewhat prevent them being scattered all over the zones. For MCL_FUTURE, allocate them as unmovable from the beginning. But that can wait until issues are reported.
I assume you have similar solution for NUMA balancing and whatever else can cause minor faults?

Quoted 2 lines

Make compact_unevictable_allowed = 0 default and remove it from /proc on RT.

Removing it is maybe going too far in terms of RT kernel differences confusing users? Change the default sure, perhaps making it read-only, but removing?

Quoted 1 line

Link https://lore.kernel.org/linux-mm/[email protected]/ open here

In any case the sysctl Documentation/ should be updated? And perhaps also the mlock manpage as you noted in the older thread above?

Thanks,
Vlastimil

Since commit
5bbe3547aa3ba ("mm: allow compaction of unevictable pages")

it is allowed to examine mlocked pages and compact them by default. On -RT even minor pagefaults are problematic because it may take a few 100us to resolve them and until then the task is blocked.

Make compact_unevictable_allowed = 0 default and RO on RT.

Documentation/admin-guide/sysctl/vm.rst allowed to examine the unevictable lru (mlocked pages) for pages to compact. · line 131

Quoted 8 lines

index 64aeee1009cab..bbfa59d25eec3 100644

Documentation/admin-guide/sysctl/vm.rst +1 -0

@@ -128,6 +128,7 @@ allowed to examine the unevictable lru (mlocked pages) for pages to compact.
 This should be used on systems where stalls for minor page faults are an
 acceptable trade for large contiguous free memory.  Set to 0 to prevent
 compaction from moving pages that are unevictable.  Default value is 1.
+On CONFIG_PREEMPT_RT the default value is 0.

This doesn't mention that the file is unwritable on -rt, and it doesn't explain *why* -rt has different behaviour.

Quoted 8 lines

kernel/sysctl.c +2 -0

@@ -1483,7 +1483,11 @@ static struct ctl_table vm_table[] = {
 		.procname	= "compact_unevictable_allowed",
 		.data		= &sysctl_compact_unevictable_allowed,
 		.maxlen		= sizeof(int),
+#ifdef CONFIG_PREEMPT_RT
+		.mode		= 0444,

kernel/sysctl.c static struct ctl_table vm_table[] = { · line 1490

Quoted 3 lines

kernel/sysctl.c +2 -0

+#else
 		.mode		= 0644,
+#endif

This is non-backward-compatible and introduces a possibility that tested-on-non-rt userspace will fail on -rt kernels. It might be better to accept the writes, but to ignore them. Probably with a pr_warn_once() to let people know what we did.

But do we really need to take the option away from -rt users? Perhaps someone wants this feature and can accept the latency hit. How about switching the default and otherwise leaving the kernel behaviour as-is and simply emitting a warning letting -rt users know that they might not want to enable this?

Since commit
5bbe3547aa3ba ("mm: allow compaction of unevictable pages")

it is allowed to examine mlocked pages and compact them by default. On -RT even minor pagefaults are problematic because it may take a few 100us to resolve them and until then the task is blocked.

Make compact_unevictable_allowed = 0 default and issue a warning on RT if it is changed.

kernel/sysctl.c +6 -0

@@ -2572,6 +2577,26 @@ int proc_dointvec(struct ctl_table *table, int write,
 	return do_proc_dointvec(table, write, buffer, lenp, ppos, NULL, NULL);
 }
 
+#ifdef CONFIG_COMPACTION
+static int proc_dointvec_warn_RT_change(struct ctl_table *table, int write,
+					void __user *buffer, size_t *lenp,
+					loff_t *ppos)
+{
+	int ret, old;

kernel/sysctl.c +3 -0

+	return ret;
+}
+#endif

Quoted 15 lines

kernel/sysctl.c +11 -0

@@ -2572,6 +2577,26 @@ int proc_dointvec(struct ctl_table *table, int write,
 	return do_proc_dointvec(table, write, buffer, lenp, ppos, NULL, NULL);
 }
 
+#ifdef CONFIG_COMPACTION
+static int proc_dointvec_warn_RT_change(struct ctl_table *table, int write,
+					void __user *buffer, size_t *lenp,
+					loff_t *ppos)
+{
+	int ret, old;
+
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT) || !write)
+		return proc_dointvec(table, write, buffer, lenp, ppos);
+
+	old = *(int *)table->data;

kernel/sysctl.c int proc_dointvec(struct ctl_table *table, int write, · line 2595

Quoted 5 lines

kernel/sysctl.c +5 -0

+	ret = proc_dointvec(table, write, buffer, lenp, ppos);
+	if (ret)
+		return ret;
+	WARN_ONCE(old != *(int *)table->data, "sysctl attribute %s changed.",
+		  table->procname);

The WARN will include a stack trace which just isn't interesting. A pr_warn() would be better?

Quoted 3 lines

kernel/sysctl.c +3 -0

+	return ret;
+}
+#endif

Quoted 5 lines

kernel/sysctl.c int proc_dointvec(struct ctl_table *table, int write, · line 2595

Quoted 2 lines

kernel/sysctl.c +2 -0

+	WARN_ONCE(old != *(int *)table->data, "sysctl attribute %s changed.",
+		  table->procname);

The WARN will include a stack trace which just isn't interesting. A pr_warn() would be better?

Yeah, the only interesting part of full WARN would possibly be, which process changed it. That might be useful to print.

Since commit
5bbe3547aa3ba ("mm: allow compaction of unevictable pages")

it is allowed to examine mlocked pages and compact them by default. On -RT even minor pagefaults are problematic because it may take a few 100us to resolve them and until then the task is blocked.

Make compact_unevictable_allowed = 0 default and issue a warning on RT if it is changed.

Link https://lore.kernel.org/linux-mm/[email protected]/ open here

Acked-by Mel Gorman <[email protected]>

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

kernel/sysctl.c +6 -0

@@ -2572,6 +2577,28 @@ int proc_dointvec(struct ctl_table *table, int write,
 	return do_proc_dointvec(table, write, buffer, lenp, ppos, NULL, NULL);
 }
 
+#ifdef CONFIG_COMPACTION
+static int proc_dointvec_minmax_warn_RT_change(struct ctl_table *table,
+					       int write, void __user *buffer,
+					       size_t *lenp, loff_t *ppos)
+{
+	int ret, old;

kernel/sysctl.c +6 -0

+			     table->procname, current->comm,
+			     task_pid_nr(current));
+	return ret;
+}
+#endif
+
 /**
  * proc_douintvec - read a vector of unsigned integers
  * @table: the sysctl table

Since commit
5bbe3547aa3ba ("mm: allow compaction of unevictable pages")

it is allowed to examine mlocked pages and compact them by default. On -RT even minor pagefaults are problematic because it may take a few 100us to resolve them and until then the task is blocked.

Make compact_unevictable_allowed = 0 default and issue a warning on RT if it is changed.

Link https://lore.kernel.org/linux-mm/[email protected]/ open here

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
v2…v3: - Allow to modify the value but issue a warning if it is changed.

v1…v2: - Make the proc file RO instead removing it.
- Mention this change in Documentation/…/vm.rst.

 Documentation/admin-guide/sysctl/vm.rst |  3 +++
 kernel/sysctl.c                         | 27 ++++++++++++++++++++++++-
 mm/compaction.c                         |  4 ++++
 3 files changed, 33 insertions(+), 1 deletion(-)

Documentation/admin-guide/sysctl/vm.rst +3 -0

index 64aeee1009cab..0329a4d3fa9ec 100644
@@ -128,6 +128,9 @@ allowed to examine the unevictable lru (mlocked pages) for pages to compact.
 This should be used on systems where stalls for minor page faults are an
 acceptable trade for large contiguous free memory.  Set to 0 to prevent
 compaction from moving pages that are unevictable.  Default value is 1.
+On CONFIG_PREEMPT_RT the default value is 0 in order to avoid a page fault, due
+to compaction, which would block the task from becomming active until the fault
+is resolved.
 
 
 dirty_background_bytes

kernel/sysctl.c +6 -1

index 982203101f961..3ace90b6ac57f 100644
@@ -212,6 +212,11 @@ static int proc_do_cad_pid(struct ctl_table *table, int write,
 		  void __user *buffer, size_t *lenp, loff_t *ppos);
 static int proc_taint(struct ctl_table *table, int write,
 			       void __user *buffer, size_t *lenp, loff_t *ppos);
+#ifdef CONFIG_COMPACTION
+static int proc_dointvec_warn_RT_change(struct ctl_table *table, int write,
+					void __user *buffer, size_t *lenp,
+					loff_t *ppos);
+#endif
 #endif
 
 #ifdef CONFIG_PRINTK

@@ -1484,7 +1489,7 @@ static struct ctl_table vm_table[] = {
 		.data		= &sysctl_compact_unevictable_allowed,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
+		.proc_handler	= proc_dointvec_warn_RT_change,
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE,
 	},

@@ -2572,6 +2577,26 @@ int proc_dointvec(struct ctl_table *table, int write,
 	return do_proc_dointvec(table, write, buffer, lenp, ppos, NULL, NULL);
 }

kernel/sysctl.c +8 -0

+	if (ret)
+		return ret;
+	WARN_ONCE(old != *(int *)table->data, "sysctl attribute %s changed.",
+		  table->procname);
+	return ret;
+}
+#endif
+
 /**
  * proc_douintvec - read a vector of unsigned integers
  * @table: the sysctl table

mm/compaction.c +4 -0

index 672d3c78c6abf..ba77809a1666e 100644
@@ -1590,7 +1590,11 @@ typedef enum {
  * Allow userspace to control policy on scanning the unevictable LRU for
  * compactable pages.
  */
+#ifdef CONFIG_PREEMPT_RT
+int sysctl_compact_unevictable_allowed __read_mostly = 0;
+#else
 int sysctl_compact_unevictable_allowed __read_mostly = 1;
+#endif
 
 static inline void
 update_fast_start_pfn(struct compact_control *cc, unsigned long pfn)

kernel/sysctl.c int proc_dointvec(struct ctl_table *table, int write, · line 2588

Quoted 13 lines

kernel/sysctl.c +9 -0

@@ -2572,6 +2577,26 @@ int proc_dointvec(struct ctl_table *table, int write,
 	return do_proc_dointvec(table, write, buffer, lenp, ppos, NULL, NULL);
 }
 
+#ifdef CONFIG_COMPACTION
+static int proc_dointvec_warn_RT_change(struct ctl_table *table, int write,
+					void __user *buffer, size_t *lenp,
+					loff_t *ppos)
+{
+	int ret, old;
+
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT) || !write)
+		return proc_dointvec(table, write, buffer, lenp, ppos);

Shouldn't you use her proc_dointvec_minmax() per Patch 1/2 ?

kernel/sysctl.c int proc_dointvec(struct ctl_table *table, int write, · line 2591

Quoted 3 lines

kernel/sysctl.c +3 -0

+
+	old = *(int *)table->data;
+	ret = proc_dointvec(table, write, buffer, lenp, ppos);

And here.

Since commit
5bbe3547aa3ba ("mm: allow compaction of unevictable pages")

it is allowed to examine mlocked pages and compact them by default. On -RT even minor pagefaults are problematic because it may take a few 100us to resolve them and until then the task is blocked.

Make compact_unevictable_allowed = 0 default and issue a warning on RT if it is changed.

Link https://lore.kernel.org/linux-mm/[email protected]/ open here

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

The proc file compact_unevictable_allowed' should allow 0 and 1 only, the extra*' attribues have been set properly but without proc_dointvec_minmax() as the `proc_handler' the limit will not be enforced.

Use proc_dointvec_minmax() as the `proc_handler' to enfoce the valid specified range.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 kernel/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/sysctl.c +1 -1

index ad5b88a53c5a8..982203101f961 100644
@@ -1484,7 +1484,7 @@ static struct ctl_table vm_table[] = {
 		.data		= &sysctl_compact_unevictable_allowed,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE,
 	},