[PATCH] mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

[PATCH] mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

[John Hubbard]

mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

Some device drivers add memory to the system via memory hotplug. When
the driver is unloaded, that memory is hot-unplugged.

However, memory hot unplug can fail. And these days, it fails a little
too easily, with respect to the above case. Specifically, if a signal is
pending on the process, hot unplug fails. This leads directly to: the
user must reboot the machine in order to unload the driver, and
therefore the device is unusable until the machine is rebooted.

During teardown paths in the kernel, a higher tolerance for failures or
imperfections is often best. That is, it is often better to continue
with the teardown, than to error out too early.

So in this case, other things (unmovable pages, un-splittable huge
pages) can also cause the above problem. However, those are demonstrably
less common than simply having a pending signal. I've got bug reports
from users who can trivially reproduce this by killing their process
with a "kill -9", for example.

Fix this by soldering on with memory hot plug, even in the presence of
pending signals.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/memory_hotplug.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 8e0fa209d533..57a46620a667 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1879,12 +1879,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
 	do {
 		pfn = start_pfn;
 		do {
-			if (signal_pending(current)) {
-				ret = -EINTR;
-				reason = "signal backoff";
-				goto failed_removal_isolated;
-			}
-
 			cond_resched();
 
 			ret = scan_movable_pages(pfn, end_pfn, &pfn);
-- 
2.41.0

[PATCH v2 00/11] A minor flurry of selftest/mm fixes

[John Hubbard]

Hi,

This is v2 of a series that fixes up build errors and warnings for at
least the 64-bit builds on x86 with clang.

There are lots of changes since v1 [1], thanks to reviews from Peter Xu, David
Hildenbrand, and Muhammad Usama Anjum. These include:

* Using "make headers", and documenting that prerequisite as well.
* Better ways to avoid clang's Wformat-security warnings
* Added Cc's, ack-by's, reviewed-by's.
* Updated commit log messages.

The series also includes an optional "improvement" of moving some uffd
code into uffd-common.[ch], which is proving to be somewhat
controversial, and so if that doesn't get resolved, then patches 9 and
10 may just get dropped. They are not required in order to get a clean
build, now that "make headers" is happening.

[1]: https://lore.kernel.org/all/20230602013358.900637-1-jhubbard@nvidia.com/

thanks,

John Hubbard
NVIDIA

John Hubbard (11):
  selftests/mm: fix uffd-stress unused function warning
  selftests/mm: fix unused variable warnings in hugetlb-madvise.c,
    migration.c
  selftests/mm: fix "warning: expression which evaluates to zero..." in
    mlock2-tests.c
  selftests/mm: fix invocation of tests that are run via shell scripts
  selftests/mm: .gitignore: add mkdirty, va_high_addr_switch
  selftests/mm: fix two -Wformat-security warnings in uffd builds
  selftests/mm: fix a "possibly uninitialized" warning in pkey-x86.h
  selftests/mm: fix uffd-unit-tests.c build failure due to missing
    MADV_COLLAPSE
  selftests/mm: move psize(), pshift() into vm_utils.c
  selftests/mm: move uffd* routines from vm_util.c to uffd-common.c
  Documentation: kselftest: "make headers" is a prerequisite

 Documentation/dev-tools/kselftest.rst         |   1 +
 tools/testing/selftests/mm/.gitignore         |   2 +
 tools/testing/selftests/mm/Makefile           |   7 +-
 tools/testing/selftests/mm/cow.c              |   7 --
 tools/testing/selftests/mm/hugepage-mremap.c  |   2 +-
 tools/testing/selftests/mm/hugetlb-madvise.c  |   8 +-
 tools/testing/selftests/mm/khugepaged.c       |  10 --
 .../selftests/mm/ksm_functional_tests.c       |   2 +-
 tools/testing/selftests/mm/migration.c        |   5 +-
 tools/testing/selftests/mm/mlock2-tests.c     |   1 -
 tools/testing/selftests/mm/pkey-x86.h         |   2 +-
 tools/testing/selftests/mm/run_vmtests.sh     |   6 +-
 tools/testing/selftests/mm/uffd-common.c      | 105 +++++++++++++++++
 tools/testing/selftests/mm/uffd-common.h      |  12 +-
 tools/testing/selftests/mm/uffd-stress.c      |  10 --
 tools/testing/selftests/mm/uffd-unit-tests.c  |  16 +--
 tools/testing/selftests/mm/vm_util.c          | 106 ++----------------
 tools/testing/selftests/mm/vm_util.h          |  36 ++----
 18 files changed, 165 insertions(+), 173 deletions(-)


base-commit: 929ed21dfdb6ee94391db51c9eedb63314ef6847
-- 
2.40.1

[PATCH v2 01/11] selftests/mm: fix uffd-stress unused function warning

[John Hubbard]

uffd_minor_feature() was unused. Remove it in order to fix the
associated clang build warning.

Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/uffd-stress.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index f1ad9eef1c3a..995ff13e74c7 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -88,16 +88,6 @@ static void uffd_stats_reset(struct uffd_args *args, unsigned long n_cpus)
 	}
 }
 
