From mboxrd@z Thu Jan  1 00:00:00 1970
From: Cyril Hrubis <chrubis@suse.cz>
Date: Thu, 9 Feb 2017 11:56:35 +0100
Subject: [LTP] [PATCH v3] syscalls/move_pages12: Add new regression test
In-Reply-To: <1486627987-3297-1-git-send-email-fenggw-fnst@cn.fujitsu.com>
References: <589C1B4A.8020303@cn.fujitsu.com>
 <1486627987-3297-1-git-send-email-fenggw-fnst@cn.fujitsu.com>
Message-ID: <20170209105635.GC12673@rei.lan>
List-Id: <ltp.lists.linux.it>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: ltp@lists.linux.it

Hi!
> +#include "tst_test.h"
> +#include "move_pages_support.h"
> +
> +#if HAVE_NUMA_MOVE_PAGES
> +
> +#define LOOPS	1000
> +#define PATH_MEMINFO	"/proc/meminfo"
> +#define PATH_NR_HUGEPAGES	"/proc/sys/vm/nr_hugepages"
> +#define PATH_HUGEPAGES	"/sys/kernel/mm/hugepages/"
> +#define TEST_PAGES	2
> +#define TEST_NODES	2
> +
> +static int pgsz, hpsz;
> +static long orig_hugepages;
> +static unsigned int node1, node2;
> +static void *addr;
> +
> +static void do_child(void)
> +{
> +	int test_pages = TEST_PAGES * hpsz / pgsz;
> +	int i, j;
> +	int *nodes, *status;
> +	void **pages;
> +	pid_t ppid = getppid();
> +
> +	pages = SAFE_MALLOC(sizeof(char *) * test_pages + 1);
> +	nodes = SAFE_MALLOC(sizeof(char *) * test_pages + 1);
> +	status = SAFE_MALLOC(sizeof(char *) * test_pages + 1);

I know that this is taken from to original reproduced, but these
allocations appears to be wrong. Both nodes and status are, as far as I
can tell, arrays of integers, so this should in fact be:

pages = SAFE_MALLOC(sizeof(char *) * test_pages);
nodes = SAFE_MALLOC(sizeof(int) * test_pages);
status = SAFE_MALLOC(sizeof(int) * test_pages);

I'm not even sure why there is + 1 in the original code, what is that
extra byte usefull for.

Does the reproducer still work once we allocate these arrays correctly?

> +	for (i = 0; i < test_pages; i++)
> +		pages[i] = addr + i * pgsz;
> +
> +	for (i = 0; ; i++) {
> +		for (j = 0; j < test_pages; j++) {
> +			if (i % 2 == 0)
> +				nodes[j] = node1;
> +			else
> +				nodes[j] = node2;
> +			status[j] = 0;
> +		}
> +
> +		TEST(numa_move_pages(ppid, test_pages,
> +			pages, nodes, status, MPOL_MF_MOVE_ALL));
> +		if (TEST_RETURN) {
> +			tst_res(TFAIL | TTERRNO, "move_pages failed");
> +			break;
> +		}
> +	}
> +
> +	exit(0);
> +}
> +
> +static void do_test(void)
> +{
> +	int i;
> +	pid_t cpid = -1;
> +	int status;
> +
> +	for (i = 0; i < LOOPS; i++) {
> +		addr = SAFE_MMAP(NULL, TEST_PAGES * hpsz,
> +			PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0);
> +
> +		memset(addr, 0, TEST_PAGES * hpsz);
> +
> +		SAFE_MUNMAP(addr, TEST_PAGES * hpsz);
> +
> +		if (i == 0) {
> +			cpid = SAFE_FORK();
> +			if (cpid == 0)
> +				do_child();
> +		}
> +	}
> +
> +	if (i == LOOPS) {
> +		SAFE_KILL(cpid, SIGKILL);
> +		SAFE_WAITPID(cpid, &status, 0);
> +		if (!WIFEXITED(status))
> +			tst_res(TPASS, "Bug not reproduced");
> +	}
> +}
> +
> +static void setup(void)
> +{
> +	int memfree, ret;
> +
> +	check_config(TEST_NODES);
> +
> +	if (access(PATH_HUGEPAGES, F_OK))
> +		tst_brk(TCONF, "Huge page not supported");
> +
> +	pgsz = (int)get_page_size();
> +	SAFE_FILE_LINES_SCANF(PATH_MEMINFO, "Hugepagesize: %d", &hpsz);
> +	hpsz *= 1024;
> +
> +	SAFE_FILE_LINES_SCANF(PATH_MEMINFO, "MemFree: %d", &memfree);
> +	memfree *= 1024;
> +	if (4 * hpsz > memfree)
> +		tst_brk(TBROK, "RAM not enough");
                                   ^
				This should rather be "Not enough free RAM"

Or something similar, but that is minor.

> +	SAFE_FILE_SCANF(PATH_NR_HUGEPAGES, "%ld", &orig_hugepages);
> +	SAFE_FILE_PRINTF(PATH_NR_HUGEPAGES, "%ld", orig_hugepages + 4);
> +
> +	ret = get_allowed_nodes(NH_MEMS, TEST_NODES, &node1, &node2);
> +	if (ret < 0)
> +		tst_brk(TBROK | TERRNO, "get_allowed_nodes: %d", ret);
> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_FILE_PRINTF(PATH_NR_HUGEPAGES, "%ld", orig_hugepages);
> +}
> +
> +static struct tst_test test = {
> +	.tid = "move_pages12",
> +	.min_kver = "2.6.32",
> +	.needs_root = 1,
> +	.forks_child = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = do_test,
> +};
> +
> +#else
> +	tst_res(TCONF, "move_pages support not found");
> +#endif

The rest looks good.

-- 
Cyril Hrubis
chrubis@suse.cz