[LTP] [PATCH v2 0/3] Hugetlb:Migrating the libhugetlbfs tests

[LTP] [PATCH v2 0/3] 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 by Cyril Hrubis and Li Wang[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().

Tarun Sahu (3):
  Hugetlb: Migrating libhugetlbfs brk_near_huge
  Hugetlb: Migrating libhugetlbfs chunk-overcommit
  Hugetlb: Migrating libhugetlbfs corrupt-by-cow-opt

 runtest/hugetlb                               |   3 +
 testcases/kernel/mem/.gitignore               |   3 +
 .../kernel/mem/hugetlb/hugemmap/hugemmap07.c  | 156 ++++++++++++++++
 .../kernel/mem/hugetlb/hugemmap/hugemmap08.c  | 166 ++++++++++++++++++
 .../kernel/mem/hugetlb/hugemmap/hugemmap09.c  |  90 ++++++++++
 testcases/kernel/mem/hugetlb/lib/hugetlb.h    |   3 +
 6 files changed, 421 insertions(+)
 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 v2 1/3] 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/hugemmap07.c  | 156 ++++++++++++++++++
 testcases/kernel/mem/hugetlb/lib/hugetlb.h    |   3 +
 4 files changed, 161 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/hugemmap07.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
new file mode 100644
index 000000000..bd33d9816
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
@@ -0,0 +1,156 @@
+// 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 int  fd = -1;
+static char hfile[MAXPATHLEN];
+static long hpage_size;
+
+static int arch_has_slice_support(void)
+{
+#ifdef __powerpc64__
+	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;
+#elif defined(__powerpc__) && !defined(PPC_NO_SEGMENTS)
+	return 1;
+#else
+	return 0;
+#endif
+}
+
+#ifdef __powerpc64__
+static void *next_chunk(void *addr)
+{
+	if (!arch_has_slice_support())
+		return PALIGN(addr, SAFE_READ_MEMINFO("Hugepagesize:")*1024);
+
+	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, SAFE_READ_MEMINFO("Hugepagesize:")*1024);
+}
+#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, 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 iff 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)
+{
+	if (!Hopt)
+		Hopt = tst_get_tmpdir();
+	SAFE_MOUNT("none", Hopt, "hugetlbfs", 0, NULL);
+
+	snprintf(hfile, sizeof(hfile), "%s/ltp_hugetlbfile%d", Hopt, getpid());
+	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
+
+	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
+	SAFE_UNLINK(hfile);
+}
+
+static void cleanup(void)
+{
+	if (fd >= 0)
+		SAFE_CLOSE(fd);
+	SAFE_UMOUNT(Hopt);
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.options = (struct tst_option[]) {
+		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H /var/hugetlbfs"},
+		{"s:", &nr_opt, "Set the number of the been allocated hugepages"},
+		{}
+	},
+	.setup = setup,
+	.cleanup = cleanup,
+	.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 v2 2/3] 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  | 166 ++++++++++++++++++
 3 files changed, 168 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..2fbda0fcb
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
@@ -0,0 +1,166 @@
+// 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 char hfile[MAXPATHLEN];
+static int fd = -1;
+static long hpage_size;
+
+static void test_chunk_overcommit(void)
+{
+	unsigned long totpages, chunk1, chunk2;
+	void *p, *q;
+	pid_t child;
+	int status;
+
+	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
+	SAFE_UNLINK(hfile);
+
+	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,
+		 fd, 0);
+
+	q = mmap(NULL, chunk2*hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
+		 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_CLOSE(fd);
+}
+
+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)
+{
+	if (!Hopt)
+		Hopt = tst_get_tmpdir();
+	SAFE_MOUNT("none", Hopt, "hugetlbfs", 0, NULL);
+
+	snprintf(hfile, sizeof(hfile), "%s/ltp_huetlbfile%d", Hopt, getpid());
+	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
+}
+
+static void cleanup(void)
+{
+	if (fd >= 0)
+		SAFE_CLOSE(fd);
+	SAFE_UMOUNT(Hopt);
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.forks_child = 1,
+	.options = (struct tst_option[]) {
+		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H /var/hugetlbfs"},
+		{"s:", &nr_opt, "Set the number of the been allocated hugepages"},
+		{}
+	},
+	.save_restore = (const struct tst_path_val[]) {
+		{PROC_OVERCOMMIT, NULL},
+		{}
+	},
+	.tcnt = 2,
+	.setup = setup,
+	.cleanup = cleanup,
+	.test = run_test,
+	.hugepages = {3, TST_NEEDS},
+};
+
-- 
2.31.1


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