-static inline uint64_t uffd_minor_feature(void)
-{
-	if (test_type == TEST_HUGETLB && map_shared)
-		return UFFD_FEATURE_MINOR_HUGETLBFS;
-	else if (test_type == TEST_SHMEM)
-		return UFFD_FEATURE_MINOR_SHMEM;
-	else
-		return 0;
-}
-
 static void *locking_thread(void *arg)
 {
 	unsigned long cpu = (unsigned long) arg;
-- 
2.40.1

[PATCH v2 02/11] selftests/mm: fix unused variable warnings in hugetlb-madvise.c, migration.c

[John Hubbard]

Dummy variables are required in order to make these two (similar)
routines work, so in both cases, declare the variables as volatile in
order to avoid the clang compiler warning.

Furthermore, in order to ensure that each test actually does what is
intended, add an asm volatile invocation (thanks to David Hildenbrand
for the suggestion), with a clarifying comment so that it survives
future maintenance.

Cc: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/hugetlb-madvise.c | 8 ++++++--
 tools/testing/selftests/mm/migration.c       | 5 ++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/mm/hugetlb-madvise.c b/tools/testing/selftests/mm/hugetlb-madvise.c
index 28426e30d9bc..d55322df4b73 100644
--- a/tools/testing/selftests/mm/hugetlb-madvise.c
+++ b/tools/testing/selftests/mm/hugetlb-madvise.c
@@ -65,11 +65,15 @@ void write_fault_pages(void *addr, unsigned long nr_pages)
 
 void read_fault_pages(void *addr, unsigned long nr_pages)
 {
-	unsigned long dummy = 0;
+	volatile unsigned long dummy = 0;
 	unsigned long i;
 
-	for (i = 0; i < nr_pages; i++)
+	for (i = 0; i < nr_pages; i++) {
 		dummy += *((unsigned long *)(addr + (i * huge_page_size)));
+
+		/* Prevent the compiler from optimizing out the entire loop: */
+		asm volatile("" : "+r" (dummy));
+	}
 }
 
 int main(int argc, char **argv)
diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c
index 1cec8425e3ca..379581567f27 100644
--- a/tools/testing/selftests/mm/migration.c
+++ b/tools/testing/selftests/mm/migration.c
@@ -95,12 +95,15 @@ int migrate(uint64_t *ptr, int n1, int n2)
 
 void *access_mem(void *ptr)
 {
-	uint64_t y = 0;
+	volatile uint64_t y = 0;
 	volatile uint64_t *x = ptr;
 
 	while (1) {
 		pthread_testcancel();
 		y += *x;
+
+		/* Prevent the compiler from optimizing out the writes to y: */
+		asm volatile("" : "+r" (y));
 	}
 
 	return NULL;
-- 
2.40.1

[PATCH v2 03/11] selftests/mm: fix "warning: expression which evaluates to zero..." in mlock2-tests.c

[John Hubbard]

The stop variable is a char*, and the code was assigning a char value to
it. This was generating a warning when compiling with clang.

However, as both David and Peter pointed out, stop is not even used
after the problematic assignment to a char type. So just delete that
line entirely.

Cc: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/mlock2-tests.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
index 11b2301f3aa3..80cddc0de206 100644
--- a/tools/testing/selftests/mm/mlock2-tests.c
+++ b/tools/testing/selftests/mm/mlock2-tests.c
@@ -50,7 +50,6 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
 			printf("cannot parse /proc/self/maps\n");
 			goto out;
 		}
-		stop = '\0';
 
 		sscanf(line, "%lx", &start);
 		sscanf(end_addr, "%lx", &end);
-- 
2.40.1

[PATCH v2 04/11] selftests/mm: fix invocation of tests that are run via shell scripts

[John Hubbard]

We cannot depend upon git to reliably retain the executable bit on shell
scripts, or so I was told several years ago while working on this same
run_vmtests.sh script. And sure enough, things such as test_hmm.sh are
lately failing to run, due to lacking execute permissions.

Fix this by explicitly adding "bash" to each of the shell script
invocations. Leave fixing the overall approach to another day.

Acked-by: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/run_vmtests.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
index 4893eb60d96d..8f81432e4bac 100644
--- a/tools/testing/selftests/mm/run_vmtests.sh
+++ b/tools/testing/selftests/mm/run_vmtests.sh
@@ -242,18 +242,18 @@ if [ $VADDR64 -ne 0 ]; then
 	if [ "$ARCH" == "$ARCH_ARM64" ]; then
 		echo 6 > /proc/sys/vm/nr_hugepages
 	fi
-	CATEGORY="hugevm" run_test ./va_high_addr_switch.sh
+	CATEGORY="hugevm" run_test bash ./va_high_addr_switch.sh
 	if [ "$ARCH" == "$ARCH_ARM64" ]; then
 		echo $prev_nr_hugepages > /proc/sys/vm/nr_hugepages
 	fi
 fi # VADDR64
 
 # vmalloc stability smoke test
-CATEGORY="vmalloc" run_test ./test_vmalloc.sh smoke
+CATEGORY="vmalloc" run_test bash ./test_vmalloc.sh smoke
 
 CATEGORY="mremap" run_test ./mremap_dontunmap
 
-CATEGORY="hmm" run_test ./test_hmm.sh smoke
+CATEGORY="hmm" run_test bash ./test_hmm.sh smoke
 
 # MADV_POPULATE_READ and MADV_POPULATE_WRITE tests
 CATEGORY="madv_populate" run_test ./madv_populate
-- 
2.40.1

[PATCH v2 05/11] selftests/mm: .gitignore: add mkdirty, va_high_addr_switch

[John Hubbard]

These new build products were left out of .gitignore, so add them now.

Reviewed-by: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/.gitignore | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index 8917455f4f51..ab215303d8e9 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -39,3 +39,5 @@ local_config.h
 local_config.mk
 ksm_functional_tests
 mdwe_test
+mkdirty
+va_high_addr_switch
\ No newline at end of file
-- 
2.40.1

[PATCH v2 06/11] selftests/mm: fix two -Wformat-security warnings in uffd builds

[John Hubbard]

The uffd tests generate two compile time warnings from clang's
-Wformat-security setting. These trigger at the call sites for
uffd_test_start() and uffd_test_skip().

1) Fix the uffd_test_start() issue by removing the intermediate
test_name variable (thanks to David Hildenbrand for showing how to do
this).

2) Fix the uffd_test_skip() issue by observing that there is no need for
a macro and a variable args approach, because all callers of
uffd_test_skip() pass in a simple char* string, without any format
specifiers. So just change uffd_test_skip() into a regular C function.

Cc: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/uffd-unit-tests.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
index 269c86768a02..04d91f144d1c 100644
--- a/tools/testing/selftests/mm/uffd-unit-tests.c
+++ b/tools/testing/selftests/mm/uffd-unit-tests.c
@@ -109,12 +109,11 @@ static void uffd_test_pass(void)
 		ksft_inc_fail_cnt();		\
 	} while (0)
 
