linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V4 1/2] selftests: vm: bring common functions to a new file
@ 2022-03-15  8:50 Muhammad Usama Anjum
  2022-03-15  8:50 ` [PATCH V4 2/2] selftests: vm: Add test for Soft-Dirty PTE bit Muhammad Usama Anjum
  2022-03-16  8:55 ` [PATCH V4 1/2] selftests: vm: bring common functions to a new file David Hildenbrand
  0 siblings, 2 replies; 6+ messages in thread
From: Muhammad Usama Anjum @ 2022-03-15  8:50 UTC (permalink / raw)
  To: Andrew Morton, Shuah Khan
  Cc: Muhammad Usama Anjum, kernel, linux-kernel, linux-mm, linux-kselftest

Bring common functions to a new file. These functions can be used in the
new tests. This helps in code duplication.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/vm/Makefile           |   7 +-
 tools/testing/selftests/vm/madv_populate.c    |  34 +-----
 .../selftests/vm/split_huge_page_test.c       |  77 +------------
 tools/testing/selftests/vm/vm_util.c          | 103 ++++++++++++++++++
 tools/testing/selftests/vm/vm_util.h          |  15 +++
 5 files changed, 125 insertions(+), 111 deletions(-)
 create mode 100644 tools/testing/selftests/vm/vm_util.c
 create mode 100644 tools/testing/selftests/vm/vm_util.h

diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
index 5e43f072f5b76..4e68edb26d6b6 100644
--- a/tools/testing/selftests/vm/Makefile
+++ b/tools/testing/selftests/vm/Makefile
@@ -34,7 +34,7 @@ TEST_GEN_FILES += hugepage-mremap
 TEST_GEN_FILES += hugepage-shm
 TEST_GEN_FILES += hugepage-vmemmap
 TEST_GEN_FILES += khugepaged
-TEST_GEN_FILES += madv_populate
+TEST_GEN_PROGS = madv_populate
 TEST_GEN_FILES += map_fixed_noreplace
 TEST_GEN_FILES += map_hugetlb
 TEST_GEN_FILES += map_populate
@@ -47,7 +47,7 @@ TEST_GEN_FILES += on-fault-limit
 TEST_GEN_FILES += thuge-gen
 TEST_GEN_FILES += transhuge-stress
 TEST_GEN_FILES += userfaultfd
-TEST_GEN_FILES += split_huge_page_test
+TEST_GEN_PROGS += split_huge_page_test
 TEST_GEN_FILES += ksm_tests
 
 ifeq ($(MACHINE),x86_64)
@@ -91,6 +91,9 @@ TEST_FILES := test_vmalloc.sh
 KSFT_KHDR_INSTALL := 1
 include ../lib.mk
 
+$(OUTPUT)/madv_populate: vm_util.c
+$(OUTPUT)/split_huge_page_test: vm_util.c
+
 ifeq ($(MACHINE),x86_64)
 BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32))
 BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64))
diff --git a/tools/testing/selftests/vm/madv_populate.c b/tools/testing/selftests/vm/madv_populate.c
index 3ee0e82756002..715a42e8e2cdb 100644
--- a/tools/testing/selftests/vm/madv_populate.c
+++ b/tools/testing/selftests/vm/madv_populate.c
@@ -18,6 +18,7 @@
 #include <sys/mman.h>
 
 #include "../kselftest.h"