[LTP] [PATCH v2 3/3] 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  | 90 +++++++++++++++++++
 3 files changed, 92 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..067556793
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
@@ -0,0 +1,90 @@
+// 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 int  fd = -1;
+static char hfile[MAXPATHLEN];
+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, 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, 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, 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)
+{
+	if (!Hopt)
+		Hopt = tst_get_tmpdir();
+	SAFE_MOUNT("none", Hopt, "hugetlbfs", 0, NULL);
+
+	snprintf(hfile, sizeof(hfile), "%s/ltp_hugetlbfile%d", Hopt, getpid());
+	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
+
+	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
+	SAFE_UNLINK(hfile);
+}
+
+static void cleanup(void)
+{
+	if (fd >= 0)
+		SAFE_CLOSE(fd);
+	SAFE_UMOUNT(Hopt);
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.options = (struct tst_option[]) {
+		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H /var/hugetlbfs"},
+		{"s:", &nr_opt, "Set the number of the been allocated hugepages"},
+		{}
+	},
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = run_test,
+	.hugepages = {2, TST_NEEDS},
+};
-- 
2.31.1


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

Re: [LTP] [PATCH v2 1/3] Hugetlb: Migrating libhugetlbfs brk_near_huge

[Richard Palethorpe]

Hello,

Tarun Sahu <tsahu@linux.ibm.com> writes:

> 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.

The actual test looks good, but it seems that the existing LTP library
functions which you are using need some work. It looks like this test
could be much smaller.

>
> Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> ---
>  runtest/hugetlb                               |   1 +
>  testcases/kernel/mem/.gitignore               |   1 +
>  .../kernel/mem/hugetlb/hugemmap/hugemmap07.c  | 156 ++++++++++++++++++
>  testcases/kernel/mem/hugetlb/lib/hugetlb.h    |   3 +
>  4 files changed, 161 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/hugemmap07.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> new file mode 100644
> index 000000000..bd33d9816
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> @@ -0,0 +1,156 @@
> +// 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 int  fd = -1;
> +static char hfile[MAXPATHLEN];
> +static long hpage_size;
> +
> +static int arch_has_slice_support(void)

This only appears to be used on __powerpc64__ in next_chunk. So it can
be defined in the ifdef for next_chunk.

Otherwise we could get unused function warnings and IMO it is more
readable.

> +{
> +#ifdef __powerpc64__
> +	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;
> +#elif defined(__powerpc__) && !defined(PPC_NO_SEGMENTS)

This elif doesn't appear to be applicable.

> +	return 1;
> +#else
> +	return 0;
> +#endif
> +}
> +
> +#ifdef __powerpc64__
> +static void *next_chunk(void *addr)
> +{
> +	if (!arch_has_slice_support())
> +		return PALIGN(addr,
> SAFE_READ_MEMINFO("Hugepagesize:")*1024);

In setup we set hpage_size, but then keep reading it.

> +
> +	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, SAFE_READ_MEMINFO("Hugepagesize:")*1024);
> +}
> +#endif

If these functions are used in later tests I guess they should go in
tst_hugepages.h