-#define  uffd_test_skip(...)  do {		\
-		printf("skipped [reason: ");	\
-		printf(__VA_ARGS__);		\
-		printf("]\n");			\
-		ksft_inc_xskip_cnt();		\
-	} while (0)
+static void uffd_test_skip(const char *message)
+{
+	printf("skipped [reason: %s]\n", message);
+	ksft_inc_xskip_cnt();
+}
 
 /*
  * Returns 1 if specific userfaultfd supported, 0 otherwise.  Note, we'll
@@ -1149,7 +1148,6 @@ int main(int argc, char *argv[])
 	uffd_test_case_t *test;
 	mem_type_t *mem_type;
 	uffd_test_args_t args;
-	char test_name[128];
 	const char *errmsg;
 	int has_uffd, opt;
 	int i, j;
@@ -1192,10 +1190,8 @@ int main(int argc, char *argv[])
 			mem_type = &mem_types[j];
 			if (!(test->mem_targets & mem_type->mem_flag))
 				continue;
-			snprintf(test_name, sizeof(test_name),
-				 "%s on %s", test->name, mem_type->name);
 
-			uffd_test_start(test_name);
+			uffd_test_start("%s on %s", test->name, mem_type->name);
 			if (!uffd_feature_supported(test)) {
 				uffd_test_skip("feature missing");
 				continue;
-- 
2.40.1

[PATCH v2 07/11] selftests/mm: fix a "possibly uninitialized" warning in pkey-x86.h

[John Hubbard]

This fixes a real bug, too, because xstate_size()  was assuming that
the stack variable xstate_size was initialized to zero. That's not
guaranteed nor even especially likely.

Reviewed-by: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/pkey-x86.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/pkey-x86.h b/tools/testing/selftests/mm/pkey-x86.h
index 72c14cd3ddc7..e32ae8a1cd99 100644
--- a/tools/testing/selftests/mm/pkey-x86.h
+++ b/tools/testing/selftests/mm/pkey-x86.h
@@ -132,7 +132,7 @@ int pkey_reg_xstate_offset(void)
 	unsigned int ecx;
 	unsigned int edx;
 	int xstate_offset;
-	int xstate_size;
+	int xstate_size = 0;
 	unsigned long XSTATE_CPUID = 0xd;
 	int leaf;
 
-- 
2.40.1

[PATCH v2 08/11] selftests/mm: fix uffd-unit-tests.c build failure due to missing MADV_COLLAPSE

[John Hubbard]

MADV_PAGEOUT, MADV_POPULATE_READ, MADV_COLLAPSE are conditionally
defined as necessary. However, that was being done in .c files, and a
new build failure came up that would have been automatically avoided had
these been in a common header file.

So consolidate and move them all to vm_util.h, which fixes the build
failure.

An alternative approach from Muhammad Usama Anjum was: rely on "make
headers" being required, and include asm-generic/mman-common.h. This
works in the sense that it builds, but it still generates warnings about
duplicate MADV_* symbols, and the goal here is to get a fully clean (no
warnings) build here.

Reviewed-by: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/cow.c        |  7 -------
 tools/testing/selftests/mm/khugepaged.c | 10 ----------
 tools/testing/selftests/mm/vm_util.h    | 10 ++++++++++
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c
index dc9d6fe86028..8882b05ec9c8 100644
--- a/tools/testing/selftests/mm/cow.c
+++ b/tools/testing/selftests/mm/cow.c
@@ -30,13 +30,6 @@
 #include "../kselftest.h"
 #include "vm_util.h"
 
-#ifndef MADV_PAGEOUT
-#define MADV_PAGEOUT 21
-#endif
-#ifndef MADV_COLLAPSE
-#define MADV_COLLAPSE 25
-#endif
-
 static size_t pagesize;
 static int pagemap_fd;
 static size_t thpsize;
diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
index 97adc0f34f9c..e88ee039d0eb 100644
--- a/tools/testing/selftests/mm/khugepaged.c
+++ b/tools/testing/selftests/mm/khugepaged.c
@@ -22,16 +22,6 @@
 
 #include "vm_util.h"
 
-#ifndef MADV_PAGEOUT
-#define MADV_PAGEOUT 21
-#endif
-#ifndef MADV_POPULATE_READ
-#define MADV_POPULATE_READ 22
-#endif
-#ifndef MADV_COLLAPSE
-#define MADV_COLLAPSE 25
-#endif
-
 #define BASE_ADDR ((void *)(1UL << 30))
 static unsigned long hpage_pmd_size;
 static unsigned long page_size;
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index b950bd16083a..07f39ed2efba 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -63,3 +63,13 @@ int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
 
 #define PAGEMAP_PRESENT(ent)	(((ent) & (1ull << 63)) != 0)
 #define PAGEMAP_PFN(ent)	((ent) & ((1ull << 55) - 1))
+
+#ifndef MADV_PAGEOUT
+#define MADV_PAGEOUT 21
+#endif
+#ifndef MADV_POPULATE_READ
+#define MADV_POPULATE_READ 22
+#endif
+#ifndef MADV_COLLAPSE
+#define MADV_COLLAPSE 25
+#endif
-- 
2.40.1

[PATCH v2 09/11] selftests/mm: move psize(), pshift() into vm_utils.c

[John Hubbard]

This is in preparation for linking test programs with both vm_utils.c
and uffd-common.c. The static inline routines, while normally not a
problem, in this case complicate an already fragile header file
situation: the header files including other header files leads to
compilation failures in a subsequent patch that moves code around.

Anyway, there is no particular need for inlining here, so turn these
into normal functions, as a workaround to avoid refactoring the header
file includes for now.

Cc: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/vm_util.c | 14 ++++++++++++++
 tools/testing/selftests/mm/vm_util.h | 16 ++--------------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index 9b06a5034808..01296c17df02 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -301,3 +301,17 @@ int uffd_get_features(uint64_t *features)
 
 	return 0;
 }
+
+unsigned int psize(void)
+{
+	if (!__page_size)
+		__page_size = sysconf(_SC_PAGESIZE);
+	return __page_size;
+}
+
+unsigned int pshift(void)
+{
+	if (!__page_shift)
+		__page_shift = (ffsl(psize()) - 1);
+	return __page_shift;
+}
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index 07f39ed2efba..8aa543a3678b 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -3,7 +3,6 @@
 #include <stdbool.h>
 #include <sys/mman.h>
 #include <err.h>
-#include <string.h> /* ffsl() */
 #include <unistd.h> /* _SC_PAGESIZE */
 
 #define BIT_ULL(nr)                   (1ULL << (nr))
@@ -17,19 +16,8 @@
 extern unsigned int __page_size;
 extern unsigned int __page_shift;
 
-static inline unsigned int psize(void)
-{
-	if (!__page_size)
-		__page_size = sysconf(_SC_PAGESIZE);
-	return __page_size;
-}
-
-static inline unsigned int pshift(void)
-{
-	if (!__page_shift)
-		__page_shift = (ffsl(psize()) - 1);
-	return __page_shift;
-}
+unsigned int psize(void);
+unsigned int pshift(void);
 
 uint64_t pagemap_get_entry(int fd, char *start);
 bool pagemap_is_softdirty(int fd, char *start);