+#include "vm_util.h"
 
 /*
  * For now, we're using 2 MiB of private anonymous memory for all tests.
@@ -26,18 +27,6 @@
 
 static size_t pagesize;
 
-static uint64_t pagemap_get_entry(int fd, char *start)
-{
-	const unsigned long pfn = (unsigned long)start / pagesize;
-	uint64_t entry;
-	int ret;
-
-	ret = pread(fd, &entry, sizeof(entry), pfn * sizeof(entry));
-	if (ret != sizeof(entry))
-		ksft_exit_fail_msg("reading pagemap failed\n");
-	return entry;
-}
-
 static bool pagemap_is_populated(int fd, char *start)
 {
 	uint64_t entry = pagemap_get_entry(fd, start);
@@ -46,13 +35,6 @@ static bool pagemap_is_populated(int fd, char *start)
 	return entry & 0xc000000000000000ull;
 }
 
-static bool pagemap_is_softdirty(int fd, char *start)
-{
-	uint64_t entry = pagemap_get_entry(fd, start);
-
-	return entry & 0x0080000000000000ull;
-}
-
 static void sense_support(void)
 {
 	char *addr;
@@ -258,20 +240,6 @@ static bool range_is_not_softdirty(char *start, ssize_t size)
 	return ret;
 }
 
-static void clear_softdirty(void)
-{
-	int fd = open("/proc/self/clear_refs", O_WRONLY);
-	const char *ctrl = "4";
-	int ret;
-
-	if (fd < 0)
-		ksft_exit_fail_msg("opening clear_refs failed\n");
-	ret = write(fd, ctrl, strlen(ctrl));
-	if (ret != strlen(ctrl))
-		ksft_exit_fail_msg("writing clear_refs failed\n");
-	close(fd);
-}
-
 static void test_softdirty(void)
 {
 	char *addr;
diff --git a/tools/testing/selftests/vm/split_huge_page_test.c b/tools/testing/selftests/vm/split_huge_page_test.c
index 52497b7b9f1db..b6b381611fb6d 100644
--- a/tools/testing/selftests/vm/split_huge_page_test.c
+++ b/tools/testing/selftests/vm/split_huge_page_test.c
@@ -16,6 +16,7 @@
 #include <sys/mount.h>
 #include <malloc.h>
 #include <stdbool.h>
+#include "vm_util.h"
 
 uint64_t pagesize;
 unsigned int pageshift;
@@ -51,30 +52,6 @@ int is_backed_by_thp(char *vaddr, int pagemap_file, int kpageflags_file)
 	return 0;
 }
 
-
-static uint64_t read_pmd_pagesize(void)
-{
-	int fd;
-	char buf[20];
-	ssize_t num_read;
-
-	fd = open(PMD_SIZE_PATH, O_RDONLY);
-	if (fd == -1) {
-		perror("Open hpage_pmd_size failed");
-		exit(EXIT_FAILURE);
-	}
-	num_read = read(fd, buf, 19);
-	if (num_read < 1) {
-		close(fd);
-		perror("Read hpage_pmd_size failed");
-		exit(EXIT_FAILURE);
-	}
-	buf[num_read] = '\0';
-	close(fd);
-
-	return strtoul(buf, NULL, 10);
-}
-
 static int write_file(const char *path, const char *buf, size_t buflen)
 {
 	int fd;
@@ -113,58 +90,6 @@ static void write_debugfs(const char *fmt, ...)
 	}
 }
 
-#define MAX_LINE_LENGTH 500
-
-static bool check_for_pattern(FILE *fp, const char *pattern, char *buf)
-{
-	while (fgets(buf, MAX_LINE_LENGTH, fp) != NULL) {
-		if (!strncmp(buf, pattern, strlen(pattern)))
-			return true;
-	}
-	return false;
-}
-
-static uint64_t check_huge(void *addr)
-{
-	uint64_t thp = 0;
-	int ret;
-	FILE *fp;
-	char buffer[MAX_LINE_LENGTH];
-	char addr_pattern[MAX_LINE_LENGTH];
-
-	ret = snprintf(addr_pattern, MAX_LINE_LENGTH, "%08lx-",
-		       (unsigned long) addr);
-	if (ret >= MAX_LINE_LENGTH) {
-		printf("%s: Pattern is too long\n", __func__);
-		exit(EXIT_FAILURE);
-	}
-
-
-	fp = fopen(SMAP_PATH, "r");
-	if (!fp) {
-		printf("%s: Failed to open file %s\n", __func__, SMAP_PATH);
-		exit(EXIT_FAILURE);
-	}
-	if (!check_for_pattern(fp, addr_pattern, buffer))
-		goto err_out;
-
-	/*
-	 * Fetch the AnonHugePages: in the same block and check the number of
-	 * hugepages.
-	 */
-	if (!check_for_pattern(fp, "AnonHugePages:", buffer))
-		goto err_out;
-
-	if (sscanf(buffer, "AnonHugePages:%10ld kB", &thp) != 1) {
-		printf("Reading smap error\n");
-		exit(EXIT_FAILURE);
-	}
-
-err_out:
-	fclose(fp);
-	return thp;
-}
-
 void split_pmd_thp(void)
 {
 	char *one_page;
diff --git a/tools/testing/selftests/vm/vm_util.c b/tools/testing/selftests/vm/vm_util.c
new file mode 100644
index 0000000000000..c946e04df9236
--- /dev/null
+++ b/tools/testing/selftests/vm/vm_util.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdbool.h>
+#include <string.h>
+#include "vm_util.h"
+
+uint64_t pagemap_get_entry(int fd, char *start)
+{
+	const unsigned long pfn = (unsigned long)start / getpagesize();
+	uint64_t entry;
+	int ret;
+
+	ret = pread(fd, &entry, sizeof(entry), pfn * sizeof(entry));
+	if (ret != sizeof(entry))
+		ksft_exit_fail_msg("reading pagemap failed\n");
+	return entry;
+}
+
+bool pagemap_is_softdirty(int fd, char *start)
+{
+	uint64_t entry = pagemap_get_entry(fd, start);
+
+	return ((entry >> DIRTY_BIT_LOCATION) & 1);
+}
+
+void clear_softdirty(void)
+{
+	int ret;
+	const char *ctrl = "4";
+	int fd = open("/proc/self/clear_refs", O_WRONLY);
+
+	if (fd < 0)
+		ksft_exit_fail_msg("opening clear_refs failed\n");
+	ret = write(fd, ctrl, strlen(ctrl));
+	close(fd);
+	if (ret != strlen(ctrl))
+		ksft_exit_fail_msg("writing clear_refs failed\n");
+}
+
+
+static bool check_for_pattern(FILE *fp, const char *pattern, char *buf)
+{
+	while (fgets(buf, MAX_LINE_LENGTH, fp) != NULL) {
+		if (!strncmp(buf, pattern, strlen(pattern)))
+			return true;
+	}
+	return false;
+}
+
+uint64_t read_pmd_pagesize(void)
+{
+	int fd;
+	char buf[20];
+	ssize_t num_read;
+
+	fd = open(PMD_SIZE, O_RDONLY);
+	if (fd == -1)
+		ksft_exit_fail_msg("Open hpage_pmd_size failed\n");
+
+	num_read = read(fd, buf, 19);
+	if (num_read < 1) {
+		close(fd);
+		ksft_exit_fail_msg("Read hpage_pmd_size failed\n");
+	}
+	buf[num_read] = '\0';
+	close(fd);
+
+	return strtoul(buf, NULL, 10);
+}
+
+uint64_t check_huge(void *addr)
+{
+	uint64_t thp = 0;
+	int ret;
+	FILE *fp;
+	char buffer[MAX_LINE_LENGTH];
+	char addr_pattern[MAX_LINE_LENGTH];
+
+	ret = snprintf(addr_pattern, MAX_LINE_LENGTH, "%08lx-",
+		       (unsigned long) addr);
+	if (ret >= MAX_LINE_LENGTH)
+		ksft_exit_fail_msg("%s: Pattern is too long\n", __func__);
+
+	fp = fopen(SMAP, "r");
+	if (!fp)
+		ksft_exit_fail_msg("%s: Failed to open file %s\n", __func__, SMAP);
+
+	if (!check_for_pattern(fp, addr_pattern, buffer))
+		goto err_out;
+
+	/*
+	 * Fetch the AnonHugePages: in the same block and check the number of
+	 * hugepages.
+	 */
+	if (!check_for_pattern(fp, "AnonHugePages:", buffer))
+		goto err_out;
+
+	if (sscanf(buffer, "AnonHugePages:%10ld kB", &thp) != 1)
+		ksft_exit_fail_msg("Reading smap error\n");
+
+err_out:
+	fclose(fp);
+	return thp;
+}
diff --git a/tools/testing/selftests/vm/vm_util.h b/tools/testing/selftests/vm/vm_util.h
new file mode 100644
index 0000000000000..7522dbb859f0f
--- /dev/null
+++ b/tools/testing/selftests/vm/vm_util.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <stdint.h>
+#include <fcntl.h>
+#include "../kselftest.h"
+
+#define	PMD_SIZE		"/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"
+#define	SMAP			"/proc/self/smaps"
+#define	DIRTY_BIT_LOCATION	55
+#define	MAX_LINE_LENGTH		512
+
+uint64_t pagemap_get_entry(int fd, char *start);
+bool pagemap_is_softdirty(int fd, char *start);
+void clear_softdirty(void);
+uint64_t read_pmd_pagesize(void);
+uint64_t check_huge(void *addr);
-- 
2.30.2


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

* [PATCH V4 2/2] selftests: vm: Add test for Soft-Dirty PTE bit
  2022-03-15  8:50 [PATCH V4 1/2] selftests: vm: bring common functions to a new file Muhammad Usama Anjum
@ 2022-03-15  8:50 ` Muhammad Usama Anjum
  2022-03-15 20:53   ` Gabriel Krisman Bertazi
  2022-03-16  8:55 ` [PATCH V4 1/2] selftests: vm: bring common functions to a new file David Hildenbrand
  1 sibling, 1 reply; 6+ messages in thread
From: Muhammad Usama Anjum @ 2022-03-15  8:50 UTC (permalink / raw)
  To: Andrew Morton, Shuah Khan
  Cc: Gabriel Krisman Bertazi, usama.anjum, kernel, Will Deacon,
	linux-kernel, linux-mm, linux-kselftest

From: Gabriel Krisman Bertazi <krisman@collabora.com>

This introduces three tests:
1) Sanity check soft dirty basic semantics: allocate area, clean, dirty,
check if the SD bit is flipped.
2) Check VMA reuse: validate the VM_SOFTDIRTY usage
3) Check soft-dirty on huge pages

This was motivated by Will Deacon's fix commit 912efa17e512 ("mm: proc:
Invalidate TLB after clearing soft-dirty page state"). I was tracking the
same issue that he fixed, and this test would have caught it.

CC: Will Deacon <will@kernel.org>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
V3 of this patch is in Andrew's tree. Please drop that.

Changes in V4:
Cosmetic changes
Removed global variables
Replaced ksft_print_msg with ksft_exit_fail_msg to exit the program at
once
Some other minor changes
Correct the authorship of the patch

Tests of soft dirty bit in this patch and in madv_populate.c are
non-overlapping. madv_populate.c has only one soft-dirty bit test in the
context of different advise (MADV_POPULATE_READ and
MADV_POPULATE_WRITE). This new test adds more tests.

Tab width of 8 has been used to align the macros. This alignment may look
odd in shell or email. But it looks alright in editors.

Test output:
TAP version 13
1..5
ok 1 Test test_simple
ok 2 Test test_vma_reuse reused memory location
ok 3 Test test_vma_reuse dirty bit of previous page
ok 4 Test test_hugepage huge page allocation
ok 5 Test test_hugepage huge page dirty bit
 # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0

Or

TAP version 13
1..5
ok 1 Test test_simple
ok 2 Test test_vma_reuse reused memory location
ok 3 Test test_vma_reuse dirty bit of previous page
ok 4 # SKIP Test test_hugepage huge page allocation
ok 5 # SKIP Test test_hugepage huge page dirty bit
 # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:2 error:0

Changes in V3:
Move test to selftests/vm
Use kselftest macros
Minor updates to make code more maintainable
Add configurations in config file

V2 of this patch:
https://lore.kernel.org/lkml/20210603151518.2437813-1-krisman@collabora.com/
---
 tools/testing/selftests/vm/.gitignore   |   1 +
 tools/testing/selftests/vm/Makefile     |   2 +
 tools/testing/selftests/vm/config       |   2 +
 tools/testing/selftests/vm/soft-dirty.c | 146 ++++++++++++++++++++++++
 4 files changed, 151 insertions(+)
 create mode 100644 tools/testing/selftests/vm/soft-dirty.c

diff --git a/tools/testing/selftests/vm/.gitignore b/tools/testing/selftests/vm/.gitignore
index d7507f3c7c76a..3cb4fa771ec2a 100644
--- a/tools/testing/selftests/vm/.gitignore
+++ b/tools/testing/selftests/vm/.gitignore
@@ -29,5 +29,6 @@ write_to_hugetlbfs
 hmm-tests
 memfd_secret
 local_config.*
+soft-dirty
 split_huge_page_test
 ksm_tests
diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
index 4e68edb26d6b6..f25eb30b5f0cb 100644
--- a/tools/testing/selftests/vm/Makefile
+++ b/tools/testing/selftests/vm/Makefile
@@ -47,6 +47,7 @@ TEST_GEN_FILES += on-fault-limit
 TEST_GEN_FILES += thuge-gen
 TEST_GEN_FILES += transhuge-stress
 TEST_GEN_FILES += userfaultfd
+TEST_GEN_PROGS += soft-dirty
 TEST_GEN_PROGS += split_huge_page_test
 TEST_GEN_FILES += ksm_tests
 
@@ -92,6 +93,7 @@ KSFT_KHDR_INSTALL := 1
 include ../lib.mk
 
 $(OUTPUT)/madv_populate: vm_util.c
+$(OUTPUT)/soft-dirty: vm_util.c
 $(OUTPUT)/split_huge_page_test: vm_util.c
 
 ifeq ($(MACHINE),x86_64)
diff --git a/tools/testing/selftests/vm/config b/tools/testing/selftests/vm/config
index 60e82da0de850..be087c4bc3961 100644
--- a/tools/testing/selftests/vm/config
+++ b/tools/testing/selftests/vm/config
@@ -4,3 +4,5 @@ CONFIG_TEST_VMALLOC=m
 CONFIG_DEVICE_PRIVATE=y
 CONFIG_TEST_HMM=m
 CONFIG_GUP_TEST=y
+CONFIG_TRANSPARENT_HUGEPAGE=y
+CONFIG_MEM_SOFT_DIRTY=y
diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
new file mode 100644
index 0000000000000..2d50ed3472206
--- /dev/null
+++ b/tools/testing/selftests/vm/soft-dirty.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <string.h>
+#include <stdbool.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include <malloc.h>
+#include <sys/mman.h>
+#include "../kselftest.h"
+#include "vm_util.h"
+
+#define PAGEMAP			"/proc/self/pagemap"
+#define CLEAR_REFS		"/proc/self/clear_refs"
+#define MAX_LINE_LENGTH		512
+#define TEST_ITERATIONS		10000
+
+static void test_simple(int pagemap_fd, int pagesize)
+{
+	int i;
+	char *map;
+
+	map = aligned_alloc(pagesize, pagesize);
+	if (!map)
+		ksft_exit_fail_msg("mmap failed\n");
+
+	clear_softdirty();
+
+	for (i = 0 ; i < TEST_ITERATIONS; i++) {
+		if (pagemap_is_softdirty(pagemap_fd, map) == 1) {
+			ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
+			break;
+		}
+
+		clear_softdirty();
+		map[0]++;
+
+		if (pagemap_is_softdirty(pagemap_fd, map) == 0) {
+			ksft_print_msg("dirty bit was 0, but should be 1 (i=%d)\n", i);
+			break;
+		}
+
+		clear_softdirty();
+	}
+	free(map);
+
+	ksft_test_result(i == TEST_ITERATIONS, "Test %s\n", __func__);
+}
+
+static void test_vma_reuse(int pagemap_fd, int pagesize)
+{
+	char *map, *map2;
+
+	map = mmap(NULL, pagesize, (PROT_READ | PROT_WRITE), (MAP_PRIVATE | MAP_ANON), -1, 0);
+	if (map == MAP_FAILED)
+		ksft_exit_fail_msg("mmap failed");
+
+	clear_softdirty();
+
+	/* Write to the page before unmapping and map the same size region again to check
+	 * if same memory region is gotten next time and if dirty bit is preserved across
+	 * this type of allocations.
+	 */
+	map[0]++;
+
+	munmap(map, pagesize);
+
+	map2 = mmap(NULL, pagesize, (PROT_READ | PROT_WRITE), (MAP_PRIVATE | MAP_ANON), -1, 0);
+	if (map2 == MAP_FAILED)
+		ksft_exit_fail_msg("mmap failed");
+
+	ksft_test_result(map == map2, "Test %s reused memory location\n", __func__);
+
+	ksft_test_result(pagemap_is_softdirty(pagemap_fd, map) != 0,
+			 "Test %s dirty bit of previous page\n", __func__);
+
+	munmap(map2, pagesize);
+}
+
+static void test_hugepage(int pagemap_fd, int pagesize)
+{
+	char *map;
+	int i, ret;
+	size_t hpage_len = read_pmd_pagesize();
+
+	map = memalign(hpage_len, hpage_len);
+	if (!map)
+		ksft_exit_fail_msg("memalign failed\n");
+
+	ret = madvise(map, hpage_len, MADV_HUGEPAGE);
+	if (ret)
+		ksft_exit_fail_msg("madvise failed %d\n", ret);
+
+	for (i = 0; i < hpage_len; i++)
+		map[i] = (char)i;
+
+	if (check_huge(map)) {
+		ksft_test_result_pass("Test %s huge page allocation\n", __func__);
+
+		clear_softdirty();
+		for (i = 0 ; i < TEST_ITERATIONS ; i++) {
+			if (pagemap_is_softdirty(pagemap_fd, map) == 1) {
+				ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
+				break;
+			}
+
+			clear_softdirty();
+			map[0]++;
+
+			if (pagemap_is_softdirty(pagemap_fd, map) == 0) {
+				ksft_print_msg("dirty bit was 0, but should be 1 (i=%d)\n", i);
+				break;
+			}
+			clear_softdirty();
+		}
+
+		ksft_test_result(i == TEST_ITERATIONS, "Test %s huge page dirty bit\n", __func__);
+	} else {
+		// hugepage allocation failed. skip these tests
+		ksft_test_result_skip("Test %s huge page allocation\n", __func__);
+		ksft_test_result_skip("Test %s huge page dirty bit\n", __func__);
+	}
+	free(map);
+}
+
+int main(int argc, char **argv)
+{
+	int pagemap_fd;
+	int pagesize;
+
+	ksft_print_header();
+	ksft_set_plan(5);
+
+	pagemap_fd = open(PAGEMAP, O_RDONLY);
+	if (pagemap_fd < 0)
+		ksft_exit_fail_msg("Failed to open %s\n", PAGEMAP);
+
+	pagesize = getpagesize();
+
+	test_simple(pagemap_fd, pagesize);
+	test_vma_reuse(pagemap_fd, pagesize);
+	test_hugepage(pagemap_fd, pagesize);
+
+	close(pagemap_fd);
+
+	return ksft_exit_pass();
+}
-- 
2.30.2


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

* Re: [PATCH V4 2/2] selftests: vm: Add test for Soft-Dirty PTE bit
  2022-03-15  8:50 ` [PATCH V4 2/2] selftests: vm: Add test for Soft-Dirty PTE bit Muhammad Usama Anjum
@ 2022-03-15 20:53   ` Gabriel Krisman Bertazi
  2022-03-16 17:36     ` Muhammad Usama Anjum
  0 siblings, 1 reply; 6+ messages in thread
From: Gabriel Krisman Bertazi @ 2022-03-15 20:53 UTC (permalink / raw)
  To: Muhammad Usama Anjum
  Cc: Andrew Morton, Shuah Khan, kernel, Will Deacon, linux-kernel,
	linux-mm, linux-kselftest

Muhammad Usama Anjum <usama.anjum@collabora.com> writes:

> From: Gabriel Krisman Bertazi <krisman@collabora.com>

Hi Usama,

Please, cc me on the whole thread.  I didn't get the patch 1/2 or the
cover letter.

> This introduces three tests:
> 1) Sanity check soft dirty basic semantics: allocate area, clean, dirty,
> check if the SD bit is flipped.
> 2) Check VMA reuse: validate the VM_SOFTDIRTY usage
> 3) Check soft-dirty on huge pages
>
> This was motivated by Will Deacon's fix commit 912efa17e512 ("mm: proc:
> Invalidate TLB after clearing soft-dirty page state"). I was tracking the
> same issue that he fixed, and this test would have caught it.
>
> CC: Will Deacon <will@kernel.org>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
> V3 of this patch is in Andrew's tree. Please drop that.

