[LTP] [PATCH v2] hugetlb/hugemmap: add new testcase hugemmap06.c

[Alexey Kodanev <alexey.kodanev@oracle.com>]

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