[LTP] [PATCH v3 0/4] Hugetlb:Migrating the libhugetlbfs tests

[LTP] [PATCH v3 0/4] Hugetlb:Migrating the libhugetlbfs tests

[Tarun Sahu]

Hi,
Libhugetlbfs is not being maintained actively, and some distro is dropping
support for it. There are some tests that are good for testing hugetlb
functionality in kernel.

As per previous dicussion in RFC[1], Here, this patch series consists
of hugetlb tests taken from libhugetlbfs modified to work in ltp
environment.

Based on suggestion[2], I am reposting the patches again. Also, I added
suggested changes at [3].

ref:
1:https://lore.kernel.org/all/20220908173947.17956-1-tsahu@linux.ibm.com/
2:https://lore.kernel.org/all/87wn8xi61v.fsf@suse.de/
3:https://lore.kernel.org/all/20221016125731.249078-1-tsahu@linux.ibm.com/

v1 -> v2
	1. In (brk near huge) test [PATCH 1/3] removed unused library
	function test_addr_huge() and read_maps().
v2 -> v3
	1. Added a new patch commit for two new flags "needs_hugetlbfs" &
	"needs_unlinked_hugetlb_file" in tst_test and modified tests to use
	these flags. 
	2. Added taint check in test [PATCH 1/3].
	3. Removed redundant Hopt and nr_opt option.
	4. Corrected pre-processor ARCH based conditional flags in test
	[PATCH 1/3]

Tarun Sahu (4):
  Hugetlb: Add new tst_test options for hugeltb test support
  Hugetlb: Migrating libhugetlbfs brk_near_huge
  Hugetlb: Migrating libhugetlbfs chunk-overcommit
  Hugetlb: Migrating libhugetlbfs corrupt-by-cow-opt

 include/tst_test.h                            |  20 ++-
 lib/tst_test.c                                |  51 ++++++-
 runtest/hugetlb                               |   3 +
 testcases/kernel/mem/.gitignore               |   3 +
 .../kernel/mem/hugetlb/hugemmap/Makefile      |   5 +
 .../kernel/mem/hugetlb/hugemmap/hugemmap07.c  | 129 ++++++++++++++++
 .../kernel/mem/hugetlb/hugemmap/hugemmap08.c  | 144 ++++++++++++++++++
 .../kernel/mem/hugetlb/hugemmap/hugemmap09.c  |  71 +++++++++
 testcases/kernel/mem/hugetlb/lib/hugetlb.h    |   3 +
 9 files changed, 425 insertions(+), 4 deletions(-)
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c

-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Tarun Sahu]

Most of libhugetlbfs test require mounted hugetlbfs and random opened
unlinked file for follow-up test actions.

Here, this patch adds two new field in tst_test struct(include/tst_test.h)
which user can set if the test requires mounted hugetlbfs and other one
for if test requires opened unlinked file.

Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
---
 include/tst_test.h | 20 +++++++++++++++++-
 lib/tst_test.c     | 51 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/include/tst_test.h b/include/tst_test.h
index a69965b95..f36382ae9 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -131,7 +131,7 @@ struct tst_tag {
 };
 
 extern unsigned int tst_variant;
-
+extern int tst_hugetlb_fd;
 #define TST_NO_HUGEPAGES ((unsigned long)-1)
 
 #define TST_UNLIMITED_RUNTIME (-1)
@@ -176,6 +176,18 @@ struct tst_test {
 	int all_filesystems:1;
 	int skip_in_lockdown:1;
 	int skip_in_compat:1;
+	/*
+	 * If set, the test function will create a hugetlbfs mount point
+	 * at /tmp/xxxxxx, where xxxxxx is a random string.
+	 */
+	int needs_hugetlbfs:1;
+	/*
+	 * If set, the test function will create and open a random file
+	 * under mounted hugetlbfs. To use this option, needs_hugetlbfs must
+	 * be set. The file descriptior will be set in tst_hugetlb_fd.
+	 * The close(tst_hugetlb_fd) will be called on test exit(cleanup).
+	 */
+	int needs_unlinked_hugetlb_file:1;
 
 	/*
 	 * The skip_filesystems is a NULL terminated list of filesystems the
@@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void);
  */
 void tst_set_max_runtime(int max_runtime);
 
+/*
+ * Create and open a random file inside the given dir path.
+ * It unlinks the file after opening and return file descriptor.
+ */
+int tst_create_unlinked_file(const char *path);
+
 /*
  * Returns path to the test temporary directory in a newly allocated buffer.
  */
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 8ccde1629..43cba1004 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -925,7 +925,8 @@ static int needs_tmpdir(void)
 	       tst_test->needs_device ||
 	       tst_test->mntpoint ||
 	       tst_test->resource_files ||
-	       tst_test->needs_checkpoints;
+	       tst_test->needs_checkpoints ||
+		   tst_test->needs_hugetlbfs;
 }
 
 static void copy_resources(void)
@@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char *mntpoint)
 	}
 }
 
+static void prepare_and_mount_hugetlb_fs(void)
+{
+	tst_test->mntpoint = tst_get_tmpdir();
+	SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL);
+	mntpoint_mounted = 1;
+}
+
+int tst_create_unlinked_file(const char *path)
+{
+	char template[PATH_MAX];
+	int fd;
+
+	snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX",
+			path, TCID);
+
+	fd = mkstemp(template);
+	if (fd < 0)
+		tst_brk(TBROK | TERRNO,
+			 "%s: mkstemp(%s) failed", __func__, template);
+
+	SAFE_UNLINK(template);
+	return fd;
+}
+
 static const char *limit_tmpfs_mount_size(const char *mnt_data,
 		char *buf, size_t buf_size, const char *fs_type)
 {
@@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void)
 	tst_cg_init();
 }
 
+int tst_hugetlb_fd = -1;
+
 static void do_setup(int argc, char *argv[])
 {
 	if (!tst_test)
@@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[])
 		}
 	}
 
+	if (tst_test->needs_hugetlbfs)
+		prepare_and_mount_hugetlb_fs();
+
+	if (tst_test->needs_unlinked_hugetlb_file) {
+		if (!(tst_test->needs_hugetlbfs)) {
+			tst_brk(TBROK, "Option needs_unlinked_hugetlb_file "
+					"requires option needs_hugetlbfs");
+		}
+		tst_hugetlb_fd = tst_create_unlinked_file(tst_test->mntpoint);
+	}
+
 	if (tst_test->needs_device && !mntpoint_mounted) {
 		tdev.dev = tst_acquire_device_(NULL, tst_test->dev_min_size);
 
@@ -1299,8 +1337,15 @@ static void do_cleanup(void)
 	if (ovl_mounted)
 		SAFE_UMOUNT(OVL_MNT);
 
-	if (mntpoint_mounted)
-		tst_umount(tst_test->mntpoint);
+	if (tst_hugetlb_fd >= 0)
+		SAFE_CLOSE(tst_hugetlb_fd);
+
+	if (mntpoint_mounted) {
+		if (tst_test->needs_hugetlbfs)
+			SAFE_UMOUNT(tst_test->mntpoint);
+		else
+			tst_umount(tst_test->mntpoint);
+	}
 
 	if (tst_test->needs_device && tdev.dev)
 		tst_release_device(tdev.dev);
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 2/4] Hugetlb: Migrating libhugetlbfs brk_near_huge

[Tarun Sahu]

Migrating the libhugetlbfs/testcases/brk_near_huge.c test

Test Description:
Certain kernels have a bug where brk() does not perform the same
checks that a MAP_FIXED mmap() will, allowing brk() to create a
normal page VMA in a hugepage only address region. This can lead
to oopses or other badness.

Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
---
 runtest/hugetlb                               |   1 +
 testcases/kernel/mem/.gitignore               |   1 +
 .../kernel/mem/hugetlb/hugemmap/Makefile      |   5 +
 .../kernel/mem/hugetlb/hugemmap/hugemmap07.c  | 129 ++++++++++++++++++
 testcases/kernel/mem/hugetlb/lib/hugetlb.h    |   3 +
 5 files changed, 139 insertions(+)
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c

diff --git a/runtest/hugetlb b/runtest/hugetlb
index f719217ab..f7ff81cb3 100644
--- a/runtest/hugetlb
+++ b/runtest/hugetlb
@@ -3,6 +3,7 @@ hugemmap02 hugemmap02
 hugemmap04 hugemmap04
 hugemmap05 hugemmap05
 hugemmap06 hugemmap06
+hugemmap07 hugemmap07
 hugemmap05_1 hugemmap05 -m
 hugemmap05_2 hugemmap05 -s
 hugemmap05_3 hugemmap05 -s -m
diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
index ff2910533..df5256ec8 100644
--- a/testcases/kernel/mem/.gitignore
+++ b/testcases/kernel/mem/.gitignore
@@ -4,6 +4,7 @@
 /hugetlb/hugemmap/hugemmap04
 /hugetlb/hugemmap/hugemmap05
 /hugetlb/hugemmap/hugemmap06
