* [LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c
@ 2015-11-26 3:02 Li Wang
2015-11-26 9:22 ` Alexey Kodanev
0 siblings, 1 reply; 6+ messages in thread
From: Li Wang @ 2015-11-26 3:02 UTC (permalink / raw)
To: ltp
v1 ---> v2
1. remove the useless include file
2. add huge pages checking in setup()
3. mmap huge pages in reverse order
Description of Problem:
There is a race condition if we map a same file on different processes.
Region tracking is protected by mmap_sem and hugetlb_instantiation_mutex.
When we do mmap, we don't grab a hugetlb_instantiation_mutex, but only
mmap_sem (exclusively). This doesn't prevent other tasks from modifying
the region structure, so it can be modified by two processes concurrently.
Testcase hugemmap06.c is the trigger to cause system crash:
crash> bt -s
PID: 4492 TASK: ffff88033e437520 CPU: 2 COMMAND: "hugemmap06"
#0 [ffff88033dbb3960] machine_kexec+395 at ffffffff8103d1ab
#1 [ffff88033dbb39c0] crash_kexec+114 at ffffffff810cc4f2
#2 [ffff88033dbb3a90] oops_end+192 at ffffffff8153c840
#3 [ffff88033dbb3ac0] die+91 at ffffffff81010f5b
#4 [ffff88033dbb3af0] do_general_protection+338 at ffffffff8153c332
#5 [ffff88033dbb3b20] general_protection+37 at ffffffff8153bb05
[exception RIP: list_del+40]
RIP: ffffffff812a3598 RSP: ffff88033dbb3bd8 RFLAGS: 00010292
RAX: dead000000100100 RBX: ffff88013cf37340 RCX: 0000000000002dc2
RDX: dead000000200200 RSI: 0000000000000046 RDI: 0000000000000009
RBP: ffff88033dbb3be8 R8: 0000000000015598 R9: 0000000000000000
R10: 000000000000000f R11: 0000000000000009 R12: 000000000000000a
R13: ffff88033d64b9e8 R14: ffff88033e5b9720 R15: ffff88013cf37340
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0000
#6 [ffff88033dbb3bf0] region_add+154 at ffffffff811698da
#7 [ffff88033dbb3c40] alloc_huge_page+669 at ffffffff8116a61d
#8 [ffff88033dbb3ce0] hugetlb_fault+1083 at ffffffff8116b9bb
#9 [ffff88033dbb3d90] handle_mm_fault+917 at ffffffff81153295
#10 [ffff88033dbb3e00] __do_page_fault+326 at ffffffff8104f156
#11 [ffff88033dbb3f20] do_page_fault+62 at ffffffff8153e78e
#12 [ffff88033dbb3f50] page_fault+37 at ffffffff8153bb35
RIP: 00000000004027c6 RSP: 00007f7cadef9e80 RFLAGS: 00010297
RAX: 000000005a49238f RBX: 00007ffcb2d19320 RCX: 000000357498e084
RDX: 000000357498e0b0 RSI: 00007f7cadef9e5c RDI: 000000357498e4e0
RBP: 0000000000000008 R8: 000000357498e0a0 R9: 000000357498e100
R10: 00007f7cadefa9d0 R11: 0000000000000206 R12: 0000000000000007
R13: 0000000000000002 R14: 0000000000000003 R15: 00002aaaac000000
ORIG_RAX: ffffffffffffffff CS: 0033 SS: 002b
The fix are all these below commits:
commit f522c3ac00a49128115f99a5fcb95a447601c1c3
Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
Date:Wed Sep 11 14:21:53 2013 - 0700
commit 9119 a41e9091fb3a8204039d595bcdae24193c57
Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
Date:Thu Apr 3 14:47:25 2014 - 0700
commit 7 b24d8616be33616efd41ff67d3c76362c60ca84
Author:Davidlohr Bueso < davidlohr @ hp.com >
Date:Thu Apr 3 14:47:27 2014 - 0700
commit 1406ec9 ba6c65cb69e9243bff07ca3f51e2525e0
Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
Date:Thu Apr 3 14:47:26 2014 - 0700
Signed-off-by: Li Wang <liwang@redhat.com>
---
runtest/hugetlb | 1 +
testcases/kernel/mem/.gitignore | 1 +
testcases/kernel/mem/hugetlb/hugemmap/Makefile | 2 +
testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c | 192 +++++++++++++++++++++
4 files changed, 196 insertions(+)
create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c
diff --git a/runtest/hugetlb b/runtest/hugetlb
index 2e9f215..ac24513 100644
--- a/runtest/hugetlb
+++ b/runtest/hugetlb
@@ -2,6 +2,7 @@ hugemmap01 hugemmap01
hugemmap02 hugemmap02
hugemmap04 hugemmap04
hugemmap05 hugemmap05
+hugemmap06 hugemmap06
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 4702377..ac8e6d8 100644
--- a/testcases/kernel/mem/.gitignore
+++ b/testcases/kernel/mem/.gitignore
@@ -3,6 +3,7 @@
/hugetlb/hugemmap/hugemmap02
/hugetlb/hugemmap/hugemmap04
/hugetlb/hugemmap/hugemmap05
+/hugetlb/hugemmap/hugemmap06
/hugetlb/hugeshmat/hugeshmat01
/hugetlb/hugeshmat/hugeshmat02
/hugetlb/hugeshmat/hugeshmat03
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/Makefile b/testcases/kernel/mem/hugetlb/hugemmap/Makefile
index 71da630..3d22042 100644
--- a/testcases/kernel/mem/hugetlb/hugemmap/Makefile
+++ b/testcases/kernel/mem/hugetlb/hugemmap/Makefile
@@ -25,3 +25,5 @@ top_srcdir ?= ../../../../..
include $(top_srcdir)/include/mk/testcases.mk
include $(top_srcdir)/testcases/kernel/mem/include/libmem.mk
include $(top_srcdir)/include/mk/generic_leaf_target.mk
+
+hugemmap06: CFLAGS+=-pthread
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c
new file mode 100644
index 0000000..77250f2
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c
@@ -0,0 +1,192 @@
+/*
+ * Copyright (c) Author: Herton R. Krzesinski <herton@redhat.com>
+ * Modify: Li Wang <liwang@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+/*
+ * DESCRIPTION
+ *
+ * There is a race condition if we map a same file on different processes.
+ * Region tracking is protected by mmap_sem and hugetlb_instantiation_mutex.
+ * When we do mmap, we don't grab a hugetlb_instantiation_mutex, but only
+ * mmap_sem (exclusively). This doesn't prevent other tasks from modifying
+ * the region structure, so it can be modified by two processes concurrently.
+ *
+ * This bug was fixed on stable kernel by commits:
+ *
+ * commit f522c3ac00a49128115f99a5fcb95a447601c1c3
+ * Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
+ * Date: Wed Sep 11 14:21:53 2013 -0700
+ *
+ * commit 9119a41e9091fb3a8204039d595bcdae24193c57
+ * Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
+ * Date: Thu Apr 3 14:47:25 2014 -0700
+ *
+ * commit 7b24d8616be33616efd41ff67d3c76362c60ca84
+ * Author: Davidlohr Bueso <davidlohr@hp.com>
+ * Date: Thu Apr 3 14:47:27 2014 -0700
+ *
+ * commit 1406ec9ba6c65cb69e9243bff07ca3f51e2525e0
+ * Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
+ * Date: Thu Apr 3 14:47:26 2014 -0700
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include "test.h"
+#include "mem.h"
+
+char *TCID = "hugemmap06";
+int TST_TOTAL = 5;
+
+static long hpage_size;
+static long hugepages;
+static long orig_hugepages;
+
+struct mp {
+ void *addr;
+ int sz;
+};
+
+#define ARSZ 50
+
+void setup(void)
+{
+ tst_require_root();
+ check_hugepage();
+
+ hpage_size = read_meminfo("Hugepagesize:") * 1024;
+ orig_hugepages = get_sys_tune("nr_hugepages");
+
+ hugepages = (ARSZ + 1) * TST_TOTAL;
+
+ if (hugepages * read_meminfo("Hugepagesize:") > read_meminfo("MemTotal:"))
+ tst_brkm(TCONF, NULL, "System RAM is not enough to test.");
+
+ set_sys_tune("nr_hugepages", hugepages, 1);
+
+ TEST_PAUSE;
+}
+
+void cleanup(void)
+{
+ set_sys_tune("nr_hugepages", orig_hugepages, 0);
+}
+
+void *thr(void *arg)
+{
+ struct mp *mmap_sz = arg;
+ int i, lim, a, b, c;
+
+ srand(time(NULL));
+ lim = rand() % 10;
+ for (i = 0; i < lim; i++) {
+ a = rand() % mmap_sz->sz;
+ for (c = 0; c <= a; c++) {
+ b = rand() % mmap_sz->sz;
+ *((int *)((char *)mmap_sz->addr + (b * hpage_size))) = rand();
+ }
+ }
+ return NULL;
+}
+
+void do_mmap(void)
+{
+ int i, sz;
+ void *addr, *new_addr;
+ struct mp mmap_sz[ARSZ];
+ pthread_t tid[ARSZ];
+
+ sz = ARSZ;
+ addr = mmap(NULL, sz * hpage_size,
+ PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
+ -1, 0);
+
+ if (addr == MAP_FAILED) {
+ if (errno == ENOMEM) {
+ tst_brkm(TCONF, cleanup,
+ "Cannot allocate hugepage, memory too fragmented?");
+ }
+
+ tst_brkm(TBROK | TERRNO, cleanup, "Cannot allocate hugepage");
+ }
+
+ for (i = ARSZ - 1; i > 0; i--) {
+ mmap_sz[i].sz = sz;
+ mmap_sz[i].addr = addr;
+
+ TEST(pthread_create(tid + i, NULL, thr, &mmap_sz[i]));
+ if (TEST_RETURN)
+ tst_brkm(TBROK | TRERRNO, cleanup,
+ "pthread_create failed");
+
+ new_addr = mmap(addr, (sz - 1) * hpage_size,
+ PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB | MAP_FIXED,
+ -1, 0);
+
+ if (new_addr == MAP_FAILED) {
+ TEST(pthread_join(tid[i], NULL));
+ if (TEST_RETURN)
+ tst_brkm(TBROK | TRERRNO, cleanup,
+ "pthread_join failed");
+ tst_brkm(TFAIL | TERRNO, cleanup, "mremap failed");
+ }
+ sz--;
+ addr = new_addr;
+ }
+
+ for (++i; i < ARSZ; i++) {
+ TEST(pthread_join(tid[i], NULL));
+ if (TEST_RETURN)
+ tst_brkm(TBROK | TRERRNO, cleanup,
+ "pthread_join failed");
+ }
+
+ if (munmap(addr, sz * hpage_size) == -1)
+ tst_brkm(TFAIL | TERRNO, cleanup, "huge munmap failed");
+}
+
+int main(int ac, char **av)
+{
+ int lc, i;
+
+ tst_parse_opts(ac, av, NULL, NULL);
+
+ setup();
+
+ for (lc = 0; TEST_LOOPING(lc); lc++) {
+ tst_count = 0;
+
+ for (i = 0; i < TST_TOTAL; i++)
+ do_mmap();
+
+ tst_resm(TPASS, "No regression found.");
+ }
+
+ cleanup();
+ tst_exit();
+}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c
2015-11-26 3:02 [LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c Li Wang
@ 2015-11-26 9:22 ` Alexey Kodanev
2015-11-26 12:25 ` Li Wang
2015-11-26 13:59 ` Cyril Hrubis
0 siblings, 2 replies; 6+ messages in thread
From: Alexey Kodanev @ 2015-11-26 9:22 UTC (permalink / raw)
To: ltp
On 11/26/2015 06:02 AM, Li Wang wrote:
> v1 ---> v2
> 1. remove the useless include file
> 2. add huge pages checking in setup()
> 3. mmap huge pages in reverse order
A paragraph about new version changes must be after:
Signed-off-by: Li Wang <liwang@redhat.com>
---
as it shouldn't be in a commit message.
>
> Description of Problem:
> There is a race condition if we map a same file on different processes.
> Region tracking is protected by mmap_sem and hugetlb_instantiation_mutex.
> When we do mmap, we don't grab a hugetlb_instantiation_mutex, but only
> mmap_sem (exclusively). This doesn't prevent other tasks from modifying
> the region structure, so it can be modified by two processes concurrently.
>
> Testcase hugemmap06.c is the trigger to cause system crash:
>
> crash> bt -s
> PID: 4492 TASK: ffff88033e437520 CPU: 2 COMMAND: "hugemmap06"
> #0 [ffff88033dbb3960] machine_kexec+395 at ffffffff8103d1ab
> #1 [ffff88033dbb39c0] crash_kexec+114 at ffffffff810cc4f2
> #2 [ffff88033dbb3a90] oops_end+192 at ffffffff8153c840
> #3 [ffff88033dbb3ac0] die+91 at ffffffff81010f5b
> #4 [ffff88033dbb3af0] do_general_protection+338 at ffffffff8153c332
> #5 [ffff88033dbb3b20] general_protection+37 at ffffffff8153bb05
> [exception RIP: list_del+40]
> RIP: ffffffff812a3598 RSP: ffff88033dbb3bd8 RFLAGS: 00010292
> RAX: dead000000100100 RBX: ffff88013cf37340 RCX: 0000000000002dc2
> RDX: dead000000200200 RSI: 0000000000000046 RDI: 0000000000000009
> RBP: ffff88033dbb3be8 R8: 0000000000015598 R9: 0000000000000000
> R10: 000000000000000f R11: 0000000000000009 R12: 000000000000000a
> R13: ffff88033d64b9e8 R14: ffff88033e5b9720 R15: ffff88013cf37340
> ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0000
> #6 [ffff88033dbb3bf0] region_add+154 at ffffffff811698da
> #7 [ffff88033dbb3c40] alloc_huge_page+669 at ffffffff8116a61d
> #8 [ffff88033dbb3ce0] hugetlb_fault+1083 at ffffffff8116b9bb
> #9 [ffff88033dbb3d90] handle_mm_fault+917 at ffffffff81153295
> #10 [ffff88033dbb3e00] __do_page_fault+326 at ffffffff8104f156
> #11 [ffff88033dbb3f20] do_page_fault+62 at ffffffff8153e78e
> #12 [ffff88033dbb3f50] page_fault+37 at ffffffff8153bb35
> RIP: 00000000004027c6 RSP: 00007f7cadef9e80 RFLAGS: 00010297
> RAX: 000000005a49238f RBX: 00007ffcb2d19320 RCX: 000000357498e084
> RDX: 000000357498e0b0 RSI: 00007f7cadef9e5c RDI: 000000357498e4e0
> RBP: 0000000000000008 R8: 000000357498e0a0 R9: 000000357498e100
> R10: 00007f7cadefa9d0 R11: 0000000000000206 R12: 0000000000000007
> R13: 0000000000000002 R14: 0000000000000003 R15: 00002aaaac000000
> ORIG_RAX: ffffffffffffffff CS: 0033 SS: 002b
>
> The fix are all these below commits:
> commit f522c3ac00a49128115f99a5fcb95a447601c1c3
> Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
> Date:Wed Sep 11 14:21:53 2013 - 0700
This is a more appropriate way of listing commits:
f522c3ac00a4 ("mm, hugetlb: change variable name reservations to resv")
>
> commit 9119 a41e9091fb3a8204039d595bcdae24193c57
> Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
> Date:Thu Apr 3 14:47:25 2014 - 0700
>
> commit 7 b24d8616be33616efd41ff67d3c76362c60ca84
> Author:Davidlohr Bueso < davidlohr @ hp.com >
> Date:Thu Apr 3 14:47:27 2014 - 0700
>
> commit 1406ec9 ba6c65cb69e9243bff07ca3f51e2525e0
> Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
> Date:Thu Apr 3 14:47:26 2014 - 0700
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>
...
> +
> +hugemmap06: CFLAGS+=-pthread
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c
> new file mode 100644
> index 0000000..77250f2
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c
> @@ -0,0 +1,192 @@
> +/*
> + * Copyright (c) Author: Herton R. Krzesinski <herton@redhat.com>
> + * Modify: Li Wang <liwang@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> + * the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
Please change it to
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +/*
> + * DESCRIPTION
> + *
> + * There is a race condition if we map a same file on different processes.
> + * Region tracking is protected by mmap_sem and hugetlb_instantiation_mutex.
> + * When we do mmap, we don't grab a hugetlb_instantiation_mutex, but only
> + * mmap_sem (exclusively). This doesn't prevent other tasks from modifying
> + * the region structure, so it can be modified by two processes concurrently.
> + *
> + * This bug was fixed on stable kernel by commits:
> + *
> + * commit f522c3ac00a49128115f99a5fcb95a447601c1c3
> + * Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> + * Date: Wed Sep 11 14:21:53 2013 -0700
> + *
> + * commit 9119a41e9091fb3a8204039d595bcdae24193c57
> + * Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> + * Date: Thu Apr 3 14:47:25 2014 -0700
> + *
> + * commit 7b24d8616be33616efd41ff67d3c76362c60ca84
> + * Author: Davidlohr Bueso <davidlohr@hp.com>
> + * Date: Thu Apr 3 14:47:27 2014 -0700
> + *
> + * commit 1406ec9ba6c65cb69e9243bff07ca3f51e2525e0
> + * Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> + * Date: Thu Apr 3 14:47:26 2014 -0700
> + */
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include "test.h"
> +#include "mem.h"
> +
> +char *TCID = "hugemmap06";
> +int TST_TOTAL = 5;
> +
> +static long hpage_size;
> +static long hugepages;
> +static long orig_hugepages;
> +
> +struct mp {
> + void *addr;
> + int sz;
> +};
> +
> +#define ARSZ 50
> +
> +void setup(void)
> +{
> + tst_require_root();
> + check_hugepage();
> +
> + hpage_size = read_meminfo("Hugepagesize:") * 1024;
> + orig_hugepages = get_sys_tune("nr_hugepages");
> +
> + hugepages = (ARSZ + 1) * TST_TOTAL;
> +
> + if (hugepages * read_meminfo("Hugepagesize:") > read_meminfo("MemTotal:"))
> + tst_brkm(TCONF, NULL, "System RAM is not enough to test.");
> +
> + set_sys_tune("nr_hugepages", hugepages, 1);
> +
> + TEST_PAUSE;
> +}
> +
> +void cleanup(void)
> +{
> + set_sys_tune("nr_hugepages", orig_hugepages, 0);
> +}
> +
> +void *thr(void *arg)
> +{
> + struct mp *mmap_sz = arg;
> + int i, lim, a, b, c;
> +
> + srand(time(NULL));
> + lim = rand() % 10;
> + for (i = 0; i < lim; i++) {
> + a = rand() % mmap_sz->sz;
> + for (c = 0; c <= a; c++) {
> + b = rand() % mmap_sz->sz;
> + *((int *)((char *)mmap_sz->addr + (b * hpage_size))) = rand();
> + }
> + }
> + return NULL;
> +}
> +
> +void do_mmap(void)
> +{
> + int i, sz;
> + void *addr, *new_addr;
> + struct mp mmap_sz[ARSZ];
> + pthread_t tid[ARSZ];
> +
> + sz = ARSZ;
> + addr = mmap(NULL, sz * hpage_size,
> + PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
> + -1, 0);
> +
> + if (addr == MAP_FAILED) {
> + if (errno == ENOMEM) {
> + tst_brkm(TCONF, cleanup,
> + "Cannot allocate hugepage, memory too fragmented?");
> + }
> +
> + tst_brkm(TBROK | TERRNO, cleanup, "Cannot allocate hugepage");
> + }
> +
> + for (i = ARSZ - 1; i > 0; i--) {
Why this is done in reverse order?
> + mmap_sz[i].sz = sz;
> + mmap_sz[i].addr = addr;
> +
> + TEST(pthread_create(tid + i, NULL, thr, &mmap_sz[i]));
This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i), not tid + i.
> + if (TEST_RETURN)
> + tst_brkm(TBROK | TRERRNO, cleanup,
> + "pthread_create failed");
> +
> + new_addr = mmap(addr, (sz - 1) * hpage_size,
> + PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB | MAP_FIXED,
> + -1, 0);
> +
> + if (new_addr == MAP_FAILED) {
> + TEST(pthread_join(tid[i], NULL));
> + if (TEST_RETURN)
> + tst_brkm(TBROK | TRERRNO, cleanup,
> + "pthread_join failed");
> + tst_brkm(TFAIL | TERRNO, cleanup, "mremap failed");
> + }
> + sz--;
> + addr = new_addr;
> + }
> +
> + for (++i; i < ARSZ; i++) {
Could you initialize "i" here explicitly?
> + TEST(pthread_join(tid[i], NULL));
> + if (TEST_RETURN)
> + tst_brkm(TBROK | TRERRNO, cleanup,
> + "pthread_join failed");
> + }
> +
> + if (munmap(addr, sz * hpage_size) == -1)
> + tst_brkm(TFAIL | TERRNO, cleanup, "huge munmap failed");
> +}
> +
> +int main(int ac, char **av)
> +{
> + int lc, i;
> +
> + tst_parse_opts(ac, av, NULL, NULL);
> +
> + setup();
> +
> + for (lc = 0; TEST_LOOPING(lc); lc++) {
> + tst_count = 0;
> +
> + for (i = 0; i < TST_TOTAL; i++)
> + do_mmap();
> +
> + tst_resm(TPASS, "No regression found.");
> + }
> +
> + cleanup();
> + tst_exit();
Isn't tst_exit() calling cleanup?
Best regards,
Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c
2015-11-26 9:22 ` Alexey Kodanev
@ 2015-11-26 12:25 ` Li Wang
2015-11-26 18:05 ` Alexey Kodanev
2015-11-26 13:59 ` Cyril Hrubis
1 sibling, 1 reply; 6+ messages in thread
From: Li Wang @ 2015-11-26 12:25 UTC (permalink / raw)
To: ltp
Hi,
On Thu, Nov 26, 2015 at 5:22 PM, Alexey Kodanev <alexey.kodanev@oracle.com>
wrote:
> On 11/26/2015 06:02 AM, Li Wang wrote:
>
>> v1 ---> v2
>> 1. remove the useless include file
>> 2. add huge pages checking in setup()
>> 3. mmap huge pages in reverse order
>>
>
> A paragraph about new version changes must be after:
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>
> as it shouldn't be in a commit message.
ok, thanks for reminding.
>
>
>
>> The fix are all these below commits:
>> commit f522c3ac00a49128115f99a5fcb95a447601c1c3
>> Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
>> Date:Wed Sep 11 14:21:53 2013 - 0700
>>
>
> This is a more appropriate way of listing commits:
>
> f522c3ac00a4 ("mm, hugetlb: change variable name reservations to resv")
>
yeah, that's pretty good.
> + * Copyright (c) Author: Herton R. Krzesinski <herton@redhat.com>
>> + * Modify: Li Wang <liwang@redhat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
>> + * the GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> + * 02110-1301 USA
>> + */
>>
> Please change it to
>
> You should have received a copy of the GNU General Public License
> along with this program. If not, see <http://www.gnu.org/licenses/>.
>
>
ok, let me read it again. ;-)
> + tst_brkm(TBROK | TERRNO, cleanup, "Cannot allocate
>> hugepage");
>> + }
>> +
>> + for (i = ARSZ - 1; i > 0; i--) {
>>
>
> Why this is done in reverse order?
>
well, as Jan pointed in the previous comments:
"How about allocating the largest area first and then mmaping smaller ones
on top of it?
That could prevent situation where first smallest area mmaps a gap between
existing libraries/heap/etc. and then larger ones with same start overlap
with those - since we write to those areas, bad things could happen."
I think that is very clear to describe the potential issue. since I run the
program on some s390x for many times, it is easily to get segment fault.
after done in reverse order. the error gone.
here I print the detailed mapings of the program, it show something that.
-----
Program received signal SIGSEGV, Segmentation fault.
0x0000004cf80a68b0 in mmap64 () from /lib64/libc.so.6
(gdb) bt
#0 0x0000004cf80a68b0 in mmap64 () from /lib64/libc.so.6
Cannot access memory at address 0x70
Start Addr End Addr Size Offset objfile
0x80000000 0x80001000 0x1000
0 /root/a.out
0x80001000 0x80002000 0x1000
0x1000 /root/a.out
0xb7a9b000 0xb7abc000 0x21000
0 [heap]
0x4cf7f73000 0x4cf7f94000 0x21000
0 /lib64/ld-2.12.so
0x4cf7f94000 0x4cf7f95000 0x1000
0x20000 /lib64/ld-2.12.so
0x4cf7f95000 0x4cf7f96000 0x1000
0x21000 /lib64/ld-2.12.so
0x4cf7f96000 0x4cf7f97000 0x1000 0
0x4cf7f9d000 0x4cf814d000 0x1b0000
0 /lib64/libc-2.12.so
0x4cf814d000 0x4cf8151000 0x4000
0x1af000 /lib64/libc-2.12.so
0x4cf8151000 0x4cf8152000 0x1000
0x1b3000 /lib64/libc-2.12.so
0x4cf8152000 0x4cf8157000 0x5000 0
0x4cf8159000 0x4cf8176000 0x1d000
0 /lib64/libpthread-2.12.so
0x4cf8176000 0x4cf8177000 0x1000
0x1c000 /lib64/libpthread-2.12.so
0x4cf8177000 0x4cf8178000 0x1000
0x1d000 /lib64/libpthread-2.12.so
>
> + mmap_sz[i].sz = sz;
>> + mmap_sz[i].addr = addr;
>> +
>> + TEST(pthread_create(tid + i, NULL, thr, &mmap_sz[i]));
>>
>
> This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i), not tid +
> i.
>
sorry, probably you want say this:
(pthread_t *)((char *)tid + sizeof(pthread_t) * i)
but I think the original way is right.
-----------
# cat a.c
#include <stdio.h>
int main()
{
int a[2];
*a = 'a';
*(a + 1) = 'b';
printf("a[1] = %d\n", a[1]);
printf("&a[0] = %p\n", a);
printf("&a[1] = %p\n", a + 1);
}
--------
# ./a.out
a[1] = 98
&a[0] = 0x3ffffe25f18
&a[1] = 0x3ffffe25f1c
>
>
> + }
>> +
>> + for (++i; i < ARSZ; i++) {
>>
>
> Could you initialize "i" here explicitly?
ok, no problem.
>
> + cleanup();
>> + tst_exit();
>>
>
> Isn't tst_exit() calling cleanup?
>
hmm, I checked the /lib/tst_res.c and got the function:
----
void tst_exit(void)
{
pthread_mutex_lock(&tmutex);
#if DEBUG
printf("IN tst_exit\n");
fflush(stdout);
fflush(stdout);
#endif
/* Call tst_flush() flush any output in the buffer. */
tst_flush();
/* Mask out TINFO result from the exit status. */
exit(T_exitval & ~TINFO);
}
I didn't saw there anyplace call the cleanup() function. :(
>
> Best regards,
> Alexey
>
thanks!
--
Regards,
Li Wang
Email: liwang@redhat.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20151126/f732f1d2/attachment-0001.html>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c
2015-11-26 9:22 ` Alexey Kodanev
2015-11-26 12:25 ` Li Wang
@ 2015-11-26 13:59 ` Cyril Hrubis
1 sibling, 0 replies; 6+ messages in thread
From: Cyril Hrubis @ 2015-11-26 13:59 UTC (permalink / raw)
To: ltp
Hi!
> > + mmap_sz[i].sz = sz;
> > + mmap_sz[i].addr = addr;
> > +
> > + TEST(pthread_create(tid + i, NULL, thr, &mmap_sz[i]));
>
> This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i), not tid + i.
Hmm, since when arr + i != &arr[i] ?
When arr is pthread_t * type it's incremented by sizeof(pthread_t). As
far as I know arr[i] is just shorthand for *(arr + i). Which is a reason
why obscure things like i[arr] compile and give the same answer as
arr[i] even then technically the value is undefined.
If you want to increment pointer by 1 you have to explicitly cast it to
char * or intptr_t.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c
2015-11-26 12:25 ` Li Wang
@ 2015-11-26 18:05 ` Alexey Kodanev
2015-11-27 4:06 ` Li Wang
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Kodanev @ 2015-11-26 18:05 UTC (permalink / raw)
To: ltp
Hi,
On 11/26/2015 03:25 PM, Li Wang wrote:
>
>
>
> + tst_brkm(TBROK | TERRNO, cleanup, "Cannot
> allocate hugepage");
> + }
> +
> + for (i = ARSZ - 1; i > 0; i--) {
>
>
> Why this is done in reverse order?
>
>
> well, as Jan pointed in the previous comments:
>
> "How about allocating the largest area first and then mmaping smaller ones
> on top of it?
>
> That could prevent situation where first smallest area mmaps a gap between
> existing libraries/heap/etc. and then larger ones with same start overlap
> with those - since we write to those areas, bad things could happen."
>
> I think that is very clear to describe the potential issue. since I
> run the program on some s390x for many times, it is easily to get
> segment fault. after done in reverse order. the error gone.
>
> here I print the detailed mapings of the program, it show something that.
> -----
OK, but I meant that either we need to remove "sz" variable and use "i *
hpage_size" or
make it straightforward as "i" doesn't influence on the actual size
passed to mmap and threads.
So that both of the loops looks the same.
Also to use the full range of declared arrays, we should do as follows
or declare them as ARSZ - 1:
struct mp mmap_sz[ARSZ];
pthread_t tid[ARSZ];
sz = ARSZ + 1;
...
for (i = 0; i < ARSZ; ++i, --sz) {
mmap_sz[i].sz = sz;
mmap_sz[i].addr = addr;
...
}
for (i = 0; i < ARSZ; ++i) {
pthread_join(...);
}
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000004cf80a68b0 in mmap64 () from /lib64/libc.so.6
> (gdb) bt
> #0 0x0000004cf80a68b0 in mmap64 () from /lib64/libc.so.6
> Cannot access memory at address 0x70
>
>
> Start Addr End Addr Size Offset objfile
> 0x80000000 0x80001000 0x1000
> 0 /root/a.out
> 0x80001000 0x80002000 0x1000
> 0x1000 /root/a.out
> 0xb7a9b000 0xb7abc000 0x21000 0 [heap]
> 0x4cf7f73000 0x4cf7f94000 0x21000
> 0 /lib64/ld-2.12.so <http://ld-2.12.so>
> 0x4cf7f94000 0x4cf7f95000 0x1000
> 0x20000 /lib64/ld-2.12.so <http://ld-2.12.so>
> 0x4cf7f95000 0x4cf7f96000 0x1000
> 0x21000 /lib64/ld-2.12.so <http://ld-2.12.so>
> 0x4cf7f96000 0x4cf7f97000 0x1000 0
> 0x4cf7f9d000 0x4cf814d000 0x1b0000
> 0 /lib64/libc-2.12.so <http://libc-2.12.so>
> 0x4cf814d000 0x4cf8151000 0x4000
> 0x1af000 /lib64/libc-2.12.so <http://libc-2.12.so>
> 0x4cf8151000 0x4cf8152000 0x1000
> 0x1b3000 /lib64/libc-2.12.so <http://libc-2.12.so>
> 0x4cf8152000 0x4cf8157000 0x5000 0
> 0x4cf8159000 0x4cf8176000 0x1d000
> 0 /lib64/libpthread-2.12.so
> <http://libpthread-2.12.so>
> 0x4cf8176000 0x4cf8177000 0x1000
> 0x1c000 /lib64/libpthread-2.12.so
> <http://libpthread-2.12.so>
> 0x4cf8177000 0x4cf8178000 0x1000
> 0x1d000 /lib64/libpthread-2.12.so
> <http://libpthread-2.12.so>
>
>
> + mmap_sz[i].sz = sz;
> + mmap_sz[i].addr = addr;
> +
> + TEST(pthread_create(tid + i, NULL, thr,
> &mmap_sz[i]));
>
>
> This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i),
> not tid + i.
>
>
> sorry, probably you want say this:
>
> (pthread_t *)((char *)tid + sizeof(pthread_t) * i)
>
> but I think the original way is right.
Yes, you are right, sorry for misleading you.
I would recommend to use the same style in one function, so
TEST(pthread_create(tid + i, NULL, thr, mmap_sz + i));
or
TEST(pthread_create(&tid[i], NULL, thr, &mmap_sz[i]));
> -----------
> # cat a.c
> #include <stdio.h>
> int main()
> {
> int a[2];
>
> *a = 'a';
> *(a + 1) = 'b';
>
> printf("a[1] = %d\n", a[1]);
>
> printf("&a[0] = %p\n", a);
> printf("&a[1] = %p\n", a + 1);
> }
>
> --------
> # ./a.out
> a[1] = 98
> &a[0] = 0x3ffffe25f18
> &a[1] = 0x3ffffe25f1c
>
>
>
> + }
> +
> + for (++i; i < ARSZ; i++) {
>
>
> Could you initialize "i" here explicitly?
>
> ok, no problem.
>
>
> + cleanup();
> + tst_exit();
>
>
> Isn't tst_exit() calling cleanup?
>
>
> hmm, I checked the /lib/tst_res.c and got the function:
>
> ----
> void tst_exit(void)
> {
> pthread_mutex_lock(&tmutex);
>
> #if DEBUG
> printf("IN tst_exit\n");
> fflush(stdout);
> fflush(stdout);
> #endif
>
> /* Call tst_flush() flush any output in the buffer. */
> tst_flush();
>
> /* Mask out TINFO result from the exit status. */
> exit(T_exitval & ~TINFO);
> }
>
> I didn't saw there anyplace call the cleanup() function. :(
Right, I've read the mail thread recently about changing cleanup() to be
declared as a signal handler before
and somehow thought it is already implemented in LTP...
Thanks,
Alexey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20151126/81321814/attachment-0001.html>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c
2015-11-26 18:05 ` Alexey Kodanev
@ 2015-11-27 4:06 ` Li Wang
0 siblings, 0 replies; 6+ messages in thread
From: Li Wang @ 2015-11-27 4:06 UTC (permalink / raw)
To: ltp
Hi,
On Fri, Nov 27, 2015 at 2:05 AM, Alexey Kodanev <alexey.kodanev@oracle.com>
wrote:
>
> Hi,
> On 11/26/2015 03:25 PM, Li Wang wrote:
> ...
> OK, but I meant that either we need to remove "sz" variable and use "i *
> hpage_size" or
> make it straightforward as "i" doesn't influence on the actual size passed
> to mmap and threads.
>
> So that both of the loops looks the same.
>
> Also to use the full range of declared arrays, we should do as follows or
> declare them as ARSZ - 1:
>
> struct mp mmap_sz[ARSZ];
> pthread_t tid[ARSZ];
>
> sz = ARSZ + 1;
>
> ...
>
> for (i = 0; i < ARSZ; ++i, --sz) {
> mmap_sz[i].sz = sz;
> mmap_sz[i].addr = addr;
> ...
> }
>
> for (i = 0; i < ARSZ; ++i) {
> pthread_join(...);
>
> }
>
looks like this form is better. thanks!
>
>> + mmap_sz[i].sz = sz;
>>> + mmap_sz[i].addr = addr;
>>> +
>>> + TEST(pthread_create(tid + i, NULL, thr, &mmap_sz[i]));
>>>
>>
>> This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i), not tid +
>> i.
>>
>
> sorry, probably you want say this:
>
> (pthread_t *)((char *)tid + sizeof(pthread_t) * i)
>
> but I think the original way is right.
>
>
> Yes, you are right, sorry for misleading you.
>
> I would recommend to use the same style in one function, so
>
> TEST(pthread_create(tid + i, NULL, thr, mmap_sz + i));
> or
> TEST(pthread_create(&tid[i], NULL, thr, &mmap_sz[i]));
>
yeah, it is more neatly. agreed.
> ...
>
> /* Call tst_flush() flush any output in the buffer. */
> tst_flush();
>
> /* Mask out TINFO result from the exit status. */
> exit(T_exitval & ~TINFO);
> }
>
> I didn't saw there anyplace call the cleanup() function. :(
>
>
> Right, I've read the mail thread recently about changing cleanup() to be
> declared as a signal handler before
> and somehow thought it is already implemented in LTP...
>
> Thanks,
> Alexey
>
>
ok, let me post patch v3.
--
Regards,
Li Wang
Email: liwang@redhat.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20151127/a4dd7305/attachment.html>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-27 4:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-26 3:02 [LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c Li Wang
2015-11-26 9:22 ` Alexey Kodanev
2015-11-26 12:25 ` Li Wang
2015-11-26 18:05 ` Alexey Kodanev
2015-11-27 4:06 ` Li Wang
2015-11-26 13:59 ` Cyril Hrubis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.