> +
> +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, 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 iff 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)
> +{
> +	if (!Hopt)
> +		Hopt = tst_get_tmpdir();
> +	SAFE_MOUNT("none", Hopt, "hugetlbfs", 0, NULL);
> +
> +	snprintf(hfile, sizeof(hfile), "%s/ltp_hugetlbfile%d", Hopt, getpid());
> +	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
> +
> +	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
> +	SAFE_UNLINK(hfile);

I'm guessing opening this file and using it with mmap is a very common
pattern. If so, it should be encapsulated in tst_hugepage.c.

> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd >= 0)
> +		SAFE_CLOSE(fd);
> +	SAFE_UMOUNT(Hopt);
> +}
> +
> +static struct tst_test test = {
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +	.options = (struct tst_option[]) {
> +		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H /var/hugetlbfs"},

This is a source of confusion. This description suggests we pass in an
existing hugetlb mount. However it's actually where it will be mounted.

Perhaps instead we could use set/getmntent to find an existing hugetlb
mount?  Then if it is not there, try mounting it. This is what we do for
CGroups.

I'm not sure what difference it makes where the FS is mounted
anyway. Why is it even an option?

> +		{"s:", &nr_opt, "Set the number of the been allocated hugepages"},
> +		{}
> +	},
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run_test,
> +	.hugepages = {1, TST_NEEDS},

When we set this tst_hugepages.c could fill Hopt (which should be
something like tst_hugepages_mount) with the location of the mount
point. It could also open the hfile fd and store it in a static
variable like tst_hugepage_fd.

Also .taint_check should be added here to check for the type of Ooops
that are caused by this test. This makes debugging easier if the kernel
doesn't kill the test process or freeze immediately.

> +};
> 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


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v2 3/3] Hugetlb: Migrating libhugetlbfs corrupt-by-cow-opt

[Richard Palethorpe]

Hello,

Tarun Sahu <tsahu@linux.ibm.com> writes:

> 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.

Seems like this and 2/3 follow the same pattern. The setups are
reasonably similar and could be encapsulated in tst_hugepage.

> +
> +static struct tst_test test = {
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +	.options = (struct tst_option[]) {
> +		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H /var/hugetlbfs"},
> +		{"s:", &nr_opt, "Set the number of the been allocated
> hugepages"},

nr_opt also seems suspicious. The test only ever allocates one page at a
time regardless of what this is set to. Changing it will just change how
much free memory we check for unless I am mistaken.

> +		{}
> +	},
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run_test,
> +	.hugepages = {2, TST_NEEDS},
> +};
> -- 
> 2.31.1


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v2 1/3] Hugetlb: Migrating libhugetlbfs brk_near_huge

[Tarun Sahu]

Hi Richard, 
Thanks for reviewing the patch.

-- skip
> > +static int arch_has_slice_support(void)
> 
> This only appears to be used on __powerpc64__ in next_chunk. So it
> can
> be defined in the ifdef for next_chunk.
> 
> Otherwise we could get unused function warnings and IMO it is more
> readable.

Yeah, I will update it in next version.

> 
> > +{
> > +#ifdef __powerpc64__
> > +	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;
> > +#elif defined(__powerpc__) && !defined(PPC_NO_SEGMENTS)
> 
> This elif doesn't appear to be applicable.
I missed adding PPC_NO_SEGMENTS cppflags in Makefile
Will update it in next version.

> 
> > +	return 1;
> > +#else
> > +	return 0;
> > +#endif
> > +}
> > +
> > +#ifdef __powerpc64__
> > +static void *next_chunk(void *addr)
> > +{
> > +	if (!arch_has_slice_support())
> > +		return PALIGN(addr,
> > SAFE_READ_MEMINFO("Hugepagesize:")*1024);
> 
> In setup we set hpage_size, but then keep reading it.
Yeah, I missed it, Will update it.
> 
> > +
> > +	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, SAFE_READ_MEMINFO("Hugepagesize:")*1024);
> > +}
> > +#endif
> 
> If these functions are used in later tests I guess they should go in
> tst_hugepages.h
No, These functions are only used in this test.

> 
> > +	snprintf(hfile, sizeof(hfile), "%s/ltp_hugetlbfile%d", Hopt,
> > getpid());
> > +	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
> > +
> > +	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
> > +	SAFE_UNLINK(hfile);
> 
> I'm guessing opening this file and using it with mmap is a very
> common
> pattern. If so, it should be encapsulated in tst_hugepage.c.
> 
Yeah I agree. But the change in tst_hugepage.c will require change in
prexisting hugetlb tests.