-- 
2.40.1

[PATCH v2 10/11] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[John Hubbard]

Move the uffd*() routines to their natural home. Note that
ksm_functional_tests.c also depend, intentionally (due to a recent
commit [1]), upon uffd-common.[ch].

Cc: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/Makefile           |   7 +-
 tools/testing/selftests/mm/hugepage-mremap.c  |   2 +-
 .../selftests/mm/ksm_functional_tests.c       |   2 +-
 tools/testing/selftests/mm/uffd-common.c      | 105 ++++++++++++++++++
 tools/testing/selftests/mm/uffd-common.h      |  12 +-
 tools/testing/selftests/mm/vm_util.c          | 104 -----------------
 tools/testing/selftests/mm/vm_util.h          |  10 --
 7 files changed, 122 insertions(+), 120 deletions(-)

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 23af4633f0f4..a15572758954 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -109,8 +109,11 @@ include ../lib.mk
 
 $(TEST_GEN_PROGS): vm_util.c
 
-$(OUTPUT)/uffd-stress: uffd-common.c
-$(OUTPUT)/uffd-unit-tests: uffd-common.c
+$(OUTPUT)/uffd-stress:          uffd-common.c
+$(OUTPUT)/uffd-unit-tests:      uffd-common.c
+$(OUTPUT)/hugepage-mremap:      uffd-common.c
+$(OUTPUT)/write_to_hugetlbfs:   uffd-common.c
+$(OUTPUT)/ksm_functional_tests: uffd-common.c
 
 ifeq ($(MACHINE),x86_64)
 BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32))
diff --git a/tools/testing/selftests/mm/hugepage-mremap.c b/tools/testing/selftests/mm/hugepage-mremap.c
index cabd0084f57b..8158fe909f5e 100644
--- a/tools/testing/selftests/mm/hugepage-mremap.c
+++ b/tools/testing/selftests/mm/hugepage-mremap.c
@@ -24,7 +24,7 @@
 #include <sys/ioctl.h>
 #include <string.h>
 #include <stdbool.h>
-#include "vm_util.h"
+#include "uffd-common.h"
 
 #define DEFAULT_LENGTH_MB 10UL
 #define MB_TO_BYTES(x) (x * 1024 * 1024)
diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
index 26853badae70..648188ad73fa 100644
--- a/tools/testing/selftests/mm/ksm_functional_tests.c
+++ b/tools/testing/selftests/mm/ksm_functional_tests.c
@@ -22,7 +22,7 @@
 #include <linux/userfaultfd.h>
 
 #include "../kselftest.h"
-#include "vm_util.h"
+#include "uffd-common.h"
 
 #define KiB 1024u
 #define MiB (1024 * KiB)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
index 61c6250adf93..e1ad63668a05 100644
--- a/tools/testing/selftests/mm/uffd-common.c
+++ b/tools/testing/selftests/mm/uffd-common.c
@@ -6,6 +6,7 @@
  */
 
 #include "uffd-common.h"
+#include "vm_util.h"
 
 #define BASE_PMD_ADDR ((void *)(1UL << 30))
 
@@ -616,3 +617,107 @@ int copy_page(int ufd, unsigned long offset, bool wp)
 {
 	return __copy_page(ufd, offset, false, wp);
 }
+
+/* If `ioctls' non-NULL, the allowed ioctls will be returned into the var */
+int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
+			      bool miss, bool wp, bool minor, uint64_t *ioctls)
+{
+	struct uffdio_register uffdio_register = { 0 };
+	uint64_t mode = 0;
+	int ret = 0;
+
+	if (miss)
+		mode |= UFFDIO_REGISTER_MODE_MISSING;
+	if (wp)
+		mode |= UFFDIO_REGISTER_MODE_WP;
+	if (minor)
+		mode |= UFFDIO_REGISTER_MODE_MINOR;
+
+	uffdio_register.range.start = (unsigned long)addr;
+	uffdio_register.range.len = len;
+	uffdio_register.mode = mode;
+
+	if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
+		ret = -errno;
+	else if (ioctls)
+		*ioctls = uffdio_register.ioctls;
+
+	return ret;
+}
+
+int uffd_register(int uffd, void *addr, uint64_t len,
+		  bool miss, bool wp, bool minor)
+{
+	return uffd_register_with_ioctls(uffd, addr, len,
+					 miss, wp, minor, NULL);
+}
+
+int uffd_unregister(int uffd, void *addr, uint64_t len)
+{
+	struct uffdio_range range = { .start = (uintptr_t)addr, .len = len };
+	int ret = 0;
+
+	if (ioctl(uffd, UFFDIO_UNREGISTER, &range) == -1)
+		ret = -errno;
+
+	return ret;
+}
+
+int uffd_open_dev(unsigned int flags)
+{
+	int fd, uffd;
+
+	fd = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC);
+	if (fd < 0)
+		return fd;
+	uffd = ioctl(fd, USERFAULTFD_IOC_NEW, flags);
+	close(fd);
+
+	return uffd;
+}
+
+int uffd_open_sys(unsigned int flags)
+{
+#ifdef __NR_userfaultfd
+	return syscall(__NR_userfaultfd, flags);
+#else
+	return -1;
+#endif
+}
+
+int uffd_open(unsigned int flags)
+{
+	int uffd = uffd_open_sys(flags);
+
+	if (uffd < 0)
+		uffd = uffd_open_dev(flags);
+
+	return uffd;
+}
+
+int uffd_get_features(uint64_t *features)
+{
+	struct uffdio_api uffdio_api = { .api = UFFD_API, .features = 0 };
+	/*
+	 * This should by default work in most kernels; the feature list
+	 * will be the same no matter what we pass in here.
+	 */
+	int fd = uffd_open(UFFD_USER_MODE_ONLY);
+
+	if (fd < 0)
+		/* Maybe the kernel is older than user-only mode? */
+		fd = uffd_open(0);
+
+	if (fd < 0)
+		return fd;
+
+	if (ioctl(fd, UFFDIO_API, &uffdio_api)) {
+		close(fd);
+		return -errno;
+	}
+
+	*features = uffdio_api.features;
+	close(fd);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
index 6068f2346b86..a1cdb78c0762 100644
--- a/tools/testing/selftests/mm/uffd-common.h
+++ b/tools/testing/selftests/mm/uffd-common.h
@@ -19,8 +19,6 @@
 #include <signal.h>
 #include <poll.h>
 #include <string.h>
-#include <linux/mman.h>
-#include <sys/mman.h>
 #include <sys/syscall.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
@@ -110,6 +108,16 @@ int __copy_page(int ufd, unsigned long offset, bool retry, bool wp);
 int copy_page(int ufd, unsigned long offset, bool wp);
 void *uffd_poll_thread(void *arg);
 
+int uffd_register(int uffd, void *addr, uint64_t len,
+		  bool miss, bool wp, bool minor);
+int uffd_unregister(int uffd, void *addr, uint64_t len);
+int uffd_open_dev(unsigned int flags);
+int uffd_open_sys(unsigned int flags);
+int uffd_open(unsigned int flags);
+int uffd_get_features(uint64_t *features);
+int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
+			      bool miss, bool wp, bool minor, uint64_t *ioctls);
+
 #define TEST_ANON	1
 #define TEST_HUGETLB	2
 #define TEST_SHMEM	3
diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index 01296c17df02..c64a0134f83c 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -198,110 +198,6 @@ unsigned long default_huge_page_size(void)
 	return hps;
 }
 
-/* If `ioctls' non-NULL, the allowed ioctls will be returned into the var */
-int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
-			      bool miss, bool wp, bool minor, uint64_t *ioctls)
-{
-	struct uffdio_register uffdio_register = { 0 };
-	uint64_t mode = 0;
-	int ret = 0;
-
-	if (miss)
-		mode |= UFFDIO_REGISTER_MODE_MISSING;
-	if (wp)
-		mode |= UFFDIO_REGISTER_MODE_WP;
-	if (minor)
-		mode |= UFFDIO_REGISTER_MODE_MINOR;
-
-	uffdio_register.range.start = (unsigned long)addr;
-	uffdio_register.range.len = len;
-	uffdio_register.mode = mode;
-
-	if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
-		ret = -errno;
-	else if (ioctls)
-		*ioctls = uffdio_register.ioctls;
-
-	return ret;
-}
-
-int uffd_register(int uffd, void *addr, uint64_t len,
-		  bool miss, bool wp, bool minor)
-{
-	return uffd_register_with_ioctls(uffd, addr, len,
-					 miss, wp, minor, NULL);
-}
-
-int uffd_unregister(int uffd, void *addr, uint64_t len)
-{
-	struct uffdio_range range = { .start = (uintptr_t)addr, .len = len };
-	int ret = 0;
-
-	if (ioctl(uffd, UFFDIO_UNREGISTER, &range) == -1)
-		ret = -errno;
-
-	return ret;
-}
-
-int uffd_open_dev(unsigned int flags)
-{
-	int fd, uffd;
-
-	fd = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC);
-	if (fd < 0)
-		return fd;
-	uffd = ioctl(fd, USERFAULTFD_IOC_NEW, flags);
-	close(fd);
-
-	return uffd;
-}
-
-int uffd_open_sys(unsigned int flags)
-{
-#ifdef __NR_userfaultfd
-	return syscall(__NR_userfaultfd, flags);
-#else
-	return -1;
-#endif
-}
-
-int uffd_open(unsigned int flags)
-{
-	int uffd = uffd_open_sys(flags);
-
-	if (uffd < 0)
-		uffd = uffd_open_dev(flags);
-
-	return uffd;
-}
-
-int uffd_get_features(uint64_t *features)
-{
-	struct uffdio_api uffdio_api = { .api = UFFD_API, .features = 0 };
-	/*
-	 * This should by default work in most kernels; the feature list
-	 * will be the same no matter what we pass in here.
-	 */
-	int fd = uffd_open(UFFD_USER_MODE_ONLY);
-
-	if (fd < 0)
-		/* Maybe the kernel is older than user-only mode? */
-		fd = uffd_open(0);
-
-	if (fd < 0)
-		return fd;
-
-	if (ioctl(fd, UFFDIO_API, &uffdio_api)) {
-		close(fd);
-		return -errno;
-	}
-
-	*features = uffdio_api.features;
-	close(fd);
-
-	return 0;
-}
-
 unsigned int psize(void)
 {
 	if (!__page_size)
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index 8aa543a3678b..f04f82771cd0 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -33,16 +33,6 @@ bool check_huge_shmem(void *addr, int nr_hpages, uint64_t hpage_size);
 int64_t allocate_transhuge(void *ptr, int pagemap_fd);
 unsigned long default_huge_page_size(void);
 
-int uffd_register(int uffd, void *addr, uint64_t len,
-		  bool miss, bool wp, bool minor);
-int uffd_unregister(int uffd, void *addr, uint64_t len);
-int uffd_open_dev(unsigned int flags);
-int uffd_open_sys(unsigned int flags);
-int uffd_open(unsigned int flags);
-int uffd_get_features(uint64_t *features);
-int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
-			      bool miss, bool wp, bool minor, uint64_t *ioctls);
-
 /*
  * On ppc64 this will only work with radix 2M hugepage size
  */
-- 
2.40.1

Please disregard all of the selftest patches [PATCH v2 00/11] A minor flurry of selftest/mm fixes

[John Hubbard]

On 6/19/23 18:17, John Hubbard wrote:

My script picked up stale patches! Apologies!

...the first one, "[PATCH] mm/memory_hotplug.c: don't fail
hot unplug quite so eagerly", is the only one I wanted to
send. arghhh


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

[David Hildenbrand]

On 20.06.23 03:17, John Hubbard wrote:
> mm/memory_hotplug.c: don't fail hot unplug quite so eagerly
> 
> Some device drivers add memory to the system via memory hotplug. When
> the driver is unloaded, that memory is hot-unplugged.

Which interfaces are they using to add/remove memory?

> 
> However, memory hot unplug can fail. And these days, it fails a little
> too easily, with respect to the above case. Specifically, if a signal is
> pending on the process, hot unplug fails. This leads directly to: the
> user must reboot the machine in order to unload the driver, and
> therefore the device is unusable until the machine is rebooted.

Why can't they retry in user space when offlining fails with -EINTR, or 
re-trigger driver unloading?

