Re: [LTP] [PATCH 03/29] Hugetlb: Migrating libhugetlbfs corrupt-by-cow-opt

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Migrating the libhugetlbfs/testcases/corrupt-by-cow-opt.c test
> 
> Test Description: Test sanity of cow optimization on page cache. If a page
> in page cache has only 1 ref count, it is mapped for a private mapping
> directly and is overwritten freely, so next time we access the page, we
> can see corrupt data.
> 
> Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> ---
>  runtest/hugetlb                               |   1 +
>  testcases/kernel/mem/.gitignore               |   1 +
>  .../kernel/mem/hugetlb/hugemmap/hugemmap09.c  | 102 ++++++++++++++++++
>  3 files changed, 104 insertions(+)
>  create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
> 
> diff --git a/runtest/hugetlb b/runtest/hugetlb
> index 664f18827..e2ada7a97 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -5,6 +5,7 @@ hugemmap05 hugemmap05
>  hugemmap06 hugemmap06
>  hugemmap07 hugemmap07
>  hugemmap08 hugemmap08
> +hugemmap09 hugemmap09
>  hugemmap05_1 hugemmap05 -m
>  hugemmap05_2 hugemmap05 -s
>  hugemmap05_3 hugemmap05 -s -m
> diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
> index 003ce422b..1a242ffe0 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -6,6 +6,7 @@
>  /hugetlb/hugemmap/hugemmap06
>  /hugetlb/hugemmap/hugemmap07
>  /hugetlb/hugemmap/hugemmap08
> +/hugetlb/hugemmap/hugemmap09
>  /hugetlb/hugeshmat/hugeshmat01
>  /hugetlb/hugeshmat/hugeshmat02
>  /hugetlb/hugeshmat/hugeshmat03
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
> new file mode 100644
> index 000000000..eeacf68a2
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: LGPL-2.1-or-later
> +/*
> + * Copyright (C) 2013 Joonsoo Kim, LG Electronics.
> + *
> + * Test Name: Corrupt by COW optimization
> + *
> + * Test Description: Test sanity of cow optimization on page cache. If a page
> + * in page cache has only 1 ref count, it is mapped for a private mapping
> + * directly and is overwritten freely, so next time we access the page, we
> + * can see corrupt data.

Here as well.

> + * HISTORY
> + *  Written by Joonsoo Kim
> + *
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <sys/mount.h>
> +#include <limits.h>
> +#include <sys/param.h>
> +#include <sys/types.h>
> +
> +#include "hugetlb.h"
> +
> +static char *verbose;
> +static int  fd = -1;
> +static char hfile[MAXPATHLEN];
> +static long hpage_size;
> +
> +static void run_test(void)
> +{
> +	char *p;
> +	char c;
> +
> +	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
> +	SAFE_UNLINK(hfile);

This SAFE_OPEN() and SAFE_UNLINK() should be in setup()

> +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +	*p = 's';
> +	if (verbose)
> +		tst_res(TINFO, "Write %c to %p via shared mapping", *p, p);
> +	SAFE_MUNMAP(p, hpage_size);
> +
> +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
> +	*p = 'p';
> +	if (verbose)
> +		tst_res(TINFO, "Write %c to %p via private mapping", *p, p);
> +	SAFE_MUNMAP(p, hpage_size);
> +
> +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +	c = *p;
> +	if (verbose)
> +		tst_res(TINFO, "Read %c from %p via shared mapping", *p, p);
> +	SAFE_MUNMAP(p, hpage_size);

Also I'm not keen on the verbose flag, I would just print these messages
by default. It does not make sense to have verbose flag because in
practice it's not enabled on production testing and these days we got
reports from various automated systems so we better include anything
useful by default.

> +	/* Direct write from huge page */
> +	if (c != 's') {
> +		tst_res(TFAIL, "Data got corrupted.");
> +		goto fail;
> +	}
> +	tst_res(TPASS, "Successful");

Here the cleanest code would just be:

	if (c != 's')
		tst_res(TFAIL, "Data are corrupted");
	else
		tst_res(TPASS, "Data are correct");

> +	SAFE_CLOSE(fd);

And this SAFE_CLOSE() is already in cleanup() so we just need to move
the SAFE_OPEN() into the setup() and remove this one.

> +	return;
> +fail:
> +	tst_brk(TBROK, "Once failed, No point in continuing the test");
> +}
> +
> +static void setup(void)
> +{
> +	if (tst_hugepages < 2)
> +		tst_brk(TCONF, "Not enough hugepages for testing.");
> +
> +	if (!Hopt)
> +		Hopt = tst_get_tmpdir();
> +	SAFE_MOUNT("none", Hopt, "hugetlbfs", 0, NULL);
> +
> +	snprintf(hfile, sizeof(hfile), "%s/ltp_hugetlbfile%d", Hopt, getpid());
> +	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd >= 0)
> +		SAFE_CLOSE(fd);
> +	umount2(Hopt, MNT_DETACH);

Here as well, umount only what was mounted.

> +}
> +
> +static struct tst_test test = {
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +	.options = (struct tst_option[]) {
> +		{"v", &verbose, "Turns on verbose mode"},
> +		{"H:", &Hopt,   "Location of hugetlbfs, i.e.  -H /var/hugetlbfs"},
> +		{"s:", &nr_opt, "Set the number of the been allocated hugepages"},
> +		{}
> +	},
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run_test,
> +	.hugepages = {2, TST_REQUEST},
> +};
> -- 
> 2.31.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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