v3 is still in linux-next and this note is quite hidden in the middle of
the commit message.

>
> Changes in V4:
> Cosmetic changes
> Removed global variables
> Replaced ksft_print_msg with ksft_exit_fail_msg to exit the program at
> once
> Some other minor changes
> Correct the authorship of the patch
>
> Tests of soft dirty bit in this patch and in madv_populate.c are
> non-overlapping. madv_populate.c has only one soft-dirty bit test in the
> context of different advise (MADV_POPULATE_READ and
> MADV_POPULATE_WRITE). This new test adds more tests.
>
> Tab width of 8 has been used to align the macros. This alignment may look
> odd in shell or email. But it looks alright in editors.

I'm curious if you tested reverting 912efa17e512. Did the new versions
of this patch still catch the original issue?

> Test output:
> TAP version 13
> 1..5
> ok 1 Test test_simple
> ok 2 Test test_vma_reuse reused memory location
> ok 3 Test test_vma_reuse dirty bit of previous page
> ok 4 Test test_hugepage huge page allocation
> ok 5 Test test_hugepage huge page dirty bit
>  # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Or
>
> TAP version 13
> 1..5
> ok 1 Test test_simple
> ok 2 Test test_vma_reuse reused memory location
> ok 3 Test test_vma_reuse dirty bit of previous page
> ok 4 # SKIP Test test_hugepage huge page allocation
> ok 5 # SKIP Test test_hugepage huge page dirty bit
>  # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:2 error:0
>
> Changes in V3:
> Move test to selftests/vm
> Use kselftest macros
> Minor updates to make code more maintainable
> Add configurations in config file
>
> V2 of this patch:
> https://lore.kernel.org/lkml/20210603151518.2437813-1-krisman@collabora.com/
> ---
>  tools/testing/selftests/vm/.gitignore   |   1 +
>  tools/testing/selftests/vm/Makefile     |   2 +
>  tools/testing/selftests/vm/config       |   2 +
>  tools/testing/selftests/vm/soft-dirty.c | 146 ++++++++++++++++++++++++
>  4 files changed, 151 insertions(+)
>  create mode 100644 tools/testing/selftests/vm/soft-dirty.c
>
> diff --git a/tools/testing/selftests/vm/.gitignore b/tools/testing/selftests/vm/.gitignore
> index d7507f3c7c76a..3cb4fa771ec2a 100644
> --- a/tools/testing/selftests/vm/.gitignore
> +++ b/tools/testing/selftests/vm/.gitignore
> @@ -29,5 +29,6 @@ write_to_hugetlbfs
>  hmm-tests
>  memfd_secret
>  local_config.*
> +soft-dirty
>  split_huge_page_test
>  ksm_tests
> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> index 4e68edb26d6b6..f25eb30b5f0cb 100644
> --- a/tools/testing/selftests/vm/Makefile
> +++ b/tools/testing/selftests/vm/Makefile
> @@ -47,6 +47,7 @@ TEST_GEN_FILES += on-fault-limit
>  TEST_GEN_FILES += thuge-gen
>  TEST_GEN_FILES += transhuge-stress
>  TEST_GEN_FILES += userfaultfd
> +TEST_GEN_PROGS += soft-dirty
>  TEST_GEN_PROGS += split_huge_page_test
>  TEST_GEN_FILES += ksm_tests
>  
> @@ -92,6 +93,7 @@ KSFT_KHDR_INSTALL := 1
>  include ../lib.mk
>  
>  $(OUTPUT)/madv_populate: vm_util.c
> +$(OUTPUT)/soft-dirty: vm_util.c
>  $(OUTPUT)/split_huge_page_test: vm_util.c
>  
>  ifeq ($(MACHINE),x86_64)
> diff --git a/tools/testing/selftests/vm/config b/tools/testing/selftests/vm/config
> index 60e82da0de850..be087c4bc3961 100644
> --- a/tools/testing/selftests/vm/config
> +++ b/tools/testing/selftests/vm/config
> @@ -4,3 +4,5 @@ CONFIG_TEST_VMALLOC=m
>  CONFIG_DEVICE_PRIVATE=y
>  CONFIG_TEST_HMM=m
>  CONFIG_GUP_TEST=y
> +CONFIG_TRANSPARENT_HUGEPAGE=y
> +CONFIG_MEM_SOFT_DIRTY=y
> diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
> new file mode 100644
> index 0000000000000..2d50ed3472206
> --- /dev/null
> +++ b/tools/testing/selftests/vm/soft-dirty.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdbool.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <malloc.h>
> +#include <sys/mman.h>
> +#include "../kselftest.h"
> +#include "vm_util.h"
> +
> +#define PAGEMAP			"/proc/self/pagemap"
> +#define CLEAR_REFS		"/proc/self/clear_refs"
> +#define MAX_LINE_LENGTH		512

MAX_LINE_LENGTH is no longer used after check_for_pattern was dropped.

Can't the previous defines and file handling functions also go the
vm_util.h?

> +#define TEST_ITERATIONS		10000
> +
> +static void test_simple(int pagemap_fd, int pagesize)
> +{
> +	int i;
> +	char *map;
> +
> +	map = aligned_alloc(pagesize, pagesize);
> +	if (!map)
> +		ksft_exit_fail_msg("mmap failed\n");
> +
> +	clear_softdirty();
> +
> +	for (i = 0 ; i < TEST_ITERATIONS; i++) {
> +		if (pagemap_is_softdirty(pagemap_fd, map) == 1) {
> +			ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
> +			break;
> +		}
> +
> +		clear_softdirty();
> +		map[0]++;


This will overflow several times during TEST_ITERATIONS.  While it is
not broken, since we care about causing the page fault, it is not
obvious.  Can you add a comment or do something like this instead?

  map[0] = !map[0];

-- 
Gabriel Krisman Bertazi

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

* Re: [PATCH V4 1/2] selftests: vm: bring common functions to a new file
  2022-03-15  8:50 [PATCH V4 1/2] selftests: vm: bring common functions to a new file Muhammad Usama Anjum
  2022-03-15  8:50 ` [PATCH V4 2/2] selftests: vm: Add test for Soft-Dirty PTE bit Muhammad Usama Anjum
@ 2022-03-16  8:55 ` David Hildenbrand
  2022-03-16 16:47   ` Muhammad Usama Anjum
  1 sibling, 1 reply; 6+ messages in thread
From: David Hildenbrand @ 2022-03-16  8:55 UTC (permalink / raw)
  To: Muhammad Usama Anjum, Andrew Morton, Shuah Khan
  Cc: kernel, linux-kernel, linux-mm, linux-kselftest

On 15.03.22 09:50, Muhammad Usama Anjum wrote:
> Bring common functions to a new file. These functions can be used in the
> new tests. This helps in code duplication.
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
>  tools/testing/selftests/vm/Makefile           |   7 +-
>  tools/testing/selftests/vm/madv_populate.c    |  34 +-----
>  .../selftests/vm/split_huge_page_test.c       |  77 +------------
>  tools/testing/selftests/vm/vm_util.c          | 103 ++++++++++++++++++
>  tools/testing/selftests/vm/vm_util.h          |  15 +++
>  5 files changed, 125 insertions(+), 111 deletions(-)
>  create mode 100644 tools/testing/selftests/vm/vm_util.c
>  create mode 100644 tools/testing/selftests/vm/vm_util.h
> 
> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> index 5e43f072f5b76..4e68edb26d6b6 100644
> --- a/tools/testing/selftests/vm/Makefile
> +++ b/tools/testing/selftests/vm/Makefile
> @@ -34,7 +34,7 @@ TEST_GEN_FILES += hugepage-mremap
>  TEST_GEN_FILES += hugepage-shm
>  TEST_GEN_FILES += hugepage-vmemmap
>  TEST_GEN_FILES += khugepaged
> -TEST_GEN_FILES += madv_populate
> +TEST_GEN_PROGS = madv_populate
>  TEST_GEN_FILES += map_fixed_noreplace
>  TEST_GEN_FILES += map_hugetlb
>  TEST_GEN_FILES += map_populate
> @@ -47,7 +47,7 @@ TEST_GEN_FILES += on-fault-limit
>  TEST_GEN_FILES += thuge-gen
>  TEST_GEN_FILES += transhuge-stress
>  TEST_GEN_FILES += userfaultfd
> -TEST_GEN_FILES += split_huge_page_test
> +TEST_GEN_PROGS += split_huge_page_test
>  TEST_GEN_FILES += ksm_tests
>  
>  ifeq ($(MACHINE),x86_64)
> @@ -91,6 +91,9 @@ TEST_FILES := test_vmalloc.sh
>  KSFT_KHDR_INSTALL := 1
>  include ../lib.mk
>  
> +$(OUTPUT)/madv_populate: vm_util.c
> +$(OUTPUT)/split_huge_page_test: vm_util.c
> +


[...]

> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdbool.h>
> +#include <string.h>
> +#include "vm_util.h"
> +
> +uint64_t pagemap_get_entry(int fd, char *start)
> +{
> +	const unsigned long pfn = (unsigned long)start / getpagesize();
> +	uint64_t entry;
> +	int ret;
> +
> +	ret = pread(fd, &entry, sizeof(entry), pfn * sizeof(entry));
> +	if (ret != sizeof(entry))
> +		ksft_exit_fail_msg("reading pagemap failed\n");
> +	return entry;
> +}
> +
> +bool pagemap_is_softdirty(int fd, char *start)
> +{
> +	uint64_t entry = pagemap_get_entry(fd, start);
> +
> +	return ((entry >> DIRTY_BIT_LOCATION) & 1);
> +}

Please leave code you're moving around as untouched as possible to avoid
unrelated bugs that happen by mistake and are hard to review.

> +
> +void clear_softdirty(void)
> +{
> +	int ret;
> +	const char *ctrl = "4";
> +	int fd = open("/proc/self/clear_refs", O_WRONLY);
> +
> +	if (fd < 0)
> +		ksft_exit_fail_msg("opening clear_refs failed\n");
> +	ret = write(fd, ctrl, strlen(ctrl));
> +	close(fd);
> +	if (ret != strlen(ctrl))
> +		ksft_exit_fail_msg("writing clear_refs failed\n");
> +}
> +
> +
> +static bool check_for_pattern(FILE *fp, const char *pattern, char *buf)
> +{
> +	while (fgets(buf, MAX_LINE_LENGTH, fp) != NULL) {
> +		if (!strncmp(buf, pattern, strlen(pattern)))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +uint64_t read_pmd_pagesize(void)
> +{
> +	int fd;
> +	char buf[20];
> +	ssize_t num_read;
> +
> +	fd = open(PMD_SIZE, O_RDONLY);
> +	if (fd == -1)
> +		ksft_exit_fail_msg("Open hpage_pmd_size failed\n");
> +
> +	num_read = read(fd, buf, 19);
> +	if (num_read < 1) {
> +		close(fd);
> +		ksft_exit_fail_msg("Read hpage_pmd_size failed\n");
> +	}
> +	buf[num_read] = '\0';
> +	close(fd);
> +
> +	return strtoul(buf, NULL, 10);
> +}
> +
> +uint64_t check_huge(void *addr)
> +{
> +	uint64_t thp = 0;
> +	int ret;
> +	FILE *fp;
> +	char buffer[MAX_LINE_LENGTH];
> +	char addr_pattern[MAX_LINE_LENGTH];
> +
> +	ret = snprintf(addr_pattern, MAX_LINE_LENGTH, "%08lx-",
> +		       (unsigned long) addr);
> +	if (ret >= MAX_LINE_LENGTH)
> +		ksft_exit_fail_msg("%s: Pattern is too long\n", __func__);
> +
> +	fp = fopen(SMAP, "r");
> +	if (!fp)
> +		ksft_exit_fail_msg("%s: Failed to open file %s\n", __func__, SMAP);
> +
> +	if (!check_for_pattern(fp, addr_pattern, buffer))
> +		goto err_out;
> +
> +	/*
> +	 * Fetch the AnonHugePages: in the same block and check the number of
> +	 * hugepages.
> +	 */
> +	if (!check_for_pattern(fp, "AnonHugePages:", buffer))
> +		goto err_out;
> +
> +	if (sscanf(buffer, "AnonHugePages:%10ld kB", &thp) != 1)
> +		ksft_exit_fail_msg("Reading smap error\n");
> +
> +err_out:
> +	fclose(fp);
> +	return thp;
> +}
> diff --git a/tools/testing/selftests/vm/vm_util.h b/tools/testing/selftests/vm/vm_util.h
> new file mode 100644
> index 0000000000000..7522dbb859f0f
> --- /dev/null
> +++ b/tools/testing/selftests/vm/vm_util.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <stdint.h>
> +#include <fcntl.h>
> +#include "../kselftest.h"
> +
> +#define	PMD_SIZE		"/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"

Ehm no. PMD_SIZE_PATH at best -- just as it used to.

> +#define	SMAP			"/proc/self/smaps"

SMAPS_PATH

> +#define	DIRTY_BIT_LOCATION	55

Please inline that just as it used to. There is no value in a magic
define without any proper namespace.

> +#define	MAX_LINE_LENGTH		512

This used to be 500. Why the change?


Also: weird indentation and these all look like the should go into
vm_util.c. They are not used outside that file.



-- 
Thanks,

David / dhildenb


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

* Re: [PATCH V4 1/2] selftests: vm: bring common functions to a new file
  2022-03-16  8:55 ` [PATCH V4 1/2] selftests: vm: bring common functions to a new file David Hildenbrand
@ 2022-03-16 16:47   ` Muhammad Usama Anjum
  0 siblings, 0 replies; 6+ messages in thread
From: Muhammad Usama Anjum @ 2022-03-16 16:47 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: usama.anjum, kernel, linux-kernel, linux-mm, linux-kselftest,
	Andrew Morton, Gabriel Krisman Bertazi, Shuah Khan

Hi David,

Thank you for the review. I'll correct everything mentioned here in the
next iteration.

+[Gabriel]

On 3/16/22 1:55 PM, David Hildenbrand wrote:
> On 15.03.22 09:50, Muhammad Usama Anjum wrote:
>> Bring common functions to a new file. These functions can be used in the
>> new tests. This helps in code duplication.
>>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>>  tools/testing/selftests/vm/Makefile           |   7 +-
>>  tools/testing/selftests/vm/madv_populate.c    |  34 +-----
>>  .../selftests/vm/split_huge_page_test.c       |  77 +------------
>>  tools/testing/selftests/vm/vm_util.c          | 103 ++++++++++++++++++
>>  tools/testing/selftests/vm/vm_util.h          |  15 +++
>>  5 files changed, 125 insertions(+), 111 deletions(-)
>>  create mode 100644 tools/testing/selftests/vm/vm_util.c
>>  create mode 100644 tools/testing/selftests/vm/vm_util.h
>>
>> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
>> index 5e43f072f5b76..4e68edb26d6b6 100644
>> --- a/tools/testing/selftests/vm/Makefile
>> +++ b/tools/testing/selftests/vm/Makefile
>> @@ -34,7 +34,7 @@ TEST_GEN_FILES += hugepage-mremap
>>  TEST_GEN_FILES += hugepage-shm
>>  TEST_GEN_FILES += hugepage-vmemmap
>>  TEST_GEN_FILES += khugepaged
>> -TEST_GEN_FILES += madv_populate
>> +TEST_GEN_PROGS = madv_populate
>>  TEST_GEN_FILES += map_fixed_noreplace
>>  TEST_GEN_FILES += map_hugetlb
>>  TEST_GEN_FILES += map_populate
>> @@ -47,7 +47,7 @@ TEST_GEN_FILES += on-fault-limit
>>  TEST_GEN_FILES += thuge-gen
>>  TEST_GEN_FILES += transhuge-stress
>>  TEST_GEN_FILES += userfaultfd
>> -TEST_GEN_FILES += split_huge_page_test
>> +TEST_GEN_PROGS += split_huge_page_test
>>  TEST_GEN_FILES += ksm_tests
>>  
>>  ifeq ($(MACHINE),x86_64)
>> @@ -91,6 +91,9 @@ TEST_FILES := test_vmalloc.sh
>>  KSFT_KHDR_INSTALL := 1
>>  include ../lib.mk
>>  
>> +$(OUTPUT)/madv_populate: vm_util.c
>> +$(OUTPUT)/split_huge_page_test: vm_util.c
>> +
> 
> 
> [...]
> 
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include <stdbool.h>
>> +#include <string.h>
>> +#include "vm_util.h"
>> +
>> +uint64_t pagemap_get_entry(int fd, char *start)
>> +{
>> +	const unsigned long pfn = (unsigned long)start / getpagesize();
>> +	uint64_t entry;
>> +	int ret;
>> +
>> +	ret = pread(fd, &entry, sizeof(entry), pfn * sizeof(entry));
>> +	if (ret != sizeof(entry))
>> +		ksft_exit_fail_msg("reading pagemap failed\n");
>> +	return entry;
>> +}
>> +
>> +bool pagemap_is_softdirty(int fd, char *start)
>> +{
>> +	uint64_t entry = pagemap_get_entry(fd, start);
>> +
>> +	return ((entry >> DIRTY_BIT_LOCATION) & 1);
>> +}
> 
> Please leave code you're moving around as untouched as possible to avoid
> unrelated bugs that happen by mistake and are hard to review.
> 
>> +
>> +void clear_softdirty(void)
>> +{
>> +	int ret;
>> +	const char *ctrl = "4";
>> +	int fd = open("/proc/self/clear_refs", O_WRONLY);
>> +
>> +	if (fd < 0)
>> +		ksft_exit_fail_msg("opening clear_refs failed\n");
>> +	ret = write(fd, ctrl, strlen(ctrl));
>> +	close(fd);
>> +	if (ret != strlen(ctrl))
>> +		ksft_exit_fail_msg("writing clear_refs failed\n");
>> +}
>> +
>> +
>> +static bool check_for_pattern(FILE *fp, const char *pattern, char *buf)
>> +{
>> +	while (fgets(buf, MAX_LINE_LENGTH, fp) != NULL) {
>> +		if (!strncmp(buf, pattern, strlen(pattern)))
>> +			return true;
>> +	}
>> +	return false;
>> +}
>> +
>> +uint64_t read_pmd_pagesize(void)
>> +{
>> +	int fd;
>> +	char buf[20];
>> +	ssize_t num_read;
>> +
>> +	fd = open(PMD_SIZE, O_RDONLY);
>> +	if (fd == -1)
>> +		ksft_exit_fail_msg("Open hpage_pmd_size failed\n");
>> +
>> +	num_read = read(fd, buf, 19);
>> +	if (num_read < 1) {
>> +		close(fd);
>> +		ksft_exit_fail_msg("Read hpage_pmd_size failed\n");
>> +	}
>> +	buf[num_read] = '\0';
>> +	close(fd);
>> +
>> +	return strtoul(buf, NULL, 10);
>> +}
>> +
>> +uint64_t check_huge(void *addr)
>> +{
>> +	uint64_t thp = 0;
>> +	int ret;
>> +	FILE *fp;
>> +	char buffer[MAX_LINE_LENGTH];
>> +	char addr_pattern[MAX_LINE_LENGTH];
>> +
>> +	ret = snprintf(addr_pattern, MAX_LINE_LENGTH, "%08lx-",
>> +		       (unsigned long) addr);
>> +	if (ret >= MAX_LINE_LENGTH)
>> +		ksft_exit_fail_msg("%s: Pattern is too long\n", __func__);
>> +
>> +	fp = fopen(SMAP, "r");
>> +	if (!fp)
>> +		ksft_exit_fail_msg("%s: Failed to open file %s\n", __func__, SMAP);
>> +
>> +	if (!check_for_pattern(fp, addr_pattern, buffer))
>> +		goto err_out;
>> +
>> +	/*
>> +	 * Fetch the AnonHugePages: in the same block and check the number of
>> +	 * hugepages.
>> +	 */
>> +	if (!check_for_pattern(fp, "AnonHugePages:", buffer))
>> +		goto err_out;
>> +
>> +	if (sscanf(buffer, "AnonHugePages:%10ld kB", &thp) != 1)
>> +		ksft_exit_fail_msg("Reading smap error\n");
>> +
>> +err_out:
>> +	fclose(fp);
>> +	return thp;
>> +}
>> diff --git a/tools/testing/selftests/vm/vm_util.h b/tools/testing/selftests/vm/vm_util.h
>> new file mode 100644
>> index 0000000000000..7522dbb859f0f
>> --- /dev/null
>> +++ b/tools/testing/selftests/vm/vm_util.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#include <stdint.h>
>> +#include <fcntl.h>
>> +#include "../kselftest.h"
>> +
>> +#define	PMD_SIZE		"/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"
> 
> Ehm no. PMD_SIZE_PATH at best -- just as it used to.
> 
>> +#define	SMAP			"/proc/self/smaps"
> 
> SMAPS_PATH
> 
>> +#define	DIRTY_BIT_LOCATION	55
> 
> Please inline that just as it used to. There is no value in a magic
> define without any proper namespace.
> 
>> +#define	MAX_LINE_LENGTH		512
> 
> This used to be 500. Why the change?
> 
> 
> Also: weird indentation and these all look like the should go into
> vm_util.c. They are not used outside that file.
> 
> 
> 

-- 
Muhammad Usama Anjum

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

* Re: [PATCH V4 2/2] selftests: vm: Add test for Soft-Dirty PTE bit
  2022-03-15 20:53   ` Gabriel Krisman Bertazi
@ 2022-03-16 17:36     ` Muhammad Usama Anjum
  0 siblings, 0 replies; 6+ messages in thread
From: Muhammad Usama Anjum @ 2022-03-16 17:36 UTC (permalink / raw)
  To: Gabriel Krisman Bertazi
  Cc: usama.anjum, Andrew Morton, Shuah Khan, kernel, Will Deacon,
	linux-kernel, linux-mm, linux-kselftest

On 3/16/22 1:53 AM, Gabriel Krisman Bertazi wrote:
> Muhammad Usama Anjum <usama.anjum@collabora.com> writes:
> 
>> From: Gabriel Krisman Bertazi <krisman@collabora.com>
> 
> Hi Usama,
> 
> Please, cc me on the whole thread.  I didn't get the patch 1/2 or the
> cover letter.
> 

Sorry, I'll correct it.

>> This introduces three tests:
>> 1) Sanity check soft dirty basic semantics: allocate area, clean, dirty,
>> check if the SD bit is flipped.
>> 2) Check VMA reuse: validate the VM_SOFTDIRTY usage
>> 3) Check soft-dirty on huge pages
>>
>> This was motivated by Will Deacon's fix commit 912efa17e512 ("mm: proc:
>> Invalidate TLB after clearing soft-dirty page state"). I was tracking the
>> same issue that he fixed, and this test would have caught it.
>>
>> CC: Will Deacon <will@kernel.org>
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>> V3 of this patch is in Andrew's tree. Please drop that.
> 
> v3 is still in linux-next and this note is quite hidden in the middle of
> the commit message.

I've tried to put this message at the top of the changelog. I can add
"Note" in the start of it. What can be some other way to highlight this
kind of important message?

>>
>> Changes in V4:
>> Cosmetic changes
>> Removed global variables
>> Replaced ksft_print_msg with ksft_exit_fail_msg to exit the program at
>> once
>> Some other minor changes
>> Correct the authorship of the patch
>>
>> Tests of soft dirty bit in this patch and in madv_populate.c are
>> non-overlapping. madv_populate.c has only one soft-dirty bit test in the
>> context of different advise (MADV_POPULATE_READ and
>> MADV_POPULATE_WRITE). This new test adds more tests.
>>
>> Tab width of 8 has been used to align the macros. This alignment may look
>> odd in shell or email. But it looks alright in editors.
> 
> I'm curious if you tested reverting 912efa17e512. Did the new versions
> of this patch still catch the original issue?

Yeah, it did after I reverted the patch and fixed build errors because
of some function's signature change and one test failed and hence issue
is caught:

TAP version 13
1..5
# dirty bit was 0, but should be 1 (i=1)
not ok 1 Test test_simple
ok 2 Test test_vma_reuse reused memory location
ok 3 Test test_vma_reuse dirty bit of previous page
ok 4 # SKIP Test test_hugepage huge page allocation
ok 5 # SKIP Test test_hugepage huge page dirty bit
# Totals: pass:2 fail:1 xfail:0 xpass:0 skip:2 error:0


>> Test output:
>> TAP version 13
>> 1..5
>> ok 1 Test test_simple
>> ok 2 Test test_vma_reuse reused memory location
>> ok 3 Test test_vma_reuse dirty bit of previous page
>> ok 4 Test test_hugepage huge page allocation
>> ok 5 Test test_hugepage huge page dirty bit
>>  # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
>>
>> Or
>>
>> TAP version 13
>> 1..5
>> ok 1 Test test_simple
>> ok 2 Test test_vma_reuse reused memory location
>> ok 3 Test test_vma_reuse dirty bit of previous page
>> ok 4 # SKIP Test test_hugepage huge page allocation
>> ok 5 # SKIP Test test_hugepage huge page dirty bit
>>  # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:2 error:0
[..]
>> +
>> +#define PAGEMAP			"/proc/self/pagemap"
>> +#define CLEAR_REFS		"/proc/self/clear_refs"
>> +#define MAX_LINE_LENGTH		512
> 
> MAX_LINE_LENGTH is no longer used after check_for_pattern was dropped.
> 
> Can't the previous defines and file handling functions also go the
> vm_util.h?
> 

I don't want to make changes in other two tests. I just want to move
some functions which we need for this test into vm_util.h while keeping
changes less.

>> +#define TEST_ITERATIONS		10000
>> +
>> +static void test_simple(int pagemap_fd, int pagesize)
>> +{
>> +	int i;
>> +	char *map;
>> +
>> +	map = aligned_alloc(pagesize, pagesize);
>> +	if (!map)
>> +		ksft_exit_fail_msg("mmap failed\n");
>> +
>> +	clear_softdirty();
>> +
>> +	for (i = 0 ; i < TEST_ITERATIONS; i++) {
>> +		if (pagemap_is_softdirty(pagemap_fd, map) == 1) {
>> +			ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
>> +			break;
>> +		}
>> +
>> +		clear_softdirty();
>> +		map[0]++;
> 
> 
> This will overflow several times during TEST_ITERATIONS.  While it is
> not broken, since we care about causing the page fault, it is not
> obvious.  Can you add a comment or do something like this instead?
> 
>   map[0] = !map[0];

Yeah, it is less obvious. I'll add a comment

-- 
Muhammad Usama Anjum

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

end of thread, other threads:[~2022-03-16 17:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-15  8:50 [PATCH V4 1/2] selftests: vm: bring common functions to a new file Muhammad Usama Anjum
2022-03-15  8:50 ` [PATCH V4 2/2] selftests: vm: Add test for Soft-Dirty PTE bit Muhammad Usama Anjum
2022-03-15 20:53   ` Gabriel Krisman Bertazi
2022-03-16 17:36     ` Muhammad Usama Anjum
2022-03-16  8:55 ` [PATCH V4 1/2] selftests: vm: bring common functions to a new file David Hildenbrand
2022-03-16 16:47   ` Muhammad Usama Anjum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).