> > +}
> > +
> > +static void cleanup(void)
> > +{
> > +	if (fd >= 0)
> > +		SAFE_CLOSE(fd);
> > +	SAFE_UMOUNT(Hopt);
> > +}
> > +
> > +static struct tst_test test = {
> > +	.needs_root = 1,
> > +	.needs_tmpdir = 1,
> > +	.options = (struct tst_option[]) {
> > +		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H
> > /var/hugetlbfs"},
> 
> This is a source of confusion. This description suggests we pass in
> an
> existing hugetlb mount. However it's actually where it will be
> mounted.
> 
> Perhaps instead we could use set/getmntent to find an existing
> hugetlb
> mount?  Then if it is not there, try mounting it. This is what we do
> for
> CGroups.
> 
> I'm not sure what difference it makes where the FS is mounted
> anyway. Why is it even an option?

Not sure, If user is ok for using premounted fs without their
permission. So instead of searching, it will mount where user will
explicitly asked for. Or otherwise if not provided, it will mount at
temp location under /tmp.

I had taken this option from hugemmap01 test. Thinking, it might be
to provide user the flexibility incase user doesnt want test to mount
fs at random location by default.

I will change the description to "Provide the location to mount the
hugetlbfs or default '/tmp/xxxxxxx'"

> 
> > +		{"s:", &nr_opt, "Set the number of the been allocated
> > hugepages"},
> > +		{}
> > +	},
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.test_all = run_test,
> > +	.hugepages = {1, TST_NEEDS},
> 
> When we set this tst_hugepages.c could fill Hopt (which should be
> something like tst_hugepages_mount) with the location of the mount
> point. It could also open the hfile fd and store it in a static
> variable like tst_hugepage_fd.
Yeah, It will move the repeated actions to single location.

This will
require change in lib/tst_hugepage.c and which will require
change in already existing test under hugetlb/ . Which will be like a
new separate change patch series.

> 
> Also .taint_check should be added here to check for the type of Ooops
> that are caused by this test. This makes debugging easier if the
> kernel
> doesn't kill the test process or freeze immediately.
Ok, Will update it in next version.

> 
> > +};
> > 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

Re: [LTP] [PATCH v2 3/3] Hugetlb: Migrating libhugetlbfs corrupt-by-cow-opt

[Tarun Sahu]

On Tue, 2022-10-25 at 12:04 +0100, Richard Palethorpe wrote:
> Hello,
> 
> Tarun Sahu <tsahu@linux.ibm.com> writes:
> 
> > 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.
> 
> Seems like this and 2/3 follow the same pattern. The setups are
> reasonably similar and could be encapsulated in tst_hugepage.
Do you mean by a encapsulating in a function. and call it from setup.
becuase it will anyway require explicit cleanup.

Or by defining a new field in struct tst_hugepage, if that is true,
that setup will automatically be done in do_setup in tst_test file.
which means it will require change in tst_test.c too. also change in 
do_cleanup in tst_test.c will also be required. 

> 
> > +
> > +static struct tst_test test = {
> > +	.needs_root = 1,
> > +	.needs_tmpdir = 1,
> > +	.options = (struct tst_option[]) {
> > +		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H
> > /var/hugetlbfs"},
> > +		{"s:", &nr_opt, "Set the number of the been allocated
> > hugepages"},
> 
> nr_opt also seems suspicious. The test only ever allocates one page
> at a
> time regardless of what this is set to. Changing it will just change
> how
> much free memory we check for unless I am mistaken.
yes, Will update it and also will check for other test cases if not
required. 

> 
> > +		{}
> > +	},
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.test_all = run_test,
> > +	.hugepages = {2, TST_NEEDS},
> > +};
> > -- 
> > 2.31.1
> 
> 


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

Re: [LTP] [PATCH v2 3/3] Hugetlb: Migrating libhugetlbfs corrupt-by-cow-opt

[Richard Palethorpe]

Hello,

Tarun Sahu <tsahu@linux.ibm.com> writes:

> On Tue, 2022-10-25 at 12:04 +0100, Richard Palethorpe wrote:
>> Hello,
>> 
>> Tarun Sahu <tsahu@linux.ibm.com> writes:
>> 
>> > 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.
>> 
>> Seems like this and 2/3 follow the same pattern. The setups are
>> reasonably similar and could be encapsulated in tst_hugepage.
> Do you mean by a encapsulating in a function. and call it from setup.
> becuase it will anyway require explicit cleanup.
>
> Or by defining a new field in struct tst_hugepage, if that is true,
> that setup will automatically be done in do_setup in tst_test file.
> which means it will require change in tst_test.c too. also change in 
> do_cleanup in tst_test.c will also be required.

Yes, it's a very common pattern, so will probably save a lot of
boilerplate.

>
>> 
>> > +
>> > +static struct tst_test test = {
>> > +	.needs_root = 1,
>> > +	.needs_tmpdir = 1,
>> > +	.options = (struct tst_option[]) {
>> > +		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H
>> > /var/hugetlbfs"},
>> > +		{"s:", &nr_opt, "Set the number of the been allocated
>> > hugepages"},
>> 
>> nr_opt also seems suspicious. The test only ever allocates one page
>> at a
>> time regardless of what this is set to. Changing it will just change
>> how
>> much free memory we check for unless I am mistaken.
> yes, Will update it and also will check for other test cases if not
> required. 
>
>> 
>> > +		{}
>> > +	},
>> > +	.setup = setup,
>> > +	.cleanup = cleanup,
>> > +	.test_all = run_test,
>> > +	.hugepages = {2, TST_NEEDS},
>> > +};
>> > -- 
>> > 2.31.1
>> 
>> 


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v2 1/3] Hugetlb: Migrating libhugetlbfs brk_near_huge

[Richard Palethorpe]

Hello,

Tarun Sahu <tsahu@linux.ibm.com> writes:

>
>> 
>> > +	snprintf(hfile, sizeof(hfile), "%s/ltp_hugetlbfile%d", Hopt,
>> > getpid());
>> > +	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
>> > +
>> > +	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
>> > +	SAFE_UNLINK(hfile);
>> 
>> I'm guessing opening this file and using it with mmap is a very
>> common
>> pattern. If so, it should be encapsulated in tst_hugepage.c.
>> 
> Yeah I agree. But the change in tst_hugepage.c will require change in
> prexisting hugetlb tests.

You don't need to update existing LTP tests if you add a new flag to
.tst_test I guess.

>
>> > +}
>> > +
>> > +static void cleanup(void)
>> > +{
>> > +	if (fd >= 0)
>> > +		SAFE_CLOSE(fd);
>> > +	SAFE_UMOUNT(Hopt);
>> > +}
>> > +
>> > +static struct tst_test test = {
>> > +	.needs_root = 1,
>> > +	.needs_tmpdir = 1,
>> > +	.options = (struct tst_option[]) {
>> > +		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H
>> > /var/hugetlbfs"},
>> 
>> This is a source of confusion. This description suggests we pass in
>> an
>> existing hugetlb mount. However it's actually where it will be
>> mounted.
>> 
>> Perhaps instead we could use set/getmntent to find an existing
>> hugetlb
>> mount?  Then if it is not there, try mounting it. This is what we do
>> for
>> CGroups.
>> 
>> I'm not sure what difference it makes where the FS is mounted
>> anyway. Why is it even an option?
>
> Not sure, If user is ok for using premounted fs without their
> permission. So instead of searching, it will mount where user will
> explicitly asked for. Or otherwise if not provided, it will mount at
> temp location under /tmp.

Does it actually create a new FS or is it just remounting the existing
hugetlb interface in a new place?

Also it requires privileges to mount an FS. It seems unlikely that some
database wanting to use hugepages would mount it themselves.

From the kernel docs:

"If the user applications are going to request huge pages using mmap system
call, then it is required that system administrator mount a file system of
type hugetlbfs"

>
> I had taken this option from hugemmap01 test. Thinking, it might be
> to provide user the flexibility incase user doesnt want test to mount
> fs at random location by default.
>
> I will change the description to "Provide the location to mount the
> hugetlbfs or default '/tmp/xxxxxxx'"
>
>> 
>> > +		{"s:", &nr_opt, "Set the number of the been allocated
>> > hugepages"},
>> > +		{}
>> > +	},
>> > +	.setup = setup,
>> > +	.cleanup = cleanup,
>> > +	.test_all = run_test,
>> > +	.hugepages = {1, TST_NEEDS},
>> 
>> When we set this tst_hugepages.c could fill Hopt (which should be
>> something like tst_hugepages_mount) with the location of the mount
>> point. It could also open the hfile fd and store it in a static
>> variable like tst_hugepage_fd.
> Yeah, It will move the repeated actions to single location.
>
> This will
> require change in lib/tst_hugepage.c and which will require
> change in already existing test under hugetlb/ . Which will be like a
> new separate change patch series.

If it results in too many complicated changes, then converting all the
existing tests could be defered to a later time by creating new
variables.

-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v2 1/3] Hugetlb: Migrating libhugetlbfs brk_near_huge

[Tarun Sahu]

Hello,
> 
> Tarun Sahu <tsahu@linux.ibm.com> writes:
> 
> > > > +	snprintf(hfile, sizeof(hfile), "%s/ltp_hugetlbfile%d",
> > > > Hopt,
> > > > getpid());
> > > > +	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
> > > > +
> > > > +	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
> > > > +	SAFE_UNLINK(hfile);
> > > 
> > > I'm guessing opening this file and using it with mmap is a very
> > > common
> > > pattern. If so, it should be encapsulated in tst_hugepage.c.
> > > 
> > Yeah I agree. But the change in tst_hugepage.c will require change
> > in
> > prexisting hugetlb tests.
> 
> You don't need to update existing LTP tests if you add a new flag to
> .tst_test I guess.
yes.
I will add new tst_test flag option for Mount and open file setup 
in next version.
> 
> > > > +}
> > > > +
> > > > +static void cleanup(void)
> > > > +{
> > > > +	if (fd >= 0)
> > > > +		SAFE_CLOSE(fd);
> > > > +	SAFE_UMOUNT(Hopt);
> > > > +}
> > > > +
> > > > +static struct tst_test test = {
> > > > +	.needs_root = 1,
> > > > +	.needs_tmpdir = 1,
> > > > +	.options = (struct tst_option[]) {
> > > > +		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -
> > > > H
> > > > /var/hugetlbfs"},
> > > 
> > > This is a source of confusion. This description suggests we pass
> > > in
> > > an
> > > existing hugetlb mount. However it's actually where it will be
> > > mounted.
> > > 
> > > Perhaps instead we could use set/getmntent to find an existing
> > > hugetlb
> > > mount?  Then if it is not there, try mounting it. This is what we
> > > do
> > > for
> > > CGroups.
> > > 
> > > I'm not sure what difference it makes where the FS is mounted
> > > anyway. Why is it even an option?
> > 
> > Not sure, If user is ok for using premounted fs without their
> > permission. So instead of searching, it will mount where user will
> > explicitly asked for. Or otherwise if not provided, it will mount
> > at
> > temp location under /tmp.
> 
> Does it actually create a new FS or is it just remounting the
> existing
> hugetlb interface in a new place?
> 
> Also it requires privileges to mount an FS. It seems unlikely that
> some
> database wanting to use hugepages would mount it themselves.
> 
> From the kernel docs:
> 
> "If the user applications are going to request huge pages using mmap
> system
> call, then it is required that system administrator mount a file
> system of
> type hugetlbfs"

I agree.
But this will make Hopt option compulsory for user to provide.
and during batch running of tests (/opt/ltp/runltp -f hugetlb), user
will have to modify each test line in /opt/ltp/runtest to provide the
particular mounted option to get the tests running.

Instead, I propose, to follow the same mechanism as the older hugetlb
test follow, create a random temp location under /tmp (/tmp/xxxxxx),
and mount and umount hugetlbfs as needed. For this way, I will also
remove the unnecessary Hopt option. Though need_root will become 
pre-requisite to run the test.

> 
> > I had taken this option from hugemmap01 test. Thinking, it might be
> > to provide user the flexibility incase user doesnt want test to
> > mount
> > fs at random location by default.
> > 
> > I will change the description to "Provide the location to mount the
> > hugetlbfs or default '/tmp/xxxxxxx'"
> > 
> > > > +		{"s:", &nr_opt, "Set the number of the been
> > > > allocated
> > > > hugepages"},
> > > > +		{}
> > > > +	},
> > > > +	.setup = setup,
> > > > +	.cleanup = cleanup,
> > > > +	.test_all = run_test,
> > > > +	.hugepages = {1, TST_NEEDS},
> > > 
> > > When we set this tst_hugepages.c could fill Hopt (which should be
> > > something like tst_hugepages_mount) with the location of the
> > > mount
> > > point. It could also open the hfile fd and store it in a static
> > > variable like tst_hugepage_fd.
> > Yeah, It will move the repeated actions to single location.
> > 
> > This will
> > require change in lib/tst_hugepage.c and which will require
> > change in already existing test under hugetlb/ . Which will be like
> > a
> > new separate change patch series.
> 
> If it results in too many complicated changes, then converting all
> the
> existing tests could be defered to a later time by creating new
> variables.
Ok.
> 


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