+/hugetlb/hugemmap/hugemmap07
 /hugetlb/hugeshmat/hugeshmat01
 /hugetlb/hugeshmat/hugeshmat02
 /hugetlb/hugeshmat/hugeshmat03
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/Makefile b/testcases/kernel/mem/hugetlb/hugemmap/Makefile
index 2d651b4aa..a368db2b6 100644
--- a/testcases/kernel/mem/hugetlb/hugemmap/Makefile
+++ b/testcases/kernel/mem/hugetlb/hugemmap/Makefile
@@ -9,3 +9,8 @@ include $(abs_srcdir)/../Makefile.inc
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
 
 hugemmap06: CFLAGS+=-pthread
+
+hugemmap07:
+ifeq ($(ARCH),ppc)
+	CPPFLAGS += -DPPC_NO_SEGMENTS
+endif
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
new file mode 100644
index 000000000..5e04cef03
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+/*
+ * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation.
+ * Author: David Gibson & Adam Litke
+ */
+
+/*\
+ *
+ * [Description]
+ *
+ * brk() near hugepage:
+ * Certain kernels have a bug where brk() does not perform the same
+ * checks that a MAP_FIXED mmap() will, allowing brk() to create a
+ * normal page VMA in a hugepage only address region. This can lead
+ * to oopses or other badness.
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <sys/mount.h>
+#include <limits.h>
+#include <sys/param.h>
+#include <sys/types.h>
+
+#include "hugetlb.h"
+#include "tst_safe_stdio.h"
+
+static long hpage_size;
+
+#ifdef __powerpc64__
+static int arch_has_slice_support(void)
+{
+	char mmu_type[16];
+	FILE *fp;
+	int ret;
+
+	fp = SAFE_POPEN("cat /proc/cpuinfo | grep MMU | awk '{ print $3}'", "r");
+	ret = fscanf(fp, "%s", mmu_type);
+	pclose(fp);
+
+	if (ret < 0)
+		tst_brk(TBROK, "Failed to determine MMU type");
+
+	return strcmp(mmu_type, "Hash") == 0;
+}
+
+static void *next_chunk(void *addr)
+{
+	if (!arch_has_slice_support())
+		return PALIGN(addr, hpage_size);
+
+	if ((unsigned long)addr < 0x100000000UL)
+		/* 256M segments below 4G */
+		return PALIGN(addr, 0x10000000UL);
+	/* 1TB segments above */
+	return PALIGN(addr, 0x10000000000UL);
+}
+#elif defined(__powerpc__) && !defined(PPC_NO_SEGMENTS)
+static void *next_chunk(void *addr)
+{
+	return PALIGN(addr, 0x10000000UL);
+}
+#elif defined(__ia64__)
+static void *next_chunk(void *addr)
+{
+	return PALIGN(addr, 0x8000000000000000UL);
+}
+#else
+static void *next_chunk(void *addr)
+{
+	return PALIGN(addr, hpage_size);
+}
+#endif
+
+static void run_test(void)
+{
+	void *brk0, *hugemap_addr, *newbrk;
+	char *p;
+	int err;
+
+	brk0 = sbrk(0);
+	tst_res(TINFO, "Initial break at %p", brk0);
+
+	hugemap_addr = next_chunk(brk0) + hpage_size;
+
+	p = SAFE_MMAP(hugemap_addr, hpage_size, PROT_READ|PROT_WRITE,
+			MAP_PRIVATE|MAP_FIXED, tst_hugetlb_fd, 0);
+	if (p != hugemap_addr) {
+		tst_res(TFAIL, "mmap() at unexpected address %p instead of %p\n", p,
+		     hugemap_addr);
+		goto cleanup;
+	}
+
+	newbrk = next_chunk(brk0) + getpagesize();
+	err = brk((void *)newbrk);
+	if (err == -1) {
+		/* Failing the brk() is an acceptable kernel response */
+		tst_res(TPASS, "Failing the brk is an acceptable response");
+	} else {
+		/* Suceeding the brk() is acceptable if the new memory is
+		 * properly accesible and we don't have a kernel blow up when
+		 * we touch it.
+		 */
+		tst_res(TINFO, "New break at %p", newbrk);
+		memset(brk0, 0, newbrk-brk0);
+		tst_res(TPASS, "memory is accessible, hence successful brk() is "
+				"an acceptable response");
+	}
+cleanup:
+	SAFE_MUNMAP(p, hpage_size);
+	err = brk(brk0);
+	if (err == -1)
+		tst_brk(TBROK, "Failed to set break at the original position");
+}
+
+static void setup(void)
+{
+	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_hugetlbfs = 1,
+	.needs_unlinked_hugetlb_file = 1,
+	.taint_check = TST_TAINT_D | TST_TAINT_W,
+	.setup = setup,
+	.test_all = run_test,
+	.hugepages = {1, TST_NEEDS},
+};
diff --git a/testcases/kernel/mem/hugetlb/lib/hugetlb.h b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
index f75109f3e..1cfeca95a 100644
--- a/testcases/kernel/mem/hugetlb/lib/hugetlb.h
+++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
@@ -20,6 +20,9 @@
 #include "old_tmpdir.h"
 #include "mem.h"
 
+#define ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
+#define PALIGN(p, a) ((void *)ALIGN((unsigned long)(p), (a)))
+
 #define SHM_RD	0400
 #define SHM_WR	0200
 #define SHM_RW	(SHM_RD|SHM_WR)
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 3/4] Hugetlb: Migrating libhugetlbfs chunk-overcommit

[Tarun Sahu]

Migrating the libhugetlbfs/testcases/chunk-overcommit.c test

Test Description: Some kernel versions after hugepage demand allocation was
added used a dubious heuristic to check if there was enough hugepage space
available for a given mapping.  The number of not-already-instantiated
pages in the mapping was compared against the total hugepage free pool. It
was very easy to confuse this heuristic into overcommitting by allocating
hugepage memory in chunks, each less than the total available pool size but
together more than available.  This would generally lead to OOM SIGKILLs of
one process or another when it tried to instantiate pages beyond the
available pool.

Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
---
 runtest/hugetlb                               |   1 +
 testcases/kernel/mem/.gitignore               |   1 +
 .../kernel/mem/hugetlb/hugemmap/hugemmap08.c  | 144 ++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c

diff --git a/runtest/hugetlb b/runtest/hugetlb
index f7ff81cb3..664f18827 100644
--- a/runtest/hugetlb
+++ b/runtest/hugetlb
@@ -4,6 +4,7 @@ hugemmap04 hugemmap04
 hugemmap05 hugemmap05
 hugemmap06 hugemmap06
 hugemmap07 hugemmap07
+hugemmap08 hugemmap08
 hugemmap05_1 hugemmap05 -m
 hugemmap05_2 hugemmap05 -s
 hugemmap05_3 hugemmap05 -s -m
diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
index df5256ec8..003ce422b 100644
--- a/testcases/kernel/mem/.gitignore
+++ b/testcases/kernel/mem/.gitignore
@@ -5,6 +5,7 @@
 /hugetlb/hugemmap/hugemmap05
 /hugetlb/hugemmap/hugemmap06
 /hugetlb/hugemmap/hugemmap07
+/hugetlb/hugemmap/hugemmap08
 /hugetlb/hugeshmat/hugeshmat01
 /hugetlb/hugeshmat/hugeshmat02
 /hugetlb/hugeshmat/hugeshmat03
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
new file mode 100644
index 000000000..61db030d5
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+/*
+ * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation.
+ * Author: David Gibson & Adam Litke
+ */
+
+/*\
+ * [Description]
+ *
+ * Chunk Overcommit:
+ * Some kernel versions after hugepage demand allocation was added used a
+ * dubious heuristic to check if there was enough hugepage space available
+ * for a given mapping.  The number of not-already-instantiated pages in
+ * the mapping was compared against the total hugepage free pool. It was
+ * very easy to confuse this heuristic into overcommitting by allocating
+ * hugepage memory in chunks, each less than the total available pool size
+ * but together more than available.  This would generally lead to OOM
+ * SIGKILLs of one process or another when it tried to instantiate pages
+ * beyond the available pool.
+ *
+ * HISTORY
+ *
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mount.h>
+#include <limits.h>
+#include <sys/param.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <signal.h>
+
+#include "hugetlb.h"
+
+#define PROC_OVERCOMMIT "/proc/sys/vm/nr_overcommit_hugepages"
+#define WITH_OVERCOMMIT 0
+#define WITHOUT_OVERCOMMIT 1
+
+static long hpage_size;
+
+static void test_chunk_overcommit(void)
+{
+	unsigned long totpages, chunk1, chunk2;
+	void *p, *q;
+	pid_t child;
+	int status;
+
+	totpages = SAFE_READ_MEMINFO("HugePages_Free:");
+
+	chunk1 = (totpages / 2) + 1;
+	chunk2 = totpages - chunk1 + 1;
+
+	tst_res(TINFO, "Free: %ld hugepages available: "
+	       "chunk1=%ld chunk2=%ld", totpages, chunk1, chunk2);
+
+	p = SAFE_MMAP(NULL, chunk1*hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
+		 tst_hugetlb_fd, 0);
+
+	q = mmap(NULL, chunk2*hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
+		 tst_hugetlb_fd, chunk1*hpage_size);
+	if (q == MAP_FAILED) {
+		if (errno != ENOMEM) {
+			tst_res(TFAIL | TERRNO, "mmap() chunk2");
+			goto cleanup1;
+		} else {
+			tst_res(TPASS, "Successful without overcommit pages");
+			goto cleanup1;
+		}
+	}
+
+	tst_res(TINFO, "Looks like we've overcommitted, testing...");
+	/* Looks like we're overcommited, but we need to confirm that
+	 * this is bad.  We touch it all in a child process because an
+	 * overcommit will generally lead to a SIGKILL which we can't
+	 * handle, of course.
+	 */
+	child = SAFE_FORK();
+
+	if (child == 0) {
+		memset(p, 0, chunk1*hpage_size);
+		memset(q, 0, chunk2*hpage_size);
+		exit(0);
+	}
+
+	SAFE_WAITPID(child, &status, 0);
+
+	if (WIFSIGNALED(status)) {
+		tst_res(TFAIL, "Killed by signal '%s' due to overcommit",
+		     tst_strsig(WTERMSIG(status)));
+		goto cleanup2;
+	}
+
+	tst_res(TPASS, "Successful with overcommit pages");
+
+cleanup2:
+	SAFE_MUNMAP(q, chunk2*hpage_size);
+
+cleanup1:
+	SAFE_MUNMAP(p, chunk1*hpage_size);
+	SAFE_FTRUNCATE(tst_hugetlb_fd, 0);
+}
+
+static void run_test(unsigned int test_type)
+{
+	unsigned long saved_oc_hugepages;
+
+	SAFE_FILE_SCANF(PROC_OVERCOMMIT, "%ld", &saved_oc_hugepages);
+	switch (test_type) {
+	case WITHOUT_OVERCOMMIT:
+		tst_res(TINFO, "Without overcommit testing...");
+		if (saved_oc_hugepages > 0)
+			SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 0);
+		break;
+	case WITH_OVERCOMMIT:
+		tst_res(TINFO, "With overcommit testing...");
+		if (saved_oc_hugepages == 0)
+			SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 2);
+		break;
+	}
+	test_chunk_overcommit();
+}
+
+static void setup(void)
+{
+	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_hugetlbfs = 1,
+	.needs_unlinked_hugetlb_file = 1,
+	.forks_child = 1,
+	.save_restore = (const struct tst_path_val[]) {
+		{PROC_OVERCOMMIT, NULL},
+		{}
+	},
+	.tcnt = 2,
+	.setup = setup,
+	.test = run_test,
+	.hugepages = {3, TST_NEEDS},
+};
+
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 4/4] Hugetlb: Migrating libhugetlbfs corrupt-by-cow-opt

[Tarun Sahu]

Migrating the libhugetlbfs/testcases/corrupt-by-cow-opt.c test

Test Description: Test sanity of cow optimization on page cache. If a page
in page cache has only 1 ref count, it is mapped for a private mapping
directly and is overwritten freely, so next time we access the page, we
can see corrupt data.

Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
---
 runtest/hugetlb                               |  1 +
 testcases/kernel/mem/.gitignore               |  1 +
 .../kernel/mem/hugetlb/hugemmap/hugemmap09.c  | 71 +++++++++++++++++++
 3 files changed, 73 insertions(+)
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c

diff --git a/runtest/hugetlb b/runtest/hugetlb
index 664f18827..e2ada7a97 100644
--- a/runtest/hugetlb
+++ b/runtest/hugetlb
@@ -5,6 +5,7 @@ hugemmap05 hugemmap05
 hugemmap06 hugemmap06
 hugemmap07 hugemmap07
 hugemmap08 hugemmap08
+hugemmap09 hugemmap09
 hugemmap05_1 hugemmap05 -m
 hugemmap05_2 hugemmap05 -s
 hugemmap05_3 hugemmap05 -s -m
diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
index 003ce422b..1a242ffe0 100644
--- a/testcases/kernel/mem/.gitignore
+++ b/testcases/kernel/mem/.gitignore
@@ -6,6 +6,7 @@
 /hugetlb/hugemmap/hugemmap06
 /hugetlb/hugemmap/hugemmap07
 /hugetlb/hugemmap/hugemmap08
+/hugetlb/hugemmap/hugemmap09
 /hugetlb/hugeshmat/hugeshmat01
 /hugetlb/hugeshmat/hugeshmat02
 /hugetlb/hugeshmat/hugeshmat03
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
new file mode 100644
index 000000000..2189fe4b2
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+/*
+ * Copyright (C) 2013 Joonsoo Kim, LG Electronics.
+ * Author: Joonsoo Kim
+ */
+
+/*\
+ * [Description]
+ *
+ * Corrupt by COW optimization:
+ * Test sanity of cow optimization on page cache. If a page in page cache
+ * has only 1 ref count, it is mapped for a private mapping directly and
+ * is overwritten freely, so next time we access the page, we can see
+ * corrupt data.
+ *
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <sys/mount.h>
+#include <limits.h>
+#include <sys/param.h>
+#include <sys/types.h>
+
+#include "hugetlb.h"
+
+static long hpage_size;
+
+static void run_test(void)
+{
+	char *p;
+	char c;
+
+	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
+			tst_hugetlb_fd, 0);
+	*p = 's';
+	tst_res(TINFO, "Write %c to %p via shared mapping", *p, p);
+	SAFE_MUNMAP(p, hpage_size);
+
+	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_PRIVATE,
+			tst_hugetlb_fd, 0);
+	*p = 'p';
+	tst_res(TINFO, "Write %c to %p via private mapping", *p, p);
+	SAFE_MUNMAP(p, hpage_size);
+
+	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
+			tst_hugetlb_fd, 0);
+	c = *p;
+	tst_res(TINFO, "Read %c from %p via shared mapping", *p, p);
+	SAFE_MUNMAP(p, hpage_size);
+
+	/* Direct write from huge page */
+	if (c != 's')
+		tst_res(TFAIL, "Data got corrupted.");
+	else
+		tst_res(TPASS, "Successful");
+}
+
+static void setup(void)
+{
+	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_hugetlbfs = 1,
+	.needs_unlinked_hugetlb_file = 1,
+	.setup = setup,
+	.test_all = run_test,
+	.hugepages = {2, TST_NEEDS},
+};
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 6590 bytes --]

Hi Tarun,

This version is much better, comments are inline below.

On Sat, Oct 29, 2022 at 3:14 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:

> Most of libhugetlbfs test require mounted hugetlbfs and random opened
> unlinked file for follow-up test actions.
>
> Here, this patch adds two new field in tst_test struct(include/tst_test.h)
> which user can set if the test requires mounted hugetlbfs and other one
> for if test requires opened unlinked file.
>
> Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> ---
>  include/tst_test.h | 20 +++++++++++++++++-
>  lib/tst_test.c     | 51 +++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/include/tst_test.h b/include/tst_test.h
> index a69965b95..f36382ae9 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -131,7 +131,7 @@ struct tst_tag {
>  };
>
>  extern unsigned int tst_variant;
> -
> +extern int tst_hugetlb_fd;
>  #define TST_NO_HUGEPAGES ((unsigned long)-1)
>
>  #define TST_UNLIMITED_RUNTIME (-1)
> @@ -176,6 +176,18 @@ struct tst_test {
>         int all_filesystems:1;
>         int skip_in_lockdown:1;
>         int skip_in_compat:1;
> +       /*
> +        * If set, the test function will create a hugetlbfs mount point
> +        * at /tmp/xxxxxx, where xxxxxx is a random string.
> +        */
> +       int needs_hugetlbfs:1;
> +       /*
> +        * If set, the test function will create and open a random file
> +        * under mounted hugetlbfs. To use this option, needs_hugetlbfs
> must
> +        * be set. The file descriptior will be set in tst_hugetlb_fd.
> +        * The close(tst_hugetlb_fd) will be called on test exit(cleanup).
> +        */
> +       int needs_unlinked_hugetlb_file:1;
>

Why not consider encapsulating these two new fields in 'struct
tst_hugepage' ?

Then the tst_test in the case can simply initialize to:

....
static struct tst_test test = {
    .needs_root = 1,
    .taint_check = TST_TAINT_D | TST_TAINT_W,
    .setup = setup,
    .test_all = run_test,
    .hugepages = {1, TST_NEEDS, 1, 1},
};



>
>         /*
>          * The skip_filesystems is a NULL terminated list of filesystems
> the
> @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void);
>   */
>  void tst_set_max_runtime(int max_runtime);
>
> +/*
> + * Create and open a random file inside the given dir path.
> + * It unlinks the file after opening and return file descriptor.
> + */
> +int tst_create_unlinked_file(const char *path);
>

what about renaming this function to tst_'get|create'_unlinked_file_fd?
I guess the 'fd' part should be emphasized here.



> +
>  /*
>   * Returns path to the test temporary directory in a newly allocated
> buffer.
>   */
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 8ccde1629..43cba1004 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -925,7 +925,8 @@ static int needs_tmpdir(void)
>                tst_test->needs_device ||
>                tst_test->mntpoint ||
>                tst_test->resource_files ||
> -              tst_test->needs_checkpoints;
> +              tst_test->needs_checkpoints ||
> +                  tst_test->needs_hugetlbfs;
>  }
>
>  static void copy_resources(void)
> @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char
> *mntpoint)
>         }
>  }
>
> +static void prepare_and_mount_hugetlb_fs(void)
> +{
> +       tst_test->mntpoint = tst_get_tmpdir();
> +       SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL);
> +       mntpoint_mounted = 1;
> +}
> +
> +int tst_create_unlinked_file(const char *path)
> +{
> +       char template[PATH_MAX];
> +       int fd;
> +
> +       snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX",
> +                       path, TCID);
> +
> +       fd = mkstemp(template);
> +       if (fd < 0)
> +               tst_brk(TBROK | TERRNO,
> +                        "%s: mkstemp(%s) failed", __func__, template);
> +
> +       SAFE_UNLINK(template);
> +       return fd;
> +}
> +
>  static const char *limit_tmpfs_mount_size(const char *mnt_data,
>                 char *buf, size_t buf_size, const char *fs_type)
>  {
> @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void)
>         tst_cg_init();
>  }
>
> +int tst_hugetlb_fd = -1;
> +
>  static void do_setup(int argc, char *argv[])
>  {
>         if (!tst_test)
> @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[])
>                 }
>         }
>
> +       if (tst_test->needs_hugetlbfs)
> +               prepare_and_mount_hugetlb_fs();
> +
> +       if (tst_test->needs_unlinked_hugetlb_file) {
> +               if (!(tst_test->needs_hugetlbfs)) {
> +                       tst_brk(TBROK, "Option needs_unlinked_hugetlb_file
> "
> +                                       "requires option needs_hugetlbfs");
> +               }
> +               tst_hugetlb_fd =
> tst_create_unlinked_file(tst_test->mntpoint);
> +       }
> +
>

Seems we have to add a confliction check[1] to avoid multiple mounting
at 'tst_test->mntpoint'. Or maybe go another method to move all the
hugetlbfs
operations into tst_hugetlb.c to isolate details from the tst_test library,
but
this will require more changes for all preexisting hugetlb tests.


[1] something like this:

@@ -1224,9 +1224,9 @@ static void do_setup(int argc, char *argv[])
        }

        if (!!tst_test->needs_rofs + !!tst_test->needs_devfs +
-           !!tst_test->needs_device > 1) {
+           !!tst_test->needs_device + !!tst_test->needs_hugetlbfs > 1) {
                tst_brk(TBROK,
-                       "Two or more of needs_{rofs, devfs, device} are
set");
+                       "Two or more of needs_{rofs, devfs, device,
hugetlb} are set");
        }


        if (tst_test->needs_device && !mntpoint_mounted) {
>                 tdev.dev = tst_acquire_device_(NULL,
> tst_test->dev_min_size);


> @@ -1299,8 +1337,15 @@ static void do_cleanup(void)
>         if (ovl_mounted)
>                 SAFE_UMOUNT(OVL_MNT);
>
> -       if (mntpoint_mounted)
> -               tst_umount(tst_test->mntpoint);
> +       if (tst_hugetlb_fd >= 0)
> +               SAFE_CLOSE(tst_hugetlb_fd);
> +
> +       if (mntpoint_mounted) {
> +               if (tst_test->needs_hugetlbfs)
> +                       SAFE_UMOUNT(tst_test->mntpoint);
> +               else
> +                       tst_umount(tst_test->mntpoint);
> +       }
>
>         if (tst_test->needs_device && tdev.dev)
>                 tst_release_device(tdev.dev);
> --
> 2.31.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 10431 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 3/4] Hugetlb: Migrating libhugetlbfs chunk-overcommit

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 7712 bytes --]

On Sat, Oct 29, 2022 at 3:14 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:

> Migrating the libhugetlbfs/testcases/chunk-overcommit.c test
>
> Test Description: Some kernel versions after hugepage demand allocation was
> added used a dubious heuristic to check if there was enough hugepage space
> available for a given mapping.  The number of not-already-instantiated
> pages in the mapping was compared against the total hugepage free pool. It
> was very easy to confuse this heuristic into overcommitting by allocating
> hugepage memory in chunks, each less than the total available pool size but
> together more than available.  This would generally lead to OOM SIGKILLs of
> one process or another when it tried to instantiate pages beyond the
> available pool.
>
> Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> ---
>  runtest/hugetlb                               |   1 +
>  testcases/kernel/mem/.gitignore               |   1 +
>  .../kernel/mem/hugetlb/hugemmap/hugemmap08.c  | 144 ++++++++++++++++++
>  3 files changed, 146 insertions(+)
>  create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
>
> diff --git a/runtest/hugetlb b/runtest/hugetlb
> index f7ff81cb3..664f18827 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -4,6 +4,7 @@ hugemmap04 hugemmap04
>  hugemmap05 hugemmap05
>  hugemmap06 hugemmap06
>  hugemmap07 hugemmap07
> +hugemmap08 hugemmap08
>  hugemmap05_1 hugemmap05 -m
>  hugemmap05_2 hugemmap05 -s
>  hugemmap05_3 hugemmap05 -s -m
> diff --git a/testcases/kernel/mem/.gitignore
> b/testcases/kernel/mem/.gitignore
> index df5256ec8..003ce422b 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -5,6 +5,7 @@
>  /hugetlb/hugemmap/hugemmap05
>  /hugetlb/hugemmap/hugemmap06
>  /hugetlb/hugemmap/hugemmap07
> +/hugetlb/hugemmap/hugemmap08
>  /hugetlb/hugeshmat/hugeshmat01
>  /hugetlb/hugeshmat/hugeshmat02
>  /hugetlb/hugeshmat/hugeshmat03
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> new file mode 100644
> index 000000000..61db030d5
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> @@ -0,0 +1,144 @@
> +// SPDX-License-Identifier: LGPL-2.1-or-later
> +/*
> + * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation.
> + * Author: David Gibson & Adam Litke
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Chunk Overcommit:
> + * Some kernel versions after hugepage demand allocation was added used a
> + * dubious heuristic to check if there was enough hugepage space available
> + * for a given mapping.  The number of not-already-instantiated pages in
> + * the mapping was compared against the total hugepage free pool. It was
> + * very easy to confuse this heuristic into overcommitting by allocating
> + * hugepage memory in chunks, each less than the total available pool size
> + * but together more than available.  This would generally lead to OOM
> + * SIGKILLs of one process or another when it tried to instantiate pages
> + * beyond the available pool.
> + *
> + * HISTORY
> + *
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/mount.h>
> +#include <limits.h>
> +#include <sys/param.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <signal.h>
> +
> +#include "hugetlb.h"
> +
> +#define PROC_OVERCOMMIT "/proc/sys/vm/nr_overcommit_hugepages"
> +#define WITH_OVERCOMMIT 0
> +#define WITHOUT_OVERCOMMIT 1
> +
> +static long hpage_size;
> +
> +static void test_chunk_overcommit(void)
> +{
> +       unsigned long totpages, chunk1, chunk2;
> +       void *p, *q;
> +       pid_t child;
> +       int status;
> +
> +       totpages = SAFE_READ_MEMINFO("HugePages_Free:");
> +
> +       chunk1 = (totpages / 2) + 1;
> +       chunk2 = totpages - chunk1 + 1;
> +
> +       tst_res(TINFO, "Free: %ld hugepages available: "
> +              "chunk1=%ld chunk2=%ld", totpages, chunk1, chunk2);
> +
> +       p = SAFE_MMAP(NULL, chunk1*hpage_size, PROT_READ|PROT_WRITE,
> MAP_SHARED,
> +                tst_hugetlb_fd, 0);
> +
> +       q = mmap(NULL, chunk2*hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
> +                tst_hugetlb_fd, chunk1*hpage_size);
> +       if (q == MAP_FAILED) {
> +               if (errno != ENOMEM) {
> +                       tst_res(TFAIL | TERRNO, "mmap() chunk2");
> +                       goto cleanup1;
> +               } else {
> +                       tst_res(TPASS, "Successful without overcommit
> pages");
> +                       goto cleanup1;
> +               }
> +       }
> +
> +       tst_res(TINFO, "Looks like we've overcommitted, testing...");
> +       /* Looks like we're overcommited, but we need to confirm that
> +        * this is bad.  We touch it all in a child process because an
> +        * overcommit will generally lead to a SIGKILL which we can't
> +        * handle, of course.
> +        */
> +       child = SAFE_FORK();
> +
> +       if (child == 0) {
> +               memset(p, 0, chunk1*hpage_size);
> +               memset(q, 0, chunk2*hpage_size);
> +               exit(0);
> +       }
> +
> +       SAFE_WAITPID(child, &status, 0);
> +
> +       if (WIFSIGNALED(status)) {
> +               tst_res(TFAIL, "Killed by signal '%s' due to overcommit",
> +                    tst_strsig(WTERMSIG(status)));
> +               goto cleanup2;
> +       }
> +
> +       tst_res(TPASS, "Successful with overcommit pages");
> +
> +cleanup2:
> +       SAFE_MUNMAP(q, chunk2*hpage_size);
> +
> +cleanup1:
> +       SAFE_MUNMAP(p, chunk1*hpage_size);
> +       SAFE_FTRUNCATE(tst_hugetlb_fd, 0);
> +}
> +
> +static void run_test(unsigned int test_type)
> +{
> +       unsigned long saved_oc_hugepages;
> +
> +       SAFE_FILE_SCANF(PROC_OVERCOMMIT, "%ld", &saved_oc_hugepages);
>

There is unnecessary to read PROC_OVERCOMMIT value again,
we already save/restore it in struct tst_path_val[], so here we
can set it directly to what we expected no matter if the original is 0 or 2.

static void run_test(unsigned int test_type)
{
        switch (test_type) {
        case WITHOUT_OVERCOMMIT:
                tst_res(TINFO, "Without overcommit testing...");
                SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 0);
                break;
        case WITH_OVERCOMMIT:
                tst_res(TINFO, "With overcommit testing...");
                SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 2);
                break;
    }
    test_chunk_overcommit();
}



> +       switch (test_type) {
> +       case WITHOUT_OVERCOMMIT:
> +               tst_res(TINFO, "Without overcommit testing...");
> +               if (saved_oc_hugepages > 0)
> +                       SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 0);
> +               break;
> +       case WITH_OVERCOMMIT:
> +               tst_res(TINFO, "With overcommit testing...");
> +               if (saved_oc_hugepages == 0)
> +                       SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 2);
> +               break;
> +       }
> +       test_chunk_overcommit();
> +}
> +
> +static void setup(void)
> +{
> +       hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
> +}
> +
> +static struct tst_test test = {
> +       .needs_root = 1,
> +       .needs_hugetlbfs = 1,
> +       .needs_unlinked_hugetlb_file = 1,
> +       .forks_child = 1,
> +       .save_restore = (const struct tst_path_val[]) {
> +               {PROC_OVERCOMMIT, NULL},
> +               {}
> +       },
> +       .tcnt = 2,
> +       .setup = setup,
> +       .test = run_test,
> +       .hugepages = {3, TST_NEEDS},
> +};
> +
> --
> 2.31.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 10266 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Tarun Sahu]

Hello,
Thanks for reviewing patch Li,

On Oct 31 2022, Li Wang wrote:
> Hi Tarun,
> 
> This version is much better, comments are inline below.
> 
> On Sat, Oct 29, 2022 at 3:14 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:
> 
> > Most of libhugetlbfs test require mounted hugetlbfs and random opened
> > unlinked file for follow-up test actions.
> >
> > Here, this patch adds two new field in tst_test struct(include/tst_test.h)
> > which user can set if the test requires mounted hugetlbfs and other one
> > for if test requires opened unlinked file.
> >
> > Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> > ---
> >  include/tst_test.h | 20 +++++++++++++++++-
> >  lib/tst_test.c     | 51 +++++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 67 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/tst_test.h b/include/tst_test.h
> > index a69965b95..f36382ae9 100644
> > --- a/include/tst_test.h
> > +++ b/include/tst_test.h
> > @@ -131,7 +131,7 @@ struct tst_tag {
> >  };
> >
> >  extern unsigned int tst_variant;
> > -
> > +extern int tst_hugetlb_fd;
> >  #define TST_NO_HUGEPAGES ((unsigned long)-1)
> >
> >  #define TST_UNLIMITED_RUNTIME (-1)
> > @@ -176,6 +176,18 @@ struct tst_test {
> >         int all_filesystems:1;
> >         int skip_in_lockdown:1;
> >         int skip_in_compat:1;
> > +       /*
> > +        * If set, the test function will create a hugetlbfs mount point
> > +        * at /tmp/xxxxxx, where xxxxxx is a random string.
> > +        */
> > +       int needs_hugetlbfs:1;
> > +       /*
> > +        * If set, the test function will create and open a random file
> > +        * under mounted hugetlbfs. To use this option, needs_hugetlbfs
> > must
> > +        * be set. The file descriptior will be set in tst_hugetlb_fd.
> > +        * The close(tst_hugetlb_fd) will be called on test exit(cleanup).
> > +        */
> > +       int needs_unlinked_hugetlb_file:1;
> >
> 
> Why not consider encapsulating these two new fields in 'struct
> tst_hugepage' ?
> 
> Then the tst_test in the case can simply initialize to:
> 
> ....
> static struct tst_test test = {
>     .needs_root = 1,
>     .taint_check = TST_TAINT_D | TST_TAINT_W,
>     .setup = setup,
>     .test_all = run_test,
>     .hugepages = {1, TST_NEEDS, 1, 1},
> };
> 
Ok, I will move these fields in tst_hugepages struct.
> 
> 
> >
> >         /*
> >          * The skip_filesystems is a NULL terminated list of filesystems
> > the
> > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void);
> >   */
> >  void tst_set_max_runtime(int max_runtime);
> >
> > +/*
> > + * Create and open a random file inside the given dir path.
> > + * It unlinks the file after opening and return file descriptor.
> > + */
> > +int tst_create_unlinked_file(const char *path);
> >
> 
> what about renaming this function to tst_'get|create'_unlinked_file_fd?
> I guess the 'fd' part should be emphasized here.
> 
OK. Will update it.
> 
> 
> > +
> >  /*
> >   * Returns path to the test temporary directory in a newly allocated
> > buffer.
> >   */
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index 8ccde1629..43cba1004 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -925,7 +925,8 @@ static int needs_tmpdir(void)
> >                tst_test->needs_device ||
> >                tst_test->mntpoint ||
> >                tst_test->resource_files ||
> > -              tst_test->needs_checkpoints;
> > +              tst_test->needs_checkpoints ||
> > +                  tst_test->needs_hugetlbfs;
> >  }
> >
> >  static void copy_resources(void)
> > @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char
> > *mntpoint)
> >         }
> >  }
> >
> > +static void prepare_and_mount_hugetlb_fs(void)
> > +{
> > +       tst_test->mntpoint = tst_get_tmpdir();
> > +       SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL);
> > +       mntpoint_mounted = 1;
> > +}
> > +
> > +int tst_create_unlinked_file(const char *path)
> > +{
> > +       char template[PATH_MAX];
> > +       int fd;
> > +
> > +       snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX",
> > +                       path, TCID);
> > +
> > +       fd = mkstemp(template);
> > +       if (fd < 0)
> > +               tst_brk(TBROK | TERRNO,
> > +                        "%s: mkstemp(%s) failed", __func__, template);
> > +
> > +       SAFE_UNLINK(template);
> > +       return fd;
> > +}
> > +
> >  static const char *limit_tmpfs_mount_size(const char *mnt_data,
> >                 char *buf, size_t buf_size, const char *fs_type)
> >  {
> > @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void)
> >         tst_cg_init();
> >  }
> >
> > +int tst_hugetlb_fd = -1;
> > +
> >  static void do_setup(int argc, char *argv[])
> >  {
> >         if (!tst_test)
> > @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[])
> >                 }
> >         }
> >
> > +       if (tst_test->needs_hugetlbfs)
> > +               prepare_and_mount_hugetlb_fs();
> > +
> > +       if (tst_test->needs_unlinked_hugetlb_file) {
> > +               if (!(tst_test->needs_hugetlbfs)) {
> > +                       tst_brk(TBROK, "Option needs_unlinked_hugetlb_file
> > "
> > +                                       "requires option needs_hugetlbfs");
> > +               }
> > +               tst_hugetlb_fd =
> > tst_create_unlinked_file(tst_test->mntpoint);
> > +       }
> > +
> >
> 
> Seems we have to add a confliction check[1] to avoid multiple mounting
> at 'tst_test->mntpoint'. Or maybe go another method to move all the
> hugetlbfs
> operations into tst_hugetlb.c to isolate details from the tst_test library,
> but
> this will require more changes for all preexisting hugetlb tests.
> 
> 
> [1] something like this:
> 
> @@ -1224,9 +1224,9 @@ static void do_setup(int argc, char *argv[])
>         }
> 
>         if (!!tst_test->needs_rofs + !!tst_test->needs_devfs +
> -           !!tst_test->needs_device > 1) {
> +           !!tst_test->needs_device + !!tst_test->needs_hugetlbfs > 1) {
>                 tst_brk(TBROK,
> -                       "Two or more of needs_{rofs, devfs, device} are
> set");
> +                       "Two or more of needs_{rofs, devfs, device,
> hugetlb} are set");
>         }
> 
OK. Will update it.
> 
>         if (tst_test->needs_device && !mntpoint_mounted) {
> >                 tdev.dev = tst_acquire_device_(NULL,
> > tst_test->dev_min_size);
> 
> 
> > @@ -1299,8 +1337,15 @@ static void do_cleanup(void)
> >         if (ovl_mounted)
> >                 SAFE_UMOUNT(OVL_MNT);
> >
> > -       if (mntpoint_mounted)
> > -               tst_umount(tst_test->mntpoint);
> > +       if (tst_hugetlb_fd >= 0)
> > +               SAFE_CLOSE(tst_hugetlb_fd);
> > +
> > +       if (mntpoint_mounted) {
> > +               if (tst_test->needs_hugetlbfs)
> > +                       SAFE_UMOUNT(tst_test->mntpoint);
> > +               else
> > +                       tst_umount(tst_test->mntpoint);
> > +       }
> >
> >         if (tst_test->needs_device && tdev.dev)
> >                 tst_release_device(tdev.dev);
> > --
> > 2.31.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> >
> >
> 
> -- 
> Regards,
> Li Wang

> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 3/4] Hugetlb: Migrating libhugetlbfs chunk-overcommit

[Tarun Sahu]

On Oct 31 2022, Li Wang wrote:
> On Sat, Oct 29, 2022 at 3:14 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:
> 
> > Migrating the libhugetlbfs/testcases/chunk-overcommit.c test
> >
> > Test Description: Some kernel versions after hugepage demand allocation was
> > added used a dubious heuristic to check if there was enough hugepage space
> > available for a given mapping.  The number of not-already-instantiated
> > pages in the mapping was compared against the total hugepage free pool. It
> > was very easy to confuse this heuristic into overcommitting by allocating
> > hugepage memory in chunks, each less than the total available pool size but
> > together more than available.  This would generally lead to OOM SIGKILLs of
> > one process or another when it tried to instantiate pages beyond the
> > available pool.
> >
> > Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> > ---
> >  runtest/hugetlb                               |   1 +
> >  testcases/kernel/mem/.gitignore               |   1 +
> >  .../kernel/mem/hugetlb/hugemmap/hugemmap08.c  | 144 ++++++++++++++++++
> >  3 files changed, 146 insertions(+)
> >  create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> >
> > diff --git a/runtest/hugetlb b/runtest/hugetlb
> > index f7ff81cb3..664f18827 100644
> > --- a/runtest/hugetlb
> > +++ b/runtest/hugetlb
> > @@ -4,6 +4,7 @@ hugemmap04 hugemmap04
> >  hugemmap05 hugemmap05
> >  hugemmap06 hugemmap06
> >  hugemmap07 hugemmap07
> > +hugemmap08 hugemmap08
> >  hugemmap05_1 hugemmap05 -m
> >  hugemmap05_2 hugemmap05 -s
> >  hugemmap05_3 hugemmap05 -s -m
> > diff --git a/testcases/kernel/mem/.gitignore
> > b/testcases/kernel/mem/.gitignore
> > index df5256ec8..003ce422b 100644
> > --- a/testcases/kernel/mem/.gitignore
> > +++ b/testcases/kernel/mem/.gitignore
> > @@ -5,6 +5,7 @@
> >  /hugetlb/hugemmap/hugemmap05
> >  /hugetlb/hugemmap/hugemmap06
> >  /hugetlb/hugemmap/hugemmap07
> > +/hugetlb/hugemmap/hugemmap08
> >  /hugetlb/hugeshmat/hugeshmat01
> >  /hugetlb/hugeshmat/hugeshmat02
> >  /hugetlb/hugeshmat/hugeshmat03
> > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> > b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> > new file mode 100644
> > index 000000000..61db030d5
> > --- /dev/null
> > +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> > @@ -0,0 +1,144 @@
> > +// SPDX-License-Identifier: LGPL-2.1-or-later
> > +/*
> > + * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation.
> > + * Author: David Gibson & Adam Litke
> > + */
> > +
> > +/*\
> > + * [Description]
> > + *
> > + * Chunk Overcommit:
> > + * Some kernel versions after hugepage demand allocation was added used a
> > + * dubious heuristic to check if there was enough hugepage space available
> > + * for a given mapping.  The number of not-already-instantiated pages in
> > + * the mapping was compared against the total hugepage free pool. It was
> > + * very easy to confuse this heuristic into overcommitting by allocating
> > + * hugepage memory in chunks, each less than the total available pool size
> > + * but together more than available.  This would generally lead to OOM
> > + * SIGKILLs of one process or another when it tried to instantiate pages
> > + * beyond the available pool.
> > + *
> > + * HISTORY
> > + *
> > + */
> > +
> > +#define _GNU_SOURCE
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <sys/mount.h>
> > +#include <limits.h>
> > +#include <sys/param.h>
> > +#include <sys/types.h>
> > +#include <sys/wait.h>
> > +#include <signal.h>
> > +
> > +#include "hugetlb.h"
> > +
> > +#define PROC_OVERCOMMIT "/proc/sys/vm/nr_overcommit_hugepages"
> > +#define WITH_OVERCOMMIT 0
> > +#define WITHOUT_OVERCOMMIT 1
> > +
> > +static long hpage_size;
> > +
> > +static void test_chunk_overcommit(void)
> > +{
> > +       unsigned long totpages, chunk1, chunk2;
> > +       void *p, *q;
> > +       pid_t child;
> > +       int status;
> > +
> > +       totpages = SAFE_READ_MEMINFO("HugePages_Free:");
> > +
> > +       chunk1 = (totpages / 2) + 1;
> > +       chunk2 = totpages - chunk1 + 1;
> > +
> > +       tst_res(TINFO, "Free: %ld hugepages available: "
> > +              "chunk1=%ld chunk2=%ld", totpages, chunk1, chunk2);
> > +
> > +       p = SAFE_MMAP(NULL, chunk1*hpage_size, PROT_READ|PROT_WRITE,
> > MAP_SHARED,
> > +                tst_hugetlb_fd, 0);
> > +
> > +       q = mmap(NULL, chunk2*hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
> > +                tst_hugetlb_fd, chunk1*hpage_size);
> > +       if (q == MAP_FAILED) {
> > +               if (errno != ENOMEM) {
> > +                       tst_res(TFAIL | TERRNO, "mmap() chunk2");
> > +                       goto cleanup1;
> > +               } else {
> > +                       tst_res(TPASS, "Successful without overcommit
> > pages");
> > +                       goto cleanup1;
> > +               }
> > +       }
> > +
> > +       tst_res(TINFO, "Looks like we've overcommitted, testing...");
> > +       /* Looks like we're overcommited, but we need to confirm that
> > +        * this is bad.  We touch it all in a child process because an
> > +        * overcommit will generally lead to a SIGKILL which we can't
> > +        * handle, of course.
> > +        */
> > +       child = SAFE_FORK();
> > +
> > +       if (child == 0) {
> > +               memset(p, 0, chunk1*hpage_size);
> > +               memset(q, 0, chunk2*hpage_size);
> > +               exit(0);
> > +       }
> > +
> > +       SAFE_WAITPID(child, &status, 0);
> > +
> > +       if (WIFSIGNALED(status)) {
> > +               tst_res(TFAIL, "Killed by signal '%s' due to overcommit",
> > +                    tst_strsig(WTERMSIG(status)));
> > +               goto cleanup2;
> > +       }
> > +
> > +       tst_res(TPASS, "Successful with overcommit pages");
> > +
> > +cleanup2:
> > +       SAFE_MUNMAP(q, chunk2*hpage_size);
> > +
> > +cleanup1:
> > +       SAFE_MUNMAP(p, chunk1*hpage_size);
> > +       SAFE_FTRUNCATE(tst_hugetlb_fd, 0);
> > +}
> > +
> > +static void run_test(unsigned int test_type)
> > +{
> > +       unsigned long saved_oc_hugepages;
> > +
> > +       SAFE_FILE_SCANF(PROC_OVERCOMMIT, "%ld", &saved_oc_hugepages);
> >
> 
> There is unnecessary to read PROC_OVERCOMMIT value again,
> we already save/restore it in struct tst_path_val[], so here we
> can set it directly to what we expected no matter if the original is 0 or 2.
> 
> static void run_test(unsigned int test_type)
> {
>         switch (test_type) {
>         case WITHOUT_OVERCOMMIT:
>                 tst_res(TINFO, "Without overcommit testing...");
>                 SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 0);
>                 break;
>         case WITH_OVERCOMMIT:
>                 tst_res(TINFO, "With overcommit testing...");
>                 SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 2);
>                 break;
>     }
>     test_chunk_overcommit();
> }
> 
Yeah, Missed it. Will update it.
> 
> 
> > +       switch (test_type) {
> > +       case WITHOUT_OVERCOMMIT:
> > +               tst_res(TINFO, "Without overcommit testing...");
> > +               if (saved_oc_hugepages > 0)
> > +                       SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 0);
> > +               break;
> > +       case WITH_OVERCOMMIT:
> > +               tst_res(TINFO, "With overcommit testing...");
> > +               if (saved_oc_hugepages == 0)
> > +                       SAFE_FILE_PRINTF(PROC_OVERCOMMIT, "%d", 2);
> > +               break;
> > +       }
> > +       test_chunk_overcommit();
> > +}
> > +
> > +static void setup(void)
> > +{
> > +       hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
> > +}
> > +
> > +static struct tst_test test = {
> > +       .needs_root = 1,
> > +       .needs_hugetlbfs = 1,
> > +       .needs_unlinked_hugetlb_file = 1,
> > +       .forks_child = 1,
> > +       .save_restore = (const struct tst_path_val[]) {
> > +               {PROC_OVERCOMMIT, NULL},
> > +               {}
> > +       },
> > +       .tcnt = 2,
> > +       .setup = setup,
> > +       .test = run_test,
> > +       .hugepages = {3, TST_NEEDS},
> > +};
> > +
> > --
> > 2.31.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> >
> >
> 
> -- 
> Regards,
> Li Wang

> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Cyril Hrubis]

Hi!
>  extern unsigned int tst_variant;
> -
> +extern int tst_hugetlb_fd;
>  #define TST_NO_HUGEPAGES ((unsigned long)-1)
>  
>  #define TST_UNLIMITED_RUNTIME (-1)
> @@ -176,6 +176,18 @@ struct tst_test {
>  	int all_filesystems:1;
>  	int skip_in_lockdown:1;
>  	int skip_in_compat:1;
> +	/*
> +	 * If set, the test function will create a hugetlbfs mount point
> +	 * at /tmp/xxxxxx, where xxxxxx is a random string.
> +	 */
> +	int needs_hugetlbfs:1;
> +	/*
> +	 * If set, the test function will create and open a random file
> +	 * under mounted hugetlbfs. To use this option, needs_hugetlbfs must
> +	 * be set. The file descriptior will be set in tst_hugetlb_fd.
> +	 * The close(tst_hugetlb_fd) will be called on test exit(cleanup).
> +	 */
> +	int needs_unlinked_hugetlb_file:1;
>  
>  	/*
>  	 * The skip_filesystems is a NULL terminated list of filesystems the
> @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void);
>   */
>  void tst_set_max_runtime(int max_runtime);
>  
> +/*
> + * Create and open a random file inside the given dir path.
> + * It unlinks the file after opening and return file descriptor.
> + */
> +int tst_create_unlinked_file(const char *path);
> +
>  /*
>   * Returns path to the test temporary directory in a newly allocated buffer.
>   */
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 8ccde1629..43cba1004 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -925,7 +925,8 @@ static int needs_tmpdir(void)
>  	       tst_test->needs_device ||
>  	       tst_test->mntpoint ||
>  	       tst_test->resource_files ||
> -	       tst_test->needs_checkpoints;
> +	       tst_test->needs_checkpoints ||
> +		   tst_test->needs_hugetlbfs;
>  }
>  
>  static void copy_resources(void)
> @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char *mntpoint)
>  	}
>  }
>  
> +static void prepare_and_mount_hugetlb_fs(void)
> +{
> +	tst_test->mntpoint = tst_get_tmpdir();
> +	SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL);
> +	mntpoint_mounted = 1;
> +}
> +
> +int tst_create_unlinked_file(const char *path)
> +{
> +	char template[PATH_MAX];
> +	int fd;
> +
> +	snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX",
> +			path, TCID);
> +
> +	fd = mkstemp(template);
> +	if (fd < 0)
> +		tst_brk(TBROK | TERRNO,
> +			 "%s: mkstemp(%s) failed", __func__, template);
> +
> +	SAFE_UNLINK(template);
> +	return fd;
> +}
> +
>  static const char *limit_tmpfs_mount_size(const char *mnt_data,
>  		char *buf, size_t buf_size, const char *fs_type)
>  {
> @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void)
>  	tst_cg_init();
>  }
>  
> +int tst_hugetlb_fd = -1;
> +
>  static void do_setup(int argc, char *argv[])
>  {
>  	if (!tst_test)
> @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[])
>  		}
>  	}
>  
> +	if (tst_test->needs_hugetlbfs)
> +		prepare_and_mount_hugetlb_fs();
> +
> +	if (tst_test->needs_unlinked_hugetlb_file) {
> +		if (!(tst_test->needs_hugetlbfs)) {
> +			tst_brk(TBROK, "Option needs_unlinked_hugetlb_file "
> +					"requires option needs_hugetlbfs");
> +		}
> +		tst_hugetlb_fd = tst_create_unlinked_file(tst_test->mntpoint);
> +	}

The function tst_create_unlinked_file() looks useful, but I do not think
that adding the needs_unlinked_hugetlb_file flag simplifies things that
much. Also this will not scale well when we would need two
filedescripors like that. Maybe we it would be cleaner to add only the
mount/umount functionality to the test library and call the
tst_create_unlinked_file() in the test setup in the testcases.

>  	if (tst_test->needs_device && !mntpoint_mounted) {
>  		tdev.dev = tst_acquire_device_(NULL, tst_test->dev_min_size);
>  
> @@ -1299,8 +1337,15 @@ static void do_cleanup(void)
>  	if (ovl_mounted)
>  		SAFE_UMOUNT(OVL_MNT);
>  
> -	if (mntpoint_mounted)
> -		tst_umount(tst_test->mntpoint);
> +	if (tst_hugetlb_fd >= 0)
> +		SAFE_CLOSE(tst_hugetlb_fd);
> +
> +	if (mntpoint_mounted) {
> +		if (tst_test->needs_hugetlbfs)
> +			SAFE_UMOUNT(tst_test->mntpoint);
> +		else
> +			tst_umount(tst_test->mntpoint);
> +	}

Is there a good reason for this, why can't we call tst_umount() for
hugetlbfs?

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Cyril Hrubis]

Hi!
> > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void);
> >   */
> >  void tst_set_max_runtime(int max_runtime);
> >
> > +/*
> > + * Create and open a random file inside the given dir path.
> > + * It unlinks the file after opening and return file descriptor.
> > + */
> > +int tst_create_unlinked_file(const char *path);
> >
> 
> what about renaming this function to tst_'get|create'_unlinked_file_fd?
> I guess the 'fd' part should be emphasized here.

It has create in name and in UNIX creat() creates file and returns a
file descriptor so I think it's fine. Maybe we can be even shorted with
something as tst_creat_unlinked(const char *path).

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Cyril Hrubis]

Hi!
> Why not consider encapsulating these two new fields in 'struct
> tst_hugepage' ?
> 
> Then the tst_test in the case can simply initialize to:
> 
> ....
> static struct tst_test test = {
>     .needs_root = 1,
>     .taint_check = TST_TAINT_D | TST_TAINT_W,
>     .setup = setup,
>     .test_all = run_test,
>     .hugepages = {1, TST_NEEDS, 1, 1},
> };

I do not like that we have magic constants in the .hugepages that are
not self describing. I would treat the hugetltbfs just as we treat
devfs, that would be:

#define MNTPOINT "hugetlbfs/"
#define HUGEFILE MNTPOINT "hugefile"

static int huge_fd;

static void setup(void)
{
	huge_fd = tst_creat_unlinked(HUGEFILE);
	...
}

static void cleanup(void)
{
	if (huge_fd > 0)
		SAFE_CLOSE(huge_fd);
}

static struct tst_test test = {
	...
	.mntpoint = MNTPOINT,
	.needs_hugetlbfs = 1,
	.setup = setup,
	.cleanup = cleanup,
	...
}


What do you think?

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Tarun Sahu]

Hi Cyril,
Please find my comments inline.

On Oct 31 2022, Cyril Hrubis wrote:
> Hi!
> > Why not consider encapsulating these two new fields in 'struct
> > tst_hugepage' ?
> > 
> > Then the tst_test in the case can simply initialize to:
> > 
> > ....
> > static struct tst_test test = {
> >     .needs_root = 1,
> >     .taint_check = TST_TAINT_D | TST_TAINT_W,
> >     .setup = setup,
> >     .test_all = run_test,
> >     .hugepages = {1, TST_NEEDS, 1, 1},
> > };
> 
> I do not like that we have magic constants in the .hugepages that are
> not self describing. I would treat the hugetltbfs just as we treat
> devfs, that would be:
> 
> #define MNTPOINT "hugetlbfs/"
> #define HUGEFILE MNTPOINT "hugefile"
> 
> static int huge_fd;
> 
> static void setup(void)
> {
> 	huge_fd = tst_creat_unlinked(HUGEFILE);
> 	...
> }
> 
> static void cleanup(void)
> {
> 	if (huge_fd > 0)
> 		SAFE_CLOSE(huge_fd);
> }
> 
> static struct tst_test test = {
> 	...
> 	.mntpoint = MNTPOINT,
> 	.needs_hugetlbfs = 1,
> 	.setup = setup,
> 	.cleanup = cleanup,
> 	...
> }
> 
> 
> What do you think?
> 
My original idea behind putting it in tst_test struct instead of
tst_hugepages was based on below two reasoning-

1. tst_hugepages seems to have only hugepages related info. It would be
better to rename it to tst_hugetlb and rename <hugepages> field in tst_test
struct to <hugetlb>. If we rename it which will require changes in already
existing tests. and Moreover, there were similar field like needs_tmpdir
in tst_test so I put it(needs_unlinked_hugetlb_file) in tst_test.
2. There was already related fields in tst_test for mounting fs, like
needs_devfs, needs_rofs, So keeping all the needs_ABCfs at same structure.

> -- 
> Cyril Hrubis
> chrubis@suse.cz
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Tarun Sahu]

On Oct 31 2022, Cyril Hrubis wrote:
> Hi!
> >  extern unsigned int tst_variant;
> > -
> > +extern int tst_hugetlb_fd;
> >  #define TST_NO_HUGEPAGES ((unsigned long)-1)
> >  
> >  #define TST_UNLIMITED_RUNTIME (-1)
> > @@ -176,6 +176,18 @@ struct tst_test {
> >  	int all_filesystems:1;
> >  	int skip_in_lockdown:1;
> >  	int skip_in_compat:1;
> > +	/*
> > +	 * If set, the test function will create a hugetlbfs mount point
> > +	 * at /tmp/xxxxxx, where xxxxxx is a random string.
> > +	 */
> > +	int needs_hugetlbfs:1;
> > +	/*
> > +	 * If set, the test function will create and open a random file
> > +	 * under mounted hugetlbfs. To use this option, needs_hugetlbfs must
> > +	 * be set. The file descriptior will be set in tst_hugetlb_fd.
> > +	 * The close(tst_hugetlb_fd) will be called on test exit(cleanup).
> > +	 */
> > +	int needs_unlinked_hugetlb_file:1;
> >  
> >  	/*
> >  	 * The skip_filesystems is a NULL terminated list of filesystems the
> > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void);
> >   */
> >  void tst_set_max_runtime(int max_runtime);
> >  
> > +/*
> > + * Create and open a random file inside the given dir path.
> > + * It unlinks the file after opening and return file descriptor.
> > + */
> > +int tst_create_unlinked_file(const char *path);
> > +
> >  /*
> >   * Returns path to the test temporary directory in a newly allocated buffer.
> >   */
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index 8ccde1629..43cba1004 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -925,7 +925,8 @@ static int needs_tmpdir(void)
> >  	       tst_test->needs_device ||
> >  	       tst_test->mntpoint ||
> >  	       tst_test->resource_files ||
> > -	       tst_test->needs_checkpoints;
> > +	       tst_test->needs_checkpoints ||
> > +		   tst_test->needs_hugetlbfs;
> >  }
> >  
> >  static void copy_resources(void)
> > @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char *mntpoint)
> >  	}
> >  }
> >  
> > +static void prepare_and_mount_hugetlb_fs(void)
> > +{
> > +	tst_test->mntpoint = tst_get_tmpdir();
> > +	SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL);
> > +	mntpoint_mounted = 1;
> > +}
> > +
> > +int tst_create_unlinked_file(const char *path)
> > +{
> > +	char template[PATH_MAX];
> > +	int fd;
> > +
> > +	snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX",
> > +			path, TCID);
> > +
> > +	fd = mkstemp(template);
> > +	if (fd < 0)
> > +		tst_brk(TBROK | TERRNO,
> > +			 "%s: mkstemp(%s) failed", __func__, template);
> > +
> > +	SAFE_UNLINK(template);
> > +	return fd;
> > +}
> > +
> >  static const char *limit_tmpfs_mount_size(const char *mnt_data,
> >  		char *buf, size_t buf_size, const char *fs_type)
> >  {
> > @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void)
> >  	tst_cg_init();
> >  }
> >  
> > +int tst_hugetlb_fd = -1;
> > +
> >  static void do_setup(int argc, char *argv[])
> >  {
> >  	if (!tst_test)
> > @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[])
> >  		}
> >  	}
> >  
> > +	if (tst_test->needs_hugetlbfs)
> > +		prepare_and_mount_hugetlb_fs();
> > +
> > +	if (tst_test->needs_unlinked_hugetlb_file) {
> > +		if (!(tst_test->needs_hugetlbfs)) {
> > +			tst_brk(TBROK, "Option needs_unlinked_hugetlb_file "
> > +					"requires option needs_hugetlbfs");
> > +		}
> > +		tst_hugetlb_fd = tst_create_unlinked_file(tst_test->mntpoint);
> > +	}
> 
> The function tst_create_unlinked_file() looks useful, but I do not think
> that adding the needs_unlinked_hugetlb_file flag simplifies things that
> much. Also this will not scale well when we would need two
> filedescripors like that. Maybe we it would be cleaner to add only the
> mount/umount functionality to the test library and call the
> tst_create_unlinked_file() in the test setup in the testcases.
> 
yes I agree, There is a test that requires two such unlinked file, 
there is one that requires one hugetlb and one normal such unlinked file.


> >  	if (tst_test->needs_device && !mntpoint_mounted) {
> >  		tdev.dev = tst_acquire_device_(NULL, tst_test->dev_min_size);
> >  
> > @@ -1299,8 +1337,15 @@ static void do_cleanup(void)
> >  	if (ovl_mounted)
> >  		SAFE_UMOUNT(OVL_MNT);
> >  
> > -	if (mntpoint_mounted)
> > -		tst_umount(tst_test->mntpoint);
> > +	if (tst_hugetlb_fd >= 0)
> > +		SAFE_CLOSE(tst_hugetlb_fd);
> > +
> > +	if (mntpoint_mounted) {
> > +		if (tst_test->needs_hugetlbfs)
> > +			SAFE_UMOUNT(tst_test->mntpoint);
> > +		else
> > +			tst_umount(tst_test->mntpoint);
> > +	}
> 
> Is there a good reason for this, why can't we call tst_umount() for
> hugetlbfs?
> 

My reason behind this is, 
tst_umount doesnt break the test, if umount fails. One of the cause
of failure can be that test left the mounted fs busy, which is not
expected.
> -- 
> Cyril Hrubis
> chrubis@suse.cz
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1245 bytes --]

On Mon, Oct 31, 2022 at 10:54 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > Why not consider encapsulating these two new fields in 'struct
> > tst_hugepage' ?
> >
> > Then the tst_test in the case can simply initialize to:
> >
> > ....
> > static struct tst_test test = {
> >     .needs_root = 1,
> >     .taint_check = TST_TAINT_D | TST_TAINT_W,
> >     .setup = setup,
> >     .test_all = run_test,
> >     .hugepages = {1, TST_NEEDS, 1, 1},
> > };
>
> I do not like that we have magic constants in the .hugepages that are
> not self describing. I would treat the hugetltbfs just as we treat
> devfs, that would be:
>
> #define MNTPOINT "hugetlbfs/"
> #define HUGEFILE MNTPOINT "hugefile"
>
> static int huge_fd;
>
> static void setup(void)
> {
>         huge_fd = tst_creat_unlinked(HUGEFILE);
>         ...
> }
>
> static void cleanup(void)
> {
>         if (huge_fd > 0)
>                 SAFE_CLOSE(huge_fd);
> }
>
> static struct tst_test test = {
>         ...
>         .mntpoint = MNTPOINT,
>         .needs_hugetlbfs = 1,
>         .setup = setup,
>         .cleanup = cleanup,
>         ...
> }
>
>
> What do you think?
>

+1 Looks good, this treats it as an FS and is separated from the hugepage
usage.


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2121 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Cyril Hrubis]

Hi!
> > Is there a good reason for this, why can't we call tst_umount() for
> > hugetlbfs?
> > 
> 
> My reason behind this is, 
> tst_umount doesnt break the test, if umount fails. One of the cause
> of failure can be that test left the mounted fs busy, which is not
> expected.

This does not matter, because the do_cleanup() is called only once at
the end of the test. The test exists once the function does finish
anyways.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp