* [LTP] [RFC PATCH 2/2] hugemmap33: Test to detect bug with migrating gigantic pages
2023-05-24 9:39 [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation Li Wang
@ 2023-05-24 9:39 ` Li Wang
2023-09-07 11:30 ` Cyril Hrubis
2023-09-07 9:26 ` [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation Cyril Hrubis
2023-09-11 8:02 ` [LTP] [PATCH v2 " Li Wang
2 siblings, 1 reply; 16+ messages in thread
From: Li Wang @ 2023-05-24 9:39 UTC (permalink / raw)
To: ltp
Signed-off-by: Li Wang <liwang@redhat.com>
---
runtest/hugetlb | 1 +
testcases/kernel/mem/.gitignore | 1 +
.../kernel/mem/hugetlb/hugemmap/hugemmap33.c | 143 ++++++++++++++++++
3 files changed, 145 insertions(+)
create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
diff --git a/runtest/hugetlb b/runtest/hugetlb
index 299c07ac9..3576e063d 100644
--- a/runtest/hugetlb
+++ b/runtest/hugetlb
@@ -35,6 +35,7 @@ hugemmap29 hugemmap29
hugemmap30 hugemmap30
hugemmap31 hugemmap31
hugemmap32 hugemmap32
+hugemmap32 hugemmap33
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 7258489ed..d130d4dcd 100644
--- a/testcases/kernel/mem/.gitignore
+++ b/testcases/kernel/mem/.gitignore
@@ -34,6 +34,7 @@
/hugetlb/hugemmap/hugemmap30
/hugetlb/hugemmap/hugemmap31
/hugetlb/hugemmap/hugemmap32
+/hugetlb/hugemmap/hugemmap33
/hugetlb/hugeshmat/hugeshmat01
/hugetlb/hugeshmat/hugeshmat02
/hugetlb/hugeshmat/hugeshmat03
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
new file mode 100644
index 000000000..12a470193
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) Linux Test Project, 2023
+ * Copyright (C) 2023, Red Hat, Inc.
+ *
+ * Author: David Hildenbrand <david@redhat.com>
+ * Port-to-LTP: Li Wang <liwang@redhat.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Migration code will first unmap the old page and replace the present PTE
+ * by a migration entry. Then migrate the page. Once that succeeded (and there
+ * are no unexpected page references), we replace the migration entries by
+ * proper present PTEs pointing at the new page.
+ *
+ * For ordinary pages we handle PTEs. For 2 MiB hugetlb/THP, it's PMDs.
+ * For 1 GiB hugetlb, it's PUDs.
+ *
+ * So without below commit, GUP-fast code was simply not aware that we could
+ * have migration entries stored in PUDs. Migration + GUP-fast code should be
+ * able to handle any such races.
+ *
+ * For example, GUP-fast will re-verify the PUD after pinning to make sure it
+ * didn't change. If it did change, it backs off.
+ *
+ * Migration code should detect the additional page references and back off
+ * as well.
+ *
+ * commit 15494520b776aa2eadc3e2fefae524764cab9cea
+ * Author: Qiujun Huang <hqjagain@gmail.com>
+ * Date: Thu Jan 30 22:12:10 2020 -0800
+ *
+ * mm: fix gup_pud_range
+ *
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <stdbool.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <sys/ioctl.h>
+#include <linux/mempolicy.h>
+#include <linux/mman.h>
+
+#include "lapi/syscalls.h"
+#include "numa_helper.h"
+#include "hugetlb.h"
+#if HAVE_NUMA_H
+#include <numa.h>
+#endif
+
+static char *mem;
+static size_t pagesize;
+static size_t hugetlbsize;
+static volatile int looping = 1;
+
+static void *migration_thread_fn(void *arg LTP_ATTRIBUTE_UNUSED)
+{
+ while (looping) {
+ TST_EXP_PASS_SILENT(syscall(__NR_mbind, mem, hugetlbsize,
+ MPOL_LOCAL, NULL, 0x7fful, MPOL_MF_MOVE));
+ }
+
+ return NULL;
+}
+
+static void run_test(void)
+{
+ ssize_t transferred;
+ struct iovec iov;
+ int fds[2];
+
+ pthread_t migration_thread;
+
+ if (!is_numa(NULL, NH_MEMS, 1))
+ tst_brk(TCONF, "requires NUMA with at least 1 node");
+
+ pagesize = getpagesize();
+ hugetlbsize = 1 * 1024 * 1024 * 1024u;
+
+ mem = mmap(NULL, hugetlbsize, PROT_READ|PROT_WRITE,
+ MAP_PRIVATE|MAP_ANON|MAP_HUGETLB|MAP_HUGE_1GB,
+ -1, 0);
+ if (mem == MAP_FAILED)
+ tst_brk(TBROK | TERRNO, "mmap() failed");
+
+ memset(mem, 1, hugetlbsize);
+
+ /* Keep migrating the page around ... */
+ TEST(pthread_create(&migration_thread, NULL, migration_thread_fn, NULL));
+ if (TST_RET)
+ tst_brk(TBROK | TRERRNO, "pthread_create failed");
+
+ while (looping) {
+ if (pipe(fds) < 0)
+ tst_brk(TBROK | TERRNO, "pipe() failed");
+
+ iov.iov_base = mem;
+ iov.iov_len = pagesize;
+ transferred = vmsplice(fds[1], &iov, 1, 0);
+ if (transferred <= 0)
+ tst_brk(TBROK | TERRNO, "vmsplice() failed");
+
+ close(fds[0]);
+ close(fds[1]);
+
+ if (!tst_remaining_runtime()) {
+ tst_res(TINFO, "Runtime exhausted, exiting");
+ looping = 0;
+ }
+ }
+
+ TEST(pthread_join(migration_thread, NULL));
+ if (TST_RET)
+ tst_brk(TBROK | TRERRNO, "pthread_join failed");
+
+ if (tst_taint_check())
+ tst_res(TFAIL, "Test resulted in kernel tainted");
+ else
+ tst_res(TPASS, "Test completed successfully");
+}
+
+static struct tst_test test = {
+ .needs_root = 1,
+ .test_all = run_test,
+ .max_runtime = 60,
+ .hugepages = {2, TST_NEEDS, 1048576},
+ .taint_check = TST_TAINT_W | TST_TAINT_D,
+ .tags = (struct tst_tag[]) {
+ {"linux-git", "15494520b776"},
+ {}
+ },
+ .supported_archs = (const char *const []) {
+ "x86",
+ "x86_64",
+ NULL
+ },
+};
--
2.40.0
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [LTP] [RFC PATCH 2/2] hugemmap33: Test to detect bug with migrating gigantic pages
2023-05-24 9:39 ` [LTP] [RFC PATCH 2/2] hugemmap33: Test to detect bug with migrating gigantic pages Li Wang
@ 2023-09-07 11:30 ` Cyril Hrubis
2023-09-11 7:47 ` Li Wang
0 siblings, 1 reply; 16+ messages in thread
From: Cyril Hrubis @ 2023-09-07 11:30 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
On Wed, May 24, 2023 at 05:39:30PM +0800, Li Wang wrote:
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
> runtest/hugetlb | 1 +
> testcases/kernel/mem/.gitignore | 1 +
> .../kernel/mem/hugetlb/hugemmap/hugemmap33.c | 143 ++++++++++++++++++
> 3 files changed, 145 insertions(+)
> create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
>
> diff --git a/runtest/hugetlb b/runtest/hugetlb
> index 299c07ac9..3576e063d 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -35,6 +35,7 @@ hugemmap29 hugemmap29
> hugemmap30 hugemmap30
> hugemmap31 hugemmap31
> hugemmap32 hugemmap32
> +hugemmap32 hugemmap33
> 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 7258489ed..d130d4dcd 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -34,6 +34,7 @@
> /hugetlb/hugemmap/hugemmap30
> /hugetlb/hugemmap/hugemmap31
> /hugetlb/hugemmap/hugemmap32
> +/hugetlb/hugemmap/hugemmap33
> /hugetlb/hugeshmat/hugeshmat01
> /hugetlb/hugeshmat/hugeshmat02
> /hugetlb/hugeshmat/hugeshmat03
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
> new file mode 100644
> index 000000000..12a470193
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) Linux Test Project, 2023
> + * Copyright (C) 2023, Red Hat, Inc.
> + *
> + * Author: David Hildenbrand <david@redhat.com>
> + * Port-to-LTP: Li Wang <liwang@redhat.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Migration code will first unmap the old page and replace the present PTE
> + * by a migration entry. Then migrate the page. Once that succeeded (and there
> + * are no unexpected page references), we replace the migration entries by
> + * proper present PTEs pointing at the new page.
> + *
> + * For ordinary pages we handle PTEs. For 2 MiB hugetlb/THP, it's PMDs.
> + * For 1 GiB hugetlb, it's PUDs.
> + *
> + * So without below commit, GUP-fast code was simply not aware that we could
> + * have migration entries stored in PUDs. Migration + GUP-fast code should be
> + * able to handle any such races.
> + *
> + * For example, GUP-fast will re-verify the PUD after pinning to make sure it
> + * didn't change. If it did change, it backs off.
> + *
> + * Migration code should detect the additional page references and back off
> + * as well.
> + *
> + * commit 15494520b776aa2eadc3e2fefae524764cab9cea
> + * Author: Qiujun Huang <hqjagain@gmail.com>
> + * Date: Thu Jan 30 22:12:10 2020 -0800
> + *
> + * mm: fix gup_pud_range
> + *
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <pthread.h>
> +#include <unistd.h>
> +#include <stdbool.h>
> +#include <sys/mman.h>
> +#include <sys/syscall.h>
> +#include <sys/ioctl.h>
> +#include <linux/mempolicy.h>
> +#include <linux/mman.h>
> +
> +#include "lapi/syscalls.h"
> +#include "numa_helper.h"
> +#include "hugetlb.h"
> +#if HAVE_NUMA_H
> +#include <numa.h>
> +#endif
This is a bit strange, do we actually use anything from numa.h?
Because:
- if we do it should be ifdefed as well, otherwise the test will not
build without libnuma-devel
- if we do not, why do we include that header in the first place
> +static char *mem;
> +static size_t pagesize;
> +static size_t hugetlbsize;
> +static volatile int looping = 1;
> +
> +static void *migration_thread_fn(void *arg LTP_ATTRIBUTE_UNUSED)
> +{
> + while (looping) {
> + TST_EXP_PASS_SILENT(syscall(__NR_mbind, mem, hugetlbsize,
> + MPOL_LOCAL, NULL, 0x7fful, MPOL_MF_MOVE));
> + }
> +
> + return NULL;
> +}
> +
> +static void run_test(void)
> +{
> + ssize_t transferred;
> + struct iovec iov;
> + int fds[2];
> +
> + pthread_t migration_thread;
> +
> + if (!is_numa(NULL, NH_MEMS, 1))
> + tst_brk(TCONF, "requires NUMA with at least 1 node");
I guess that everything as at least 1 numa node, unless CONFIG_NUMA is
off, so I wonder if we just need needs_kconfig CONFIG_NUMA=y instead.
> + pagesize = getpagesize();
> + hugetlbsize = 1 * 1024 * 1024 * 1024u;
> +
> + mem = mmap(NULL, hugetlbsize, PROT_READ|PROT_WRITE,
> + MAP_PRIVATE|MAP_ANON|MAP_HUGETLB|MAP_HUGE_1GB,
> + -1, 0);
> + if (mem == MAP_FAILED)
> + tst_brk(TBROK | TERRNO, "mmap() failed");
> +
> + memset(mem, 1, hugetlbsize);
> +
> + /* Keep migrating the page around ... */
> + TEST(pthread_create(&migration_thread, NULL, migration_thread_fn, NULL));
> + if (TST_RET)
> + tst_brk(TBROK | TRERRNO, "pthread_create failed");
We do have SAFE_PTHREAD_CREATE()
> + while (looping) {
> + if (pipe(fds) < 0)
> + tst_brk(TBROK | TERRNO, "pipe() failed");
SAFE_PIPE()
> + iov.iov_base = mem;
> + iov.iov_len = pagesize;
> + transferred = vmsplice(fds[1], &iov, 1, 0);
> + if (transferred <= 0)
> + tst_brk(TBROK | TERRNO, "vmsplice() failed");
> +
> + close(fds[0]);
> + close(fds[1]);
SAFE_CLOSE() ?
> + if (!tst_remaining_runtime()) {
> + tst_res(TINFO, "Runtime exhausted, exiting");
> + looping = 0;
> + }
> + }
> +
> + TEST(pthread_join(migration_thread, NULL));
> + if (TST_RET)
> + tst_brk(TBROK | TRERRNO, "pthread_join failed");
SAFE_PTHREAD_...()
> + if (tst_taint_check())
> + tst_res(TFAIL, "Test resulted in kernel tainted");
> + else
> + tst_res(TPASS, "Test completed successfully");
tst_test.taint_check ?
> +}
> +
> +static struct tst_test test = {
> + .needs_root = 1,
> + .test_all = run_test,
> + .max_runtime = 60,
> + .hugepages = {2, TST_NEEDS, 1048576},
^
So this is 1GB hugepage?
I wonder if it would be bettter to
define constants for these, or at
least write it as 1024 * 1024
> + .taint_check = TST_TAINT_W | TST_TAINT_D,
> + .tags = (struct tst_tag[]) {
> + {"linux-git", "15494520b776"},
> + {}
> + },
> + .supported_archs = (const char *const []) {
> + "x86",
> + "x86_64",
> + NULL
I do not see anything x86 specific in the test code, or did I miss
something?
I guess that we can leave the test enabled for all architectures as long
as we check for the existente of the hugepages of the requested size in
the library.
> + },
> +};
> --
> 2.40.0
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [LTP] [RFC PATCH 2/2] hugemmap33: Test to detect bug with migrating gigantic pages
2023-09-07 11:30 ` Cyril Hrubis
@ 2023-09-11 7:47 ` Li Wang
2023-09-11 8:11 ` Cyril Hrubis
0 siblings, 1 reply; 16+ messages in thread
From: Li Wang @ 2023-09-11 7:47 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp
Cyril Hrubis <chrubis@suse.cz> wrote:
>
> > + if (tst_taint_check())
> > + tst_res(TFAIL, "Test resulted in kernel tainted");
> > + else
> > + tst_res(TPASS, "Test completed successfully");
>
> tst_test.taint_check ?
>
We have to keep these check sentences otherwise it will
complain with no reporting results.
tst_hugepage.c:103: TINFO: 2 hugepage(s) reserved
tst_test.c:1562: TINFO: Timeout per run is 0h 01m 30s
hugemmap34.c:107: TINFO: Runtime exhausted, exiting
tst_test.c:1394: TBROK: Test haven't reported results!
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [LTP] [RFC PATCH 2/2] hugemmap33: Test to detect bug with migrating gigantic pages
2023-09-11 7:47 ` Li Wang
@ 2023-09-11 8:11 ` Cyril Hrubis
2023-09-11 8:25 ` Li Wang
0 siblings, 1 reply; 16+ messages in thread
From: Cyril Hrubis @ 2023-09-11 8:11 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
Hi!
> > > + if (tst_taint_check())
> > > + tst_res(TFAIL, "Test resulted in kernel tainted");
> > > + else
> > > + tst_res(TPASS, "Test completed successfully");
> >
> > tst_test.taint_check ?
> >
>
> We have to keep these check sentences otherwise it will
> complain with no reporting results.
>
> tst_hugepage.c:103: TINFO: 2 hugepage(s) reserved
> tst_test.c:1562: TINFO: Timeout per run is 0h 01m 30s
> hugemmap34.c:107: TINFO: Runtime exhausted, exiting
> tst_test.c:1394: TBROK: Test haven't reported results!
Right the .taint_check only produces TFAIL when kernel taint flags
change during the duration of the test.
We can as well do the same as we do in most of the CVE tests that have
this line at the end of the run() function:
tst_res(TPASS, "Nothing bad happened, probably.");
Or we can keep the tst_taint_check() in the code, but I guess that we
are missing tst_taint_init() in the test setup(), or am I mistaken?
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [LTP] [RFC PATCH 2/2] hugemmap33: Test to detect bug with migrating gigantic pages
2023-09-11 8:11 ` Cyril Hrubis
@ 2023-09-11 8:25 ` Li Wang
0 siblings, 0 replies; 16+ messages in thread
From: Li Wang @ 2023-09-11 8:25 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp
On Mon, Sep 11, 2023 at 4:11 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> Hi!
> > > > + if (tst_taint_check())
> > > > + tst_res(TFAIL, "Test resulted in kernel tainted");
> > > > + else
> > > > + tst_res(TPASS, "Test completed successfully");
> > >
> > > tst_test.taint_check ?
> > >
> >
> > We have to keep these check sentences otherwise it will
> > complain with no reporting results.
> >
> > tst_hugepage.c:103: TINFO: 2 hugepage(s) reserved
> > tst_test.c:1562: TINFO: Timeout per run is 0h 01m 30s
> > hugemmap34.c:107: TINFO: Runtime exhausted, exiting
> > tst_test.c:1394: TBROK: Test haven't reported results!
>
> Right the .taint_check only produces TFAIL when kernel taint flags
> change during the duration of the test.
>
> We can as well do the same as we do in most of the CVE tests that have
> this line at the end of the run() function:
>
> tst_res(TPASS, "Nothing bad happened, probably.");
>
+1 this is better, but I have sent out the patch-v2, someone who
merge patch can help improve this.
> Or we can keep the tst_taint_check() in the code, but I guess that we
> are missing tst_taint_init() in the test setup(), or am I mistaken?
>
No need, tst_taint_init() has been invoked in do_setup in tst_test.c.
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation
2023-05-24 9:39 [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation Li Wang
2023-05-24 9:39 ` [LTP] [RFC PATCH 2/2] hugemmap33: Test to detect bug with migrating gigantic pages Li Wang
@ 2023-09-07 9:26 ` Cyril Hrubis
2023-09-07 12:37 ` Li Wang
2023-09-11 8:02 ` [LTP] [PATCH v2 " Li Wang
2 siblings, 1 reply; 16+ messages in thread
From: Cyril Hrubis @ 2023-09-07 9:26 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
Hi!
> Typically when we make use of huge page via LTP library, .hugepages choose
> the default hugepage size, but this can not satisfy all scenarios.
>
> So this patch introduces applying a specified hpsize huge page for user.
>
> There is nothing that needs to change for the existing test cases which
> already using .hugepages, it only needs to fill one more field in the
> structure of .hugepages if a different hpsize is required.
>
> e.g.
>
> static struct tst_test test = {
> .needs_root = 1,
> ...
> .hugepages = {2, TST_NEEDS, 1048576},
> };
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
> doc/c-test-api.txt | 37 +++++++++++++++++++++++++++++++++++--
> include/tst_hugepage.h | 1 +
> lib/tst_hugepage.c | 23 +++++++++++++++++------
> 3 files changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index dcb6e1ba8..45610474c 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -2034,9 +2034,15 @@ For full documentation see the comments in 'include/tst_fuzzy_sync.h'.
> ~~~~~~~~~~~~~~~~~~~~~~~~
>
> Many of the LTP tests need to use hugepage in their testing, this allows the
> -test can reserve hugepages from system via '.hugepages = {xx, TST_REQUEST}'.
> +test can reserve specify size of hugepages from system via:
> + '.hugepages = {xx, TST_REQUEST, yy}'.
>
> -We achieved two policies for reserving hugepages:
> +xx: This is used to set how many pages we wanted.
> +
> +yy: This is used to specify the size of the hugepage.
> + (If not set it will use the default hugepage size)
> +
> +Two policies for reserving hugepages:
>
> TST_REQUEST:
> It will try the best to reserve available huge pages and return the number
> @@ -2103,6 +2109,33 @@ struct tst_test test = {
> };
> -------------------------------------------------------------------------------
>
> +or,
> +
> +[source,c]
> +-------------------------------------------------------------------------------
> +#include "tst_test.h"
> +
> +static void run(void)
> +{
> + ...
> +}
> +
> +static void setup(void)
> +{
> + /*
> + * Specify hpsize reserved automatically in the library
> + * $ echo 2 > /sys/kernel/mm//hugepages/hugepages-1048576kB/nr_hugepages
> + * Do check if 2 hpages are reserved correctly in there
> + */
> +}
> +
> +struct tst_test test = {
> + .test_all = run,
> + .hugepages = {2, TST_NEEDS, 1048576},
> + ...
> +};
> +-------------------------------------------------------------------------------
> +
> 1.35 Checking for required commands
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/include/tst_hugepage.h b/include/tst_hugepage.h
> index 46327c79a..e96bfbe53 100644
> --- a/include/tst_hugepage.h
> +++ b/include/tst_hugepage.h
> @@ -27,6 +27,7 @@ enum tst_hp_policy {
> struct tst_hugepage {
> const unsigned long number;
> enum tst_hp_policy policy;
> + const unsigned long hpsize;
> };
>
> /*
> diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
> index 728a8c3ec..b6706d412 100644
> --- a/lib/tst_hugepage.c
> +++ b/lib/tst_hugepage.c
> @@ -5,6 +5,7 @@
>
> #define TST_NO_DEFAULT_MAIN
>
> +#include <stdio.h>
> #include "tst_test.h"
> #include "tst_hugepage.h"
>
> @@ -22,9 +23,10 @@ size_t tst_get_hugepage_size(void)
>
> unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
> {
> - unsigned long val, max_hpages;
> + unsigned long val, max_hpages, hpsize;
> + char hugepage_path[PATH_MAX];
> struct tst_path_val pvl = {
> - .path = PATH_NR_HPAGES,
> + .path = hugepage_path,
This pointer (hugepage_path) is on stack and is passed to a function
that stores the structure into a list and then uses it to restore the
value at the end of the testrun, which will probably crash the test
since there will be random leftovers on the stack.
The array has to be static to have unlimited lifetime.
> .val = NULL,
> .flags = TST_SR_SKIP_MISSING | TST_SR_TCONF_RO
> };
>
> @@ -41,6 +43,13 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
> else
> tst_hugepages = hp->number;
>
> + if (hp->hpsize)
> + hpsize = hp->hpsize;
> + else
> + hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE);
> +
> + sprintf(hugepage_path, PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize);
Do we check anywhere here if the path actually exists? There is no
guarantee that specific hugepage size will exist on the system, so I
suppose that we have to do:
if (access(hugepage_path, F_OK)) {
if (hp->policy == TST_NEEDS)
tst_brk(TCONF, "Hugepage size %lu not supported", hpsize);
tst_hugepages = 0;
goto out;
}
> if (hp->number == TST_NO_HUGEPAGES) {
> tst_hugepages = 0;
> goto set_hugepages;
> @@ -52,7 +61,7 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
> goto set_hugepages;
> }
>
> - max_hpages = SAFE_READ_MEMINFO("MemFree:") / SAFE_READ_MEMINFO("Hugepagesize:");
> + max_hpages = SAFE_READ_MEMINFO("MemFree:") / hpsize;
> if (tst_hugepages > max_hpages) {
> tst_res(TINFO, "Requested number(%lu) of hugepages is too large, "
> "limiting to 80%% of the max hugepage count %lu",
> @@ -65,14 +74,16 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
>
> set_hugepages:
> tst_sys_conf_save(&pvl);
> - SAFE_FILE_PRINTF(PATH_NR_HPAGES, "%lu", tst_hugepages);
> - SAFE_FILE_SCANF(PATH_NR_HPAGES, "%lu", &val);
> +
> + SAFE_FILE_PRINTF(hugepage_path, "%lu", tst_hugepages);
> + SAFE_FILE_SCANF(hugepage_path, "%lu", &val);
> +
> if (val != tst_hugepages)
> tst_brk(TCONF, "nr_hugepages = %lu, but expect %lu. "
> "Not enough hugepages for testing.",
> val, tst_hugepages);
>
> - if (hp->policy == TST_NEEDS) {
> + if ((hp->policy == TST_NEEDS) && (!hp->hpsize)) {
This branch shouldn't be disabled for TST_NEEDS case, shouldn't there be
HugePages_Free-$(size)kB ?
> unsigned long free_hpages = SAFE_READ_MEMINFO("HugePages_Free:");
> if (hp->number > free_hpages)
> tst_brk(TCONF, "free_hpages = %lu, but expect %lu. "
> --
> 2.40.0
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation
2023-09-07 9:26 ` [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation Cyril Hrubis
@ 2023-09-07 12:37 ` Li Wang
2023-09-07 12:52 ` Li Wang
2023-09-07 14:10 ` Cyril Hrubis
0 siblings, 2 replies; 16+ messages in thread
From: Li Wang @ 2023-09-07 12:37 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp
Hi Cyril,
On Thu, Sep 7, 2023 at 5:25 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> Hi!
> > Typically when we make use of huge page via LTP library, .hugepages
> choose
> > the default hugepage size, but this can not satisfy all scenarios.
> >
> > So this patch introduces applying a specified hpsize huge page for user.
> >
> > There is nothing that needs to change for the existing test cases which
> > already using .hugepages, it only needs to fill one more field in the
> > structure of .hugepages if a different hpsize is required.
> >
> > e.g.
> >
> > static struct tst_test test = {
> > .needs_root = 1,
> > ...
> > .hugepages = {2, TST_NEEDS, 1048576},
> > };
> >
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > ---
> > doc/c-test-api.txt | 37 +++++++++++++++++++++++++++++++++++--
> > include/tst_hugepage.h | 1 +
> > lib/tst_hugepage.c | 23 +++++++++++++++++------
> > 3 files changed, 53 insertions(+), 8 deletions(-)
> >
> > diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> > index dcb6e1ba8..45610474c 100644
> > --- a/doc/c-test-api.txt
> > +++ b/doc/c-test-api.txt
> > @@ -2034,9 +2034,15 @@ For full documentation see the comments in
> 'include/tst_fuzzy_sync.h'.
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Many of the LTP tests need to use hugepage in their testing, this
> allows the
> > -test can reserve hugepages from system via '.hugepages = {xx,
> TST_REQUEST}'.
> > +test can reserve specify size of hugepages from system via:
> > + '.hugepages = {xx, TST_REQUEST, yy}'.
> >
> > -We achieved two policies for reserving hugepages:
> > +xx: This is used to set how many pages we wanted.
> > +
> > +yy: This is used to specify the size of the hugepage.
> > + (If not set it will use the default hugepage size)
> > +
> > +Two policies for reserving hugepages:
> >
> > TST_REQUEST:
> > It will try the best to reserve available huge pages and return the
> number
> > @@ -2103,6 +2109,33 @@ struct tst_test test = {
> > };
> >
> -------------------------------------------------------------------------------
> >
> > +or,
> > +
> > +[source,c]
> >
> +-------------------------------------------------------------------------------
> > +#include "tst_test.h"
> > +
> > +static void run(void)
> > +{
> > + ...
> > +}
> > +
> > +static void setup(void)
> > +{
> > + /*
> > + * Specify hpsize reserved automatically in the library
> > + * $ echo 2 >
> /sys/kernel/mm//hugepages/hugepages-1048576kB/nr_hugepages
> > + * Do check if 2 hpages are reserved correctly in there
> > + */
> > +}
> > +
> > +struct tst_test test = {
> > + .test_all = run,
> > + .hugepages = {2, TST_NEEDS, 1048576},
> > + ...
> > +};
> >
> +-------------------------------------------------------------------------------
> > +
> > 1.35 Checking for required commands
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > diff --git a/include/tst_hugepage.h b/include/tst_hugepage.h
> > index 46327c79a..e96bfbe53 100644
> > --- a/include/tst_hugepage.h
> > +++ b/include/tst_hugepage.h
> > @@ -27,6 +27,7 @@ enum tst_hp_policy {
> > struct tst_hugepage {
> > const unsigned long number;
> > enum tst_hp_policy policy;
> > + const unsigned long hpsize;
> > };
> >
> > /*
> > diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
> > index 728a8c3ec..b6706d412 100644
> > --- a/lib/tst_hugepage.c
> > +++ b/lib/tst_hugepage.c
> > @@ -5,6 +5,7 @@
> >
> > #define TST_NO_DEFAULT_MAIN
> >
> > +#include <stdio.h>
> > #include "tst_test.h"
> > #include "tst_hugepage.h"
> >
> > @@ -22,9 +23,10 @@ size_t tst_get_hugepage_size(void)
> >
> > unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
> > {
> > - unsigned long val, max_hpages;
> > + unsigned long val, max_hpages, hpsize;
> > + char hugepage_path[PATH_MAX];
> > struct tst_path_val pvl = {
> > - .path = PATH_NR_HPAGES,
> > + .path = hugepage_path,
>
> This pointer (hugepage_path) is on stack and is passed to a function
> that stores the structure into a list and then uses it to restore the
> value at the end of the testrun, which will probably crash the test
> since there will be random leftovers on the stack.
>
But the function tst_sys_conf_save_str allocates a new memory
area for saving the structure info, the original is not used for
appending to the list, isn't it?
strncpy() inside that function helps avoid the problem you pointed.
>
> The array has to be static to have unlimited lifetime.
>
> > .val = NULL,
> > .flags = TST_SR_SKIP_MISSING | TST_SR_TCONF_RO
> > };
> >
> > @@ -41,6 +43,13 @@ unsigned long tst_reserve_hugepages(struct
> tst_hugepage *hp)
> > else
> > tst_hugepages = hp->number;
> >
> > + if (hp->hpsize)
> > + hpsize = hp->hpsize;
> > + else
> > + hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE);
> > +
> > + sprintf(hugepage_path,
> PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize);
>
> Do we check anywhere here if the path actually exists? There is no
> guarantee that specific hugepage size will exist on the system, so I
> suppose that we have to do:
>
+1 good advice!
I previously thought to use .arch to guarantee that a specific path exists
but it obviously a stupid way in patch2/2.
> if (access(hugepage_path, F_OK)) {
> if (hp->policy == TST_NEEDS)
> tst_brk(TCONF, "Hugepage size %lu not supported",
> hpsize);
> tst_hugepages = 0;
> goto out;
> }
>
> > if (hp->number == TST_NO_HUGEPAGES) {
> > tst_hugepages = 0;
> > goto set_hugepages;
> > @@ -52,7 +61,7 @@ unsigned long tst_reserve_hugepages(struct
> tst_hugepage *hp)
> > goto set_hugepages;
> > }
> >
> > - max_hpages = SAFE_READ_MEMINFO("MemFree:") /
> SAFE_READ_MEMINFO("Hugepagesize:");
> > + max_hpages = SAFE_READ_MEMINFO("MemFree:") / hpsize;
> > if (tst_hugepages > max_hpages) {
> > tst_res(TINFO, "Requested number(%lu) of hugepages is too
> large, "
> > "limiting to 80%% of the max hugepage
> count %lu",
> > @@ -65,14 +74,16 @@ unsigned long tst_reserve_hugepages(struct
> tst_hugepage *hp)
> >
> > set_hugepages:
> > tst_sys_conf_save(&pvl);
> > - SAFE_FILE_PRINTF(PATH_NR_HPAGES, "%lu", tst_hugepages);
> > - SAFE_FILE_SCANF(PATH_NR_HPAGES, "%lu", &val);
> > +
> > + SAFE_FILE_PRINTF(hugepage_path, "%lu", tst_hugepages);
> > + SAFE_FILE_SCANF(hugepage_path, "%lu", &val);
> > +
> > if (val != tst_hugepages)
> > tst_brk(TCONF, "nr_hugepages = %lu, but expect %lu. "
> > "Not enough hugepages for testing.",
> > val, tst_hugepages);
> >
> > - if (hp->policy == TST_NEEDS) {
> > + if ((hp->policy == TST_NEEDS) && (!hp->hpsize)) {
>
> This branch shouldn't be disabled for TST_NEEDS case, shouldn't there be
> HugePages_Free-$(size)kB ?
>
No, this is necessary.
Unless the kernel booting with parameter 'default_hugepagesz=1G' otherwise
there won't be any info about gigantic pages in /proc/meminfo, because Linux
only set the default hugepage size(2MB for x86_64) to
HugePages_Free-$(size)kB.
>
> > unsigned long free_hpages =
> SAFE_READ_MEMINFO("HugePages_Free:");
> > if (hp->number > free_hpages)
> > tst_brk(TCONF, "free_hpages = %lu, but expect %lu.
> "
> > --
> > 2.40.0
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
>
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation
2023-09-07 12:37 ` Li Wang
@ 2023-09-07 12:52 ` Li Wang
2023-09-07 14:10 ` Cyril Hrubis
1 sibling, 0 replies; 16+ messages in thread
From: Li Wang @ 2023-09-07 12:52 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp
On Thu, Sep 7, 2023 at 8:37 PM Li Wang <liwang@redhat.com> wrote:
> > if (val != tst_hugepages)
>> > tst_brk(TCONF, "nr_hugepages = %lu, but expect %lu. "
>> > "Not enough hugepages for testing.",
>> > val, tst_hugepages);
>> >
>> > - if (hp->policy == TST_NEEDS) {
>> > + if ((hp->policy == TST_NEEDS) && (!hp->hpsize)) {
>>
>> This branch shouldn't be disabled for TST_NEEDS case, shouldn't there be
>> HugePages_Free-$(size)kB ?
>>
>
>
> No, this is necessary.
>
> Unless the kernel booting with parameter 'default_hugepagesz=1G' otherwise
> there won't be any info about gigantic pages in /proc/meminfo, because
> Linux
> only set the default hugepage size(2MB for x86_64) to
> HugePages_Free-$(size)kB.
>
And, here we probably need add additional check branch for:
'/sys/kernel/mm/hugepages/hugepages-1048576kB/free_hugepages'
to guarantee the TST_NEEDS behavior is consistent with the default hugepage.
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation
2023-09-07 12:37 ` Li Wang
2023-09-07 12:52 ` Li Wang
@ 2023-09-07 14:10 ` Cyril Hrubis
1 sibling, 0 replies; 16+ messages in thread
From: Cyril Hrubis @ 2023-09-07 14:10 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
Hi!
> > This pointer (hugepage_path) is on stack and is passed to a function
> > that stores the structure into a list and then uses it to restore the
> > value at the end of the testrun, which will probably crash the test
> > since there will be random leftovers on the stack.
> >
>
>
> But the function tst_sys_conf_save_str allocates a new memory
> area for saving the structure info, the original is not used for
> appending to the list, isn't it?
>
> strncpy() inside that function helps avoid the problem you pointed.
My bad, I was for some reason under impression that it just stores the
pointer.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread
* [LTP] [PATCH v2 1/2] lib: add support for kinds of hpsize reservation
2023-05-24 9:39 [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation Li Wang
2023-05-24 9:39 ` [LTP] [RFC PATCH 2/2] hugemmap33: Test to detect bug with migrating gigantic pages Li Wang
2023-09-07 9:26 ` [LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation Cyril Hrubis
@ 2023-09-11 8:02 ` Li Wang
2023-09-11 8:02 ` [LTP] [PATCH v2 2/2] hugemmap34: Test to detect bug with migrating gigantic pages Li Wang
2023-09-14 9:44 ` [LTP] [PATCH v2 1/2] lib: add support for kinds of hpsize reservation Li Wang
2 siblings, 2 replies; 16+ messages in thread
From: Li Wang @ 2023-09-11 8:02 UTC (permalink / raw)
To: ltp
Typically when we make use of huge page via LTP library, .hugepages choose
the default hugepage size, but this can not satisfy all scenarios.
So this patch introduces applying a specified hpsize huge page for user.
There is nothing that needs to change for the existing test cases which
already using .hugepages, it only needs to fill one more field in the
structure of .hugepages if a different hpsize is required.
e.g.
static struct tst_test test = {
.needs_root = 1,
...
.hugepages = {2, TST_NEEDS, 1024 * 1024},
};
Signed-off-by: Li Wang <liwang@redhat.com>
---
Notes:
V1 --> V2
* check if the specific hugepage size exist or not
* guarantee the HugePages_Free-$(size)kB as expected
* print the reserved hugepage size
doc/c-test-api.txt | 38 ++++++++++++++++++++++++++++++++++++--
include/tst_hugepage.h | 1 +
lib/tst_hugepage.c | 33 ++++++++++++++++++++++++++-------
3 files changed, 63 insertions(+), 9 deletions(-)
diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index f692909e2..ec728cf8a 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -2034,9 +2034,15 @@ For full documentation see the comments in 'include/tst_fuzzy_sync.h'.
~~~~~~~~~~~~~~~~~~~~~~~~
Many of the LTP tests need to use hugepage in their testing, this allows the
-test can reserve hugepages from system via '.hugepages = {xx, TST_REQUEST}'.
+test can reserve specify size of hugepages from system via:
+ '.hugepages = {xx, TST_REQUEST, yy}'.
-We achieved two policies for reserving hugepages:
+xx: This is used to set how many pages we wanted.
+
+yy: This is used to specify the size of the hugepage.
+ (If not set it will use the default hugepage size)
+
+Two policies for reserving hugepages:
TST_REQUEST:
It will try the best to reserve available huge pages and return the number
@@ -2103,6 +2109,34 @@ struct tst_test test = {
};
-------------------------------------------------------------------------------
+or,
+
+[source,c]
+-------------------------------------------------------------------------------
+#include "tst_test.h"
+#define GIGANTIC_PGSZ 1024 * 1024
+
+static void run(void)
+{
+ ...
+}
+
+static void setup(void)
+{
+ /*
+ * Specify hpsize reserved automatically in the library
+ * $ echo 2 > /sys/kernel/mm//hugepages/hugepages-1048576kB/nr_hugepages
+ * Do check if 2 hpages are reserved correctly in there
+ */
+}
+
+struct tst_test test = {
+ .test_all = run,
+ .hugepages = {2, TST_NEEDS, GIGANTIC_PGSZ},
+ ...
+};
+-------------------------------------------------------------------------------
+
1.35 Checking for required commands
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/include/tst_hugepage.h b/include/tst_hugepage.h
index 46327c79a..e96bfbe53 100644
--- a/include/tst_hugepage.h
+++ b/include/tst_hugepage.h
@@ -27,6 +27,7 @@ enum tst_hp_policy {
struct tst_hugepage {
const unsigned long number;
enum tst_hp_policy policy;
+ const unsigned long hpsize;
};
/*
diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
index d2e70a955..f4b18bbbf 100644
--- a/lib/tst_hugepage.c
+++ b/lib/tst_hugepage.c
@@ -5,6 +5,7 @@
#define TST_NO_DEFAULT_MAIN
+#include <stdio.h>
#include "tst_test.h"
#include "tst_hugepage.h"
@@ -22,9 +23,10 @@ size_t tst_get_hugepage_size(void)
unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
{
- unsigned long val, max_hpages;
+ unsigned long val, max_hpages, hpsize;
+ char hugepage_path[PATH_MAX];
struct tst_path_val pvl = {
- .path = PATH_NR_HPAGES,
+ .path = hugepage_path,
.val = NULL,
.flags = TST_SR_SKIP_MISSING | TST_SR_TCONF_RO
};
@@ -41,6 +43,19 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
else
tst_hugepages = hp->number;
+ if (hp->hpsize)
+ hpsize = hp->hpsize;
+ else
+ hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE);
+
+ sprintf(hugepage_path, PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize);
+ if (access(hugepage_path, F_OK)) {
+ if (hp->policy == TST_NEEDS)
+ tst_brk(TCONF, "Hugepage size %lu not supported", hpsize);
+ tst_hugepages = 0;
+ goto out;
+ }
+
if (hp->number == TST_NO_HUGEPAGES) {
tst_hugepages = 0;
goto set_hugepages;
@@ -53,7 +68,7 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
goto set_hugepages;
}
- max_hpages = SAFE_READ_MEMINFO("MemFree:") / SAFE_READ_MEMINFO("Hugepagesize:");
+ max_hpages = SAFE_READ_MEMINFO("MemFree:") / hpsize;
if (tst_hugepages > max_hpages) {
tst_res(TINFO, "Requested number(%lu) of hugepages is too large, "
"limiting to 80%% of the max hugepage count %lu",
@@ -66,22 +81,26 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
set_hugepages:
tst_sys_conf_save(&pvl);
- SAFE_FILE_PRINTF(PATH_NR_HPAGES, "%lu", tst_hugepages);
- SAFE_FILE_SCANF(PATH_NR_HPAGES, "%lu", &val);
+
+ SAFE_FILE_PRINTF(hugepage_path, "%lu", tst_hugepages);
+ SAFE_FILE_SCANF(hugepage_path, "%lu", &val);
+
if (val != tst_hugepages)
tst_brk(TCONF, "nr_hugepages = %lu, but expect %lu. "
"Not enough hugepages for testing.",
val, tst_hugepages);
if (hp->policy == TST_NEEDS) {
- unsigned long free_hpages = SAFE_READ_MEMINFO("HugePages_Free:");
+ unsigned long free_hpages;
+ sprintf(hugepage_path, PATH_HUGEPAGES"/hugepages-%lukB/free_hugepages", hpsize);
+ SAFE_FILE_SCANF(hugepage_path, "%lu", &free_hpages);
if (hp->number > free_hpages)
tst_brk(TCONF, "free_hpages = %lu, but expect %lu. "
"Not enough hugepages for testing.",
free_hpages, hp->number);
}
- tst_res(TINFO, "%lu hugepage(s) reserved", tst_hugepages);
+ tst_res(TINFO, "%lu (%luMB) hugepage(s) reserved", tst_hugepages, hpsize/1024);
out:
return tst_hugepages;
}
--
2.40.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 16+ messages in thread* [LTP] [PATCH v2 2/2] hugemmap34: Test to detect bug with migrating gigantic pages
2023-09-11 8:02 ` [LTP] [PATCH v2 " Li Wang
@ 2023-09-11 8:02 ` Li Wang
2023-09-11 8:33 ` Li Wang
2023-09-14 9:44 ` [LTP] [PATCH v2 1/2] lib: add support for kinds of hpsize reservation Li Wang
1 sibling, 1 reply; 16+ messages in thread
From: Li Wang @ 2023-09-11 8:02 UTC (permalink / raw)
To: ltp
Signed-off-by: Li Wang <liwang@redhat.com>
---
Notes:
V1 --> V2
* make use of safe macros
* remove unnecessary .supported_archs
* remove unuseed numa.h
runtest/hugetlb | 1 +
testcases/kernel/mem/.gitignore | 1 +
.../kernel/mem/hugetlb/hugemmap/hugemmap34.c | 134 ++++++++++++++++++
3 files changed, 136 insertions(+)
create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap34.c
diff --git a/runtest/hugetlb b/runtest/hugetlb
index 299c07ac9..0c812c780 100644
--- a/runtest/hugetlb
+++ b/runtest/hugetlb
@@ -35,6 +35,7 @@ hugemmap29 hugemmap29
hugemmap30 hugemmap30
hugemmap31 hugemmap31
hugemmap32 hugemmap32
+hugemmap34 hugemmap34
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 7258489ed..41f547edf 100644
--- a/testcases/kernel/mem/.gitignore
+++ b/testcases/kernel/mem/.gitignore
@@ -34,6 +34,7 @@
/hugetlb/hugemmap/hugemmap30
/hugetlb/hugemmap/hugemmap31
/hugetlb/hugemmap/hugemmap32
+/hugetlb/hugemmap/hugemmap34
/hugetlb/hugeshmat/hugeshmat01
/hugetlb/hugeshmat/hugeshmat02
/hugetlb/hugeshmat/hugeshmat03
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap34.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap34.c
new file mode 100644
index 000000000..3743a54d2
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap34.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) Linux Test Project, 2023
+ * Copyright (C) 2023, Red Hat, Inc.
+ *
+ * Author: David Hildenbrand <david@redhat.com>
+ * Port-to-LTP: Li Wang <liwang@redhat.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Migration code will first unmap the old page and replace the present PTE
+ * by a migration entry. Then migrate the page. Once that succeeded (and there
+ * are no unexpected page references), we replace the migration entries by
+ * proper present PTEs pointing at the new page.
+ *
+ * For ordinary pages we handle PTEs. For 2 MiB hugetlb/THP, it's PMDs.
+ * For 1 GiB hugetlb, it's PUDs.
+ *
+ * So without below commit, GUP-fast code was simply not aware that we could
+ * have migration entries stored in PUDs. Migration + GUP-fast code should be
+ * able to handle any such races.
+ *
+ * For example, GUP-fast will re-verify the PUD after pinning to make sure it
+ * didn't change. If it did change, it backs off.
+ *
+ * Migration code should detect the additional page references and back off
+ * as well.
+ *
+ * commit 15494520b776aa2eadc3e2fefae524764cab9cea
+ * Author: Qiujun Huang <hqjagain@gmail.com>
+ * Date: Thu Jan 30 22:12:10 2020 -0800
+ *
+ * mm: fix gup_pud_range
+ *
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <stdbool.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <sys/ioctl.h>
+#include <linux/mempolicy.h>
+#include <linux/mman.h>
+
+#include "lapi/syscalls.h"
+#include "tst_safe_pthread.h"
+#include "numa_helper.h"
+#include "hugetlb.h"
+
+#define GIGANTIC_PGSZ 1024 * 1024
+
+static char *mem;
+static size_t pagesize;
+static size_t hugetlbsize;
+static volatile int looping = 1;
+
+static void *migration_thread_fn(void *arg LTP_ATTRIBUTE_UNUSED)
+{
+ while (looping) {
+ TST_EXP_PASS_SILENT(syscall(__NR_mbind, mem, hugetlbsize,
+ MPOL_LOCAL, NULL, 0x7fful, MPOL_MF_MOVE));
+ }
+
+ return NULL;
+}
+
+static void run_test(void)
+{
+ ssize_t transferred;
+ struct iovec iov;
+ int fds[2];
+
+ pthread_t migration_thread;
+
+ pagesize = getpagesize();
+ hugetlbsize = 1 * 1024 * 1024 * 1024u;
+
+ mem = mmap(NULL, hugetlbsize, PROT_READ|PROT_WRITE,
+ MAP_PRIVATE|MAP_ANON|MAP_HUGETLB|MAP_HUGE_1GB,
+ -1, 0);
+ if (mem == MAP_FAILED)
+ tst_brk(TBROK | TERRNO, "mmap() failed");
+
+ memset(mem, 1, hugetlbsize);
+
+ /* Keep migrating the page around ... */
+ SAFE_PTHREAD_CREATE(&migration_thread, NULL, migration_thread_fn, NULL);
+
+ while (looping) {
+ SAFE_PIPE(fds);
+
+ iov.iov_base = mem;
+ iov.iov_len = pagesize;
+ transferred = vmsplice(fds[1], &iov, 1, 0);
+ if (transferred <= 0)
+ tst_brk(TBROK | TERRNO, "vmsplice() failed");
+
+ SAFE_CLOSE(fds[0]);
+ SAFE_CLOSE(fds[1]);
+
+ if (!tst_remaining_runtime()) {
+ tst_res(TINFO, "Runtime exhausted, exiting");
+ looping = 0;
+ }
+ }
+
+ SAFE_PTHREAD_JOIN(migration_thread, NULL);
+
+ if (tst_taint_check())
+ tst_res(TFAIL, "Test resulted in kernel tainted");
+ else
+ tst_res(TPASS, "Test completed successfully");
+}
+
+static struct tst_test test = {
+ .needs_root = 1,
+ .test_all = run_test,
+ .max_runtime = 60,
+ .taint_check = TST_TAINT_W | TST_TAINT_D,
+ .hugepages = {2, TST_NEEDS, GIGANTIC_PGSZ},
+ .needs_kconfigs = (const char *const[]){
+ "CONFIG_NUMA=y",
+ NULL
+ },
+ .tags = (struct tst_tag[]) {
+ {"linux-git", "15494520b776"},
+ {}
+ },
+};
--
2.40.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [LTP] [PATCH v2 1/2] lib: add support for kinds of hpsize reservation
2023-09-11 8:02 ` [LTP] [PATCH v2 " Li Wang
2023-09-11 8:02 ` [LTP] [PATCH v2 2/2] hugemmap34: Test to detect bug with migrating gigantic pages Li Wang
@ 2023-09-14 9:44 ` Li Wang
2023-10-18 9:10 ` Richard Palethorpe
1 sibling, 1 reply; 16+ messages in thread
From: Li Wang @ 2023-09-14 9:44 UTC (permalink / raw)
To: ltp, Cyril Hrubis
Hi Cyril,
[Please hold off on merging this patch]
The hesitating part of this method (from myself) is the new field
'hp->hpsize'.
It seems not wise to leave it to users to fill the gigantic page size
manually,
as some arches support different huge/gigantic page sizes:
x86_64 and x86: 2MB and 1GB.
PowerPC: ranging from 64KB to 16GB.
ARM64: 2MB and 1GB.
IA-64 (Itanium): from 4KB to 256MB.
we probably need a intelengent way to detect and reserve whatever
hugepage or gitantic-page that all acmplish that in ltp-library or setup().
Then people don't need to fill any byte which avoiding typo or
something wrong.
What I can think of the improved way is to extend the hugepage policy
with "_G" subfix to specified the gigantic pages.
Is this sounds better? What do you think?
Something drafted base on my patch V2:
--- a/include/tst_hugepage.h
+++ b/include/tst_hugepage.h
@@ -20,14 +20,15 @@ extern char *nr_opt; /* -s num Set the number of the
been allocated hugepages
extern char *Hopt; /* -H /.. Location of hugetlbfs, i.e. -H
/var/hugetlbfs */
enum tst_hp_policy {
- TST_REQUEST,
- TST_NEEDS,
+ TST_REQUEST_H = 0x0,
+ TST_REQUEST_G = 0x1,
+ TST_NEEDS_H = 0x2,
+ TST_NEEDS_G = 0x4,
};
struct tst_hugepage {
const unsigned long number;
enum tst_hp_policy policy;
- const unsigned long hpsize;
};
/*
@@ -35,6 +36,11 @@ struct tst_hugepage {
*/
size_t tst_get_hugepage_size(void);
+/*
+ * Get the gigantic hugepage size. Returns 0 if hugepages are not
supported.
+ */
+size_t tst_get_gigantic_size(void);
+
/*
* Try the best to request a specified number of huge pages from system,
* it will store the reserved hpage number in tst_hugepages.
diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
index f4b18bbbf..568884fbb 100644
--- a/lib/tst_hugepage.c
+++ b/lib/tst_hugepage.c
@@ -21,6 +21,30 @@ size_t tst_get_hugepage_size(void)
return SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
}
+/* Check if hugetlb page is gigantic */
+static inline int is_hugetlb_gigantic(unsigned long hpage_size)
+{
+ return (hpage_size / getpagesize()) >> 11;
+}
+
+size_t tst_get_gigantic_size(void)
+{
+ DIR *dir;
+ struct dirent *ent;
+ unsigned long g_hpage_size;
+
+ dir = SAFE_OPENDIR(PATH_HUGEPAGES);
+ while ((ent = SAFE_READDIR(dir))) {
+ if ((sscanf(ent->d_name, "hugepages-%lukB", &g_hpage_size)
== 1) &&
+ is_hugetlb_gigantic(g_hpage_size * 1024)) {
+ break;
+ }
+ }
+
+ SAFE_CLOSEDIR(dir);
+ return g_hpage_size * 1024;
+}
+
unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
{
unsigned long val, max_hpages, hpsize;
@@ -43,10 +67,10 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage
*hp)
else
tst_hugepages = hp->number;
- if (hp->hpsize)
- hpsize = hp->hpsize;
+ if (hp->policy & (TST_NEEDS_G | TST_REQUEST_G))
+ hpsize = tst_get_gigantic_size() / 1024;
else
- hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE);
+ hpsize = tst_get_hugepage_size() / 1024;
sprintf(hugepage_path,
PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize);
if (access(hugepage_path, F_OK)) {
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [LTP] [PATCH v2 1/2] lib: add support for kinds of hpsize reservation
2023-09-14 9:44 ` [LTP] [PATCH v2 1/2] lib: add support for kinds of hpsize reservation Li Wang
@ 2023-10-18 9:10 ` Richard Palethorpe
2023-10-19 8:22 ` Li Wang
0 siblings, 1 reply; 16+ messages in thread
From: Richard Palethorpe @ 2023-10-18 9:10 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
Hello,
Li Wang <liwang@redhat.com> writes:
> Hi Cyril,
>
> [Please hold off on merging this patch]
>
> The hesitating part of this method (from myself) is the new field
> 'hp->hpsize'.
> It seems not wise to leave it to users to fill the gigantic page size
> manually,
> as some arches support different huge/gigantic page sizes:
Yes, good idea.
>
> x86_64 and x86: 2MB and 1GB.
> PowerPC: ranging from 64KB to 16GB.
> ARM64: 2MB and 1GB.
> IA-64 (Itanium): from 4KB to 256MB.
>
> we probably need a intelengent way to detect and reserve whatever
> hugepage or gitantic-page that all acmplish that in ltp-library or setup().
> Then people don't need to fill any byte which avoiding typo or
> something wrong.
It seems like a special flag is needed in mmap if you want to allocate a
gigantic page other than 1GB?
>
> What I can think of the improved way is to extend the hugepage policy
> with "_G" subfix to specified the gigantic pages.
>
> Is this sounds better? What do you think?
>
> Something drafted base on my patch V2:
>
> --- a/include/tst_hugepage.h
> +++ b/include/tst_hugepage.h
> @@ -20,14 +20,15 @@ extern char *nr_opt; /* -s num Set the number of the
> been allocated hugepages
> extern char *Hopt; /* -H /.. Location of hugetlbfs, i.e. -H
> /var/hugetlbfs */
>
> enum tst_hp_policy {
> - TST_REQUEST,
> - TST_NEEDS,
> + TST_REQUEST_H = 0x0,
> + TST_REQUEST_G = 0x1,
> + TST_NEEDS_H = 0x2,
> + TST_NEEDS_G = 0x4,
> };
>
> struct tst_hugepage {
> const unsigned long number;
> enum tst_hp_policy policy;
> - const unsigned long hpsize;
> };
Why not keep hpsize and add enum tst_hp_size { TST_HUGE, TST_GIGANTIC }?
In theory more sizes can be added.
>
> /*
> @@ -35,6 +36,11 @@ struct tst_hugepage {
> */
> size_t tst_get_hugepage_size(void);
>
> +/*
> + * Get the gigantic hugepage size. Returns 0 if hugepages are not
> supported.
> + */
> +size_t tst_get_gigantic_size(void);
> +
> /*
> * Try the best to request a specified number of huge pages from system,
> * it will store the reserved hpage number in tst_hugepages.
> diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
> index f4b18bbbf..568884fbb 100644
> --- a/lib/tst_hugepage.c
> +++ b/lib/tst_hugepage.c
> @@ -21,6 +21,30 @@ size_t tst_get_hugepage_size(void)
> return SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
> }
>
> +/* Check if hugetlb page is gigantic */
> +static inline int is_hugetlb_gigantic(unsigned long hpage_size)
> +{
> + return (hpage_size / getpagesize()) >> 11;
> +}
What is 11? If it is the order or shift of hugepages then that is not
constant (see below).
> +
> +size_t tst_get_gigantic_size(void)
> +{
> + DIR *dir;
> + struct dirent *ent;
> + unsigned long g_hpage_size;
> +
> + dir = SAFE_OPENDIR(PATH_HUGEPAGES);
> + while ((ent = SAFE_READDIR(dir))) {
> + if ((sscanf(ent->d_name, "hugepages-%lukB", &g_hpage_size)
> == 1) &&
> + is_hugetlb_gigantic(g_hpage_size * 1024)) {
> + break;
> + }
> + }
I guess in theory more gigantic page sizes could be added. I'm not sure
what size we should pick, but we don't want it to be random because it
would make debugging more difficult.
So could we search for the smallest size (hugepagesize) and second
smallest (smallest gigantic page)?
> +
> + SAFE_CLOSEDIR(dir);
> + return g_hpage_size * 1024;
> +}
> +
> unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
> {
> unsigned long val, max_hpages, hpsize;
> @@ -43,10 +67,10 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage
> *hp)
> else
> tst_hugepages = hp->number;
>
> - if (hp->hpsize)
> - hpsize = hp->hpsize;
> + if (hp->policy & (TST_NEEDS_G | TST_REQUEST_G))
> + hpsize = tst_get_gigantic_size() / 1024;
> else
> - hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE);
> + hpsize = tst_get_hugepage_size() / 1024;
>
> sprintf(hugepage_path,
> PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize);
> if (access(hugepage_path, F_OK)) {
>
>
>
>
> --
> Regards,
> Li Wang
--
Thank you,
Richard.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [LTP] [PATCH v2 1/2] lib: add support for kinds of hpsize reservation
2023-10-18 9:10 ` Richard Palethorpe
@ 2023-10-19 8:22 ` Li Wang
0 siblings, 0 replies; 16+ messages in thread
From: Li Wang @ 2023-10-19 8:22 UTC (permalink / raw)
To: rpalethorpe; +Cc: ltp
On Wed, Oct 18, 2023 at 6:09 PM Richard Palethorpe <rpalethorpe@suse.de>
wrote:
> Hello,
>
> Li Wang <liwang@redhat.com> writes:
>
> > Hi Cyril,
> >
> > [Please hold off on merging this patch]
> >
> > The hesitating part of this method (from myself) is the new field
> > 'hp->hpsize'.
> > It seems not wise to leave it to users to fill the gigantic page size
> > manually,
> > as some arches support different huge/gigantic page sizes:
>
> Yes, good idea.
>
> >
> > x86_64 and x86: 2MB and 1GB.
> > PowerPC: ranging from 64KB to 16GB.
> > ARM64: 2MB and 1GB.
> > IA-64 (Itanium): from 4KB to 256MB.
> >
> > we probably need a intelengent way to detect and reserve whatever
> > hugepage or gitantic-page that all acmplish that in ltp-library or
> setup().
> > Then people don't need to fill any byte which avoiding typo or
> > something wrong.
>
> It seems like a special flag is needed in mmap if you want to allocate a
> gigantic page other than 1GB?
>
Right, MAP_HUGE_2MB or MAP_HUGE_1GB used in conjunction
with MAP_HUGETLB to select alternative hugetlb page sizes.
But no matter which one to use, we should reserve enough
memory size for them in "HugePages_Free:". That's what
the ".hugepages" are doing in LTP library.
>
> >
> > What I can think of the improved way is to extend the hugepage policy
> > with "_G" subfix to specified the gigantic pages.
> >
> > Is this sounds better? What do you think?
> >
> > Something drafted base on my patch V2:
> >
> > --- a/include/tst_hugepage.h
> > +++ b/include/tst_hugepage.h
> > @@ -20,14 +20,15 @@ extern char *nr_opt; /* -s num Set the number of
> the
> > been allocated hugepages
> > extern char *Hopt; /* -H /.. Location of hugetlbfs, i.e. -H
> > /var/hugetlbfs */
> >
> > enum tst_hp_policy {
> > - TST_REQUEST,
> > - TST_NEEDS,
> > + TST_REQUEST_H = 0x0,
> > + TST_REQUEST_G = 0x1,
> > + TST_NEEDS_H = 0x2,
> > + TST_NEEDS_G = 0x4,
> > };
> >
> > struct tst_hugepage {
> > const unsigned long number;
> > enum tst_hp_policy policy;
> > - const unsigned long hpsize;
> > };
>
> Why not keep hpsize and add enum tst_hp_size { TST_HUGE, TST_GIGANTIC }?
>
Good advice!
I think maybe the 'enum tst_hp_type' is better? This sounds like the
proper type of hugepages we choose for the test.
enum tst_hp_policy {
TST_REQUEST,
TST_NEEDS,
};
enum tst_hp_type {
TST_HUGEPAGE,
TST_GIGANTIC,
};
struct tst_hugepage {
const unsigned long number;
enum tst_hp_policy policy;
enum tst_hp_type hptype;
};
...
static struct tst_test test = {
.needs_root = 1,
...
.hugepages = {2, TST_NEEDS, TST_GIGANTIC},
};
>
> In theory more sizes can be added.
>
> >
> > /*
> > @@ -35,6 +36,11 @@ struct tst_hugepage {
> > */
> > size_t tst_get_hugepage_size(void);
> >
> > +/*
> > + * Get the gigantic hugepage size. Returns 0 if hugepages are not
> > supported.
> > + */
> > +size_t tst_get_gigantic_size(void);
> > +
> > /*
> > * Try the best to request a specified number of huge pages from system,
> > * it will store the reserved hpage number in tst_hugepages.
> > diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
> > index f4b18bbbf..568884fbb 100644
> > --- a/lib/tst_hugepage.c
> > +++ b/lib/tst_hugepage.c
> > @@ -21,6 +21,30 @@ size_t tst_get_hugepage_size(void)
> > return SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
> > }
> >
> > +/* Check if hugetlb page is gigantic */
> > +static inline int is_hugetlb_gigantic(unsigned long hpage_size)
> > +{
> > + return (hpage_size / getpagesize()) >> 11;
> > +}
>
> What is 11? If it is the order or shift of hugepages then that is not
> constant (see below).
>
This function is extracted from mem/hugetlb/lib/hugetlb.h.
I think that 11 means, the gigantic-pgsize is not less than 2048 general
pgsize.
But you're right it can't cover all situations.
>
> > +
> > +size_t tst_get_gigantic_size(void)
> > +{
> > + DIR *dir;
> > + struct dirent *ent;
> > + unsigned long g_hpage_size;
> > +
> > + dir = SAFE_OPENDIR(PATH_HUGEPAGES);
> > + while ((ent = SAFE_READDIR(dir))) {
> > + if ((sscanf(ent->d_name, "hugepages-%lukB",
> &g_hpage_size)
> > == 1) &&
> > + is_hugetlb_gigantic(g_hpage_size * 1024)) {
> > + break;
> > + }
> > + }
>
> I guess in theory more gigantic page sizes could be added. I'm not sure
> what size we should pick, but we don't want it to be random because it
> would make debugging more difficult.
>
> So could we search for the smallest size (hugepagesize) and second
> smallest (smallest gigantic page)?
>
Yes, we could have a try on this way. To intelligently judge and pick the
proper size for gigantic or huge page, which should work for most
mainstream platforms(and TCONF on no-supported systems).
>
> > +
> > + SAFE_CLOSEDIR(dir);
> > + return g_hpage_size * 1024;
> > +}
> > +
> > unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
> > {
> > unsigned long val, max_hpages, hpsize;
> > @@ -43,10 +67,10 @@ unsigned long tst_reserve_hugepages(struct
> tst_hugepage
> > *hp)
> > else
> > tst_hugepages = hp->number;
> >
> > - if (hp->hpsize)
> > - hpsize = hp->hpsize;
> > + if (hp->policy & (TST_NEEDS_G | TST_REQUEST_G))
> > + hpsize = tst_get_gigantic_size() / 1024;
> > else
> > - hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE);
> > + hpsize = tst_get_hugepage_size() / 1024;
> >
> > sprintf(hugepage_path,
> > PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize);
> > if (access(hugepage_path, F_OK)) {
> >
> >
> >
> >
> > --
> > Regards,
> > Li Wang
>
>
> --
> Thank you,
> Richard.
>
>
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 16+ messages in thread