> 
> During teardown paths in the kernel, a higher tolerance for failures or
> imperfections is often best. That is, it is often better to continue
> with the teardown, than to error out too early.
> 
> So in this case, other things (unmovable pages, un-splittable huge
> pages) can also cause the above problem. However, those are demonstrably
> less common than simply having a pending signal. I've got bug reports
> from users who can trivially reproduce this by killing their process
> with a "kill -9", for example.
> 
> Fix this by soldering on with memory hot plug, even in the presence of
> pending signals.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>   mm/memory_hotplug.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 8e0fa209d533..57a46620a667 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1879,12 +1879,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>   	do {
>   		pfn = start_pfn;
>   		do {
> -			if (signal_pending(current)) {
> -				ret = -EINTR;
> -				reason = "signal backoff";
> -				goto failed_removal_isolated;
> -			}
> -
>   			cond_resched();
>   
>   			ret = scan_movable_pages(pfn, end_pfn, &pfn);

No, we can't remove that. It's documented behavior that exists precisely 
for that reason:

https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#id21

"
When offlining is triggered from user space, the offlining context can 
be terminated by sending a fatal signal. A timeout based offlining can 
easily be implemented via:

% timeout $TIMEOUT offline_block | failure_handling
"

Otherwise, there is no way to stop an userspace-triggered offline 
operation that loops forever in the kernel.

I guess switching to fatal_signal_pending() might help to some degree, 
it should keep the timeout trick working.

But it wouldn't help in your case because where root kills arbitrary 
processes. I'm not sure if that is something we should be paying 
attention to.


-- 
Cheers,

David / dhildenb

Re: [PATCH v2 08/11] selftests/mm: fix uffd-unit-tests.c build failure due to missing MADV_COLLAPSE

[Muhammad Usama Anjum]

On 6/20/23 6:17 AM, John Hubbard wrote:
> MADV_PAGEOUT, MADV_POPULATE_READ, MADV_COLLAPSE are conditionally
> defined as necessary. However, that was being done in .c files, and a
> new build failure came up that would have been automatically avoided had
> these been in a common header file.
> 
> So consolidate and move them all to vm_util.h, which fixes the build
> failure.
> 
> An alternative approach from Muhammad Usama Anjum was: rely on "make
> headers" being required, and include asm-generic/mman-common.h. This
> works in the sense that it builds, but it still generates warnings about
> duplicate MADV_* symbols, and the goal here is to get a fully clean (no
> warnings) build here.
I've not looked in detail. But it seems like your first revision was merged
and after that my cleanup has also been merged. My cleanup patch is adding
correct header files and removing these duplicate defines: It is in
mm-stable now.
https://lore.kernel.org/all/20230619232244.81CB3C433C0@smtp.kernel.org

> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/mm/cow.c        |  7 -------
>  tools/testing/selftests/mm/khugepaged.c | 10 ----------
>  tools/testing/selftests/mm/vm_util.h    | 10 ++++++++++
>  3 files changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c
> index dc9d6fe86028..8882b05ec9c8 100644
> --- a/tools/testing/selftests/mm/cow.c
> +++ b/tools/testing/selftests/mm/cow.c
> @@ -30,13 +30,6 @@
>  #include "../kselftest.h"
>  #include "vm_util.h"
>  
> -#ifndef MADV_PAGEOUT
> -#define MADV_PAGEOUT 21
> -#endif
> -#ifndef MADV_COLLAPSE
> -#define MADV_COLLAPSE 25
> -#endif
> -
>  static size_t pagesize;
>  static int pagemap_fd;
>  static size_t thpsize;
> diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
> index 97adc0f34f9c..e88ee039d0eb 100644
> --- a/tools/testing/selftests/mm/khugepaged.c
> +++ b/tools/testing/selftests/mm/khugepaged.c
> @@ -22,16 +22,6 @@
>  
>  #include "vm_util.h"
>  
> -#ifndef MADV_PAGEOUT
> -#define MADV_PAGEOUT 21
> -#endif
> -#ifndef MADV_POPULATE_READ
> -#define MADV_POPULATE_READ 22
> -#endif
> -#ifndef MADV_COLLAPSE
> -#define MADV_COLLAPSE 25
> -#endif
> -
>  #define BASE_ADDR ((void *)(1UL << 30))
>  static unsigned long hpage_pmd_size;
>  static unsigned long page_size;
> diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
> index b950bd16083a..07f39ed2efba 100644
> --- a/tools/testing/selftests/mm/vm_util.h
> +++ b/tools/testing/selftests/mm/vm_util.h
> @@ -63,3 +63,13 @@ int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
>  
>  #define PAGEMAP_PRESENT(ent)	(((ent) & (1ull << 63)) != 0)
>  #define PAGEMAP_PFN(ent)	((ent) & ((1ull << 55) - 1))
> +
> +#ifndef MADV_PAGEOUT
> +#define MADV_PAGEOUT 21
> +#endif
> +#ifndef MADV_POPULATE_READ
> +#define MADV_POPULATE_READ 22
> +#endif
> +#ifndef MADV_COLLAPSE
> +#define MADV_COLLAPSE 25
> +#endif

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH v2 08/11] selftests/mm: fix uffd-unit-tests.c build failure due to missing MADV_COLLAPSE

[David Hildenbrand]

On 20.06.23 12:17, Muhammad Usama Anjum wrote:
> On 6/20/23 6:17 AM, John Hubbard wrote:
>> MADV_PAGEOUT, MADV_POPULATE_READ, MADV_COLLAPSE are conditionally
>> defined as necessary. However, that was being done in .c files, and a
>> new build failure came up that would have been automatically avoided had
>> these been in a common header file.
>>
>> So consolidate and move them all to vm_util.h, which fixes the build
>> failure.
>>
>> An alternative approach from Muhammad Usama Anjum was: rely on "make
>> headers" being required, and include asm-generic/mman-common.h. This
>> works in the sense that it builds, but it still generates warnings about
>> duplicate MADV_* symbols, and the goal here is to get a fully clean (no
>> warnings) build here.
> I've not looked in detail. But it seems like your first revision was merged
> and after that my cleanup has also been merged. My cleanup patch is adding
> correct header files and removing these duplicate defines: It is in
> mm-stable now.
> https://lore.kernel.org/all/20230619232244.81CB3C433C0@smtp.kernel.org

See

https://lkml.kernel.org/r/0379db8e-744d-2876-7304-2a6db8c9cac0@nvidia.com

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

[John Hubbard]

On 6/20/23 00:12, David Hildenbrand wrote:
> On 20.06.23 03:17, John Hubbard wrote:
>> mm/memory_hotplug.c: don't fail hot unplug quite so eagerly
>>
>> Some device drivers add memory to the system via memory hotplug. When
>> the driver is unloaded, that memory is hot-unplugged.
> 
> Which interfaces are they using to add/remove memory?

It's coming in from the kernel driver, like this:

offline_and_remove_memory()
     walk_memory_blocks()
         try_offline_memory_block()
             device_offline()
                 memory_subsys_offline()
                     offline_pages()

...and the above is getting invoked as part of killing a user space
process that was helping (for performance reasons) holding the device
nodes open. That triggers a final close of the file descriptors and
leads to tearing down the driver. The teardown succeeds even though
the memory was not offlined, and now everything is, to use a technical
term, "stuck". :)

More below...

> 
>>
>> However, memory hot unplug can fail. And these days, it fails a little
>> too easily, with respect to the above case. Specifically, if a signal is
>> pending on the process, hot unplug fails. This leads directly to: the
>> user must reboot the machine in order to unload the driver, and
>> therefore the device is unusable until the machine is rebooted.
> 
> Why can't they retry in user space when offlining fails with -EINTR, or re-trigger driver unloading?

If someone uses "kill -9" to kill that process, then we get here,
because user space cannot trap that signal.


...
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1879,12 +1879,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>       do {
>>           pfn = start_pfn;
>>           do {
>> -            if (signal_pending(current)) {
>> -                ret = -EINTR;
>> -                reason = "signal backoff";
>> -                goto failed_removal_isolated;
>> -            }
>> -
>>               cond_resched();
>>               ret = scan_movable_pages(pfn, end_pfn, &pfn);
> 
> No, we can't remove that. It's documented behavior that exists precisely for that reason:
> 
> https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#id21
> 
> "
> When offlining is triggered from user space, the offlining context can be terminated by sending a fatal signal. A timeout based offlining can easily be implemented via:
> 
> % timeout $TIMEOUT offline_block | failure_handling
> "
> 
> Otherwise, there is no way to stop an userspace-triggered offline operation that loops forever in the kernel.

OK yes, I see.

> 
> I guess switching to fatal_signal_pending() might help to some degree, it should keep the timeout trick working.
> 
> But it wouldn't help in your case because where root kills arbitrary processes. I'm not sure if that is something we should be paying attention to.
> 

Right. I think it would be more accurate perhaps, but it wouldn't help
this particular complaint.

Perhaps it is reasonable to claim that, "well, kill -9 *means* that you
end up here!" :) And the above patch clearly is not the way to go, but...

...what about discerning between "user initiated offline_pages" and
"offline pages as part of a driver shutdown/unload"?

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

[David Hildenbrand]

On 20.06.23 23:54, John Hubbard wrote:
> On 6/20/23 00:12, David Hildenbrand wrote:
>> On 20.06.23 03:17, John Hubbard wrote:
>>> mm/memory_hotplug.c: don't fail hot unplug quite so eagerly
>>>
>>> Some device drivers add memory to the system via memory hotplug. When
>>> the driver is unloaded, that memory is hot-unplugged.
>>
>> Which interfaces are they using to add/remove memory?
> 
> It's coming in from the kernel driver, like this:
> 
> offline_and_remove_memory()
>       walk_memory_blocks()
>           try_offline_memory_block()
>               device_offline()
>                   memory_subsys_offline()
>                       offline_pages()
> 
> ...and the above is getting invoked as part of killing a user space
> process that was helping (for performance reasons) holding the device
> nodes open. That triggers a final close of the file descriptors and
> leads to tearing down the driver. The teardown succeeds even though
> the memory was not offlined, and now everything is, to use a technical
> term, "stuck". :)
> 

Ah, I see, thanks! I thought it would just be offlining from user space.

> More below...
> 
>>
>>>
>>> However, memory hot unplug can fail. And these days, it fails a little
>>> too easily, with respect to the above case. Specifically, if a signal is
>>> pending on the process, hot unplug fails. This leads directly to: the
>>> user must reboot the machine in order to unload the driver, and
>>> therefore the device is unusable until the machine is rebooted.
>>
>> Why can't they retry in user space when offlining fails with -EINTR, or re-trigger driver unloading?
> 
> If someone uses "kill -9" to kill that process, then we get here,
> because user space cannot trap that signal.

Understood, thanks!

> 
> 
> ...
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1879,12 +1879,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>>        do {
>>>            pfn = start_pfn;
>>>            do {
>>> -            if (signal_pending(current)) {
>>> -                ret = -EINTR;
>>> -                reason = "signal backoff";
>>> -                goto failed_removal_isolated;
>>> -            }
>>> -
>>>                cond_resched();
>>>                ret = scan_movable_pages(pfn, end_pfn, &pfn);
>>
>> No, we can't remove that. It's documented behavior that exists precisely for that reason:
>>
>> https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#id21
>>
>> "
>> When offlining is triggered from user space, the offlining context can be terminated by sending a fatal signal. A timeout based offlining can easily be implemented via:
>>
>> % timeout $TIMEOUT offline_block | failure_handling
>> "
>>
>> Otherwise, there is no way to stop an userspace-triggered offline operation that loops forever in the kernel.
> 
> OK yes, I see.
> 
>>
>> I guess switching to fatal_signal_pending() might help to some degree, it should keep the timeout trick working.
>>
>> But it wouldn't help in your case because where root kills arbitrary processes. I'm not sure if that is something we should be paying attention to.
>>
> 
> Right. I think it would be more accurate perhaps, but it wouldn't help
> this particular complaint.
> 
> Perhaps it is reasonable to claim that, "well, kill -9 *means* that you
> end up here!" :) And the above patch clearly is not the way to go, but...
> 
> ...what about discerning between "user initiated offline_pages" and
> "offline pages as part of a driver shutdown/unload"?

Makes sense to me.

There are two ways for triggering it directly from user space:

1) drivers/base/core.c:online_store()
2) drivers/base/memory.c:state_store()

We cannot easily hook into 2) to indicate "we're offlining directly
from user space". SO we might have to do it the other way around.


Something along the following lines should do the trick (expect whitespace damage):


diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 53ee7654f009..acd4b739505a 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -152,6 +152,13 @@ void put_online_mems(void)
  
  bool movable_node_enabled = false;
  
+/*
+ * Protected by the device hotplug lock. Indicates whether device offlining
+ * is triggered from try_offline_memory_block() such that we don't fail memory
+ * offlining if a signal is pending.
+ */
+static bool mhp_in_try_offline_memory_block;
+
  #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
  int mhp_default_online_type = MMOP_OFFLINE;
  #else
@@ -1860,7 +1867,8 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
         do {
                 pfn = start_pfn;
                 do {
-                       if (signal_pending(current)) {
+                       if (!mhp_in_try_offline_memory_block &&
+                           signal_pending(current)) {
                                 ret = -EINTR;
                                 reason = "signal backoff";
                                 goto failed_removal_isolated;
@@ -2177,7 +2185,9 @@ static int try_offline_memory_block(struct memory_block *mem, void *arg)
         if (page && zone_idx(page_zone(page)) == ZONE_MOVABLE)
                 online_type = MMOP_ONLINE_MOVABLE;
  
+       mhp_in_try_offline_memory_block = true;
         rc = device_offline(&mem->dev);
+       mhp_in_try_offline_memory_block = false;
         /*
          * Default is MMOP_OFFLINE - change it only if offlining succeeded,
          * so try_reonline_memory_block() can do the right thing.



There is still arch/powerpc/platforms/pseries/hotplug-memory.c that calls
device_offline() and would fail on signals (not sure if relevant, like for virtio-mem it
shouldn't be that relevant).

I guess dlpar_remove_lmb() can now simply call offline_and_remove_memory().
[I might craft a patch later]

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

[David Hildenbrand]

[...]

> 
> There is still arch/powerpc/platforms/pseries/hotplug-memory.c that calls
> device_offline() and would fail on signals (not sure if relevant, like for virtio-mem it
> shouldn't be that relevant).

Oh, and of course the ACPI-triggered device_offline().

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

[John Hubbard]

On 6/21/23 01:11, David Hildenbrand wrote:
>> ...what about discerning between "user initiated offline_pages" and
>> "offline pages as part of a driver shutdown/unload"?
> 
> Makes sense to me.
> 
> There are two ways for triggering it directly from user space:
> 
> 1) drivers/base/core.c:online_store()
> 2) drivers/base/memory.c:state_store()
> 
> We cannot easily hook into 2) to indicate "we're offlining directly
> from user space". SO we might have to do it the other way around.
> 
> 
> Something along the following lines should do the trick (expect whitespace damage):
> 
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 53ee7654f009..acd4b739505a 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -152,6 +152,13 @@ void put_online_mems(void)
> 
>   bool movable_node_enabled = false;
> 
> +/*
> + * Protected by the device hotplug lock. Indicates whether device offlining
> + * is triggered from try_offline_memory_block() such that we don't fail memory
> + * offlining if a signal is pending.
> + */
> +static bool mhp_in_try_offline_memory_block;
> +
>   #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
>   int mhp_default_online_type = MMOP_OFFLINE;
>   #else
> @@ -1860,7 +1867,8 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>          do {
>                  pfn = start_pfn;
>                  do {
> -                       if (signal_pending(current)) {
> +                       if (!mhp_in_try_offline_memory_block &&
> +                           signal_pending(current)) {
>                                  ret = -EINTR;
>                                  reason = "signal backoff";
>                                  goto failed_removal_isolated;
> @@ -2177,7 +2185,9 @@ static int try_offline_memory_block(struct memory_block *mem, void *arg)
>          if (page && zone_idx(page_zone(page)) == ZONE_MOVABLE)
>                  online_type = MMOP_ONLINE_MOVABLE;
> 
> +       mhp_in_try_offline_memory_block = true;
>          rc = device_offline(&mem->dev);
> +       mhp_in_try_offline_memory_block = false;
>          /*
>           * Default is MMOP_OFFLINE - change it only if offlining succeeded,
>           * so try_reonline_memory_block() can do the right thing.
> 
> 
> 
> There is still arch/powerpc/platforms/pseries/hotplug-memory.c that calls
> device_offline() and would fail on signals (not sure if relevant, like for virtio-mem it
> shouldn't be that relevant).
> 
> I guess dlpar_remove_lmb() can now simply call offline_and_remove_memory().
> [I might craft a patch later]
> 

This direction looks good to me, I'd love to see a patch if you
put something together.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v2 05/11] selftests/mm: .gitignore: add mkdirty, va_high_addr_switch

[John Hubbard]

On 6/5/23 08:53, Peter Xu wrote:
> On Fri, Jun 02, 2023 at 07:15:52PM -0700, John Hubbard wrote:
>> These new build products were left out of .gitignore, so add them now.
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>   tools/testing/selftests/mm/.gitignore | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
>> index 8917455f4f51..ab215303d8e9 100644
>> --- a/tools/testing/selftests/mm/.gitignore
>> +++ b/tools/testing/selftests/mm/.gitignore
>> @@ -39,3 +39,5 @@ local_config.h
>>   local_config.mk
>>   ksm_functional_tests
>>   mdwe_test
>> +mkdirty
>> +va_high_addr_switch
>> \ No newline at end of file
> 
> Maybe also the new gup_longterm?
> 

Yes I could add that. It might make slightly easier for Andrew because it
would preemptively avoid a minor merge conflict.

Unless David is thinking of adding it to another version of that series?

> Reviewed-by: Peter Xu <peterx@redhat.com>
> 

Thanks for this (and the other) reviews!

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v2 05/11] selftests/mm: .gitignore: add mkdirty, va_high_addr_switch

[Peter Xu]

On Fri, Jun 02, 2023 at 07:15:52PM -0700, John Hubbard wrote:
> These new build products were left out of .gitignore, so add them now.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/mm/.gitignore | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
> index 8917455f4f51..ab215303d8e9 100644
> --- a/tools/testing/selftests/mm/.gitignore
> +++ b/tools/testing/selftests/mm/.gitignore
> @@ -39,3 +39,5 @@ local_config.h
>  local_config.mk
>  ksm_functional_tests
>  mdwe_test
> +mkdirty
> +va_high_addr_switch
> \ No newline at end of file

Maybe also the new gup_longterm?

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v2 05/11] selftests/mm: .gitignore: add mkdirty, va_high_addr_switch

[John Hubbard]

These new build products were left out of .gitignore, so add them now.

Reviewed-by: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/.gitignore | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index 8917455f4f51..ab215303d8e9 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -39,3 +39,5 @@ local_config.h
 local_config.mk
 ksm_functional_tests
 mdwe_test
+mkdirty
+va_high_addr_switch
\ No newline at end of file
-- 
2.40.1