Re: [LTP] [PATCH] syscalls/madvise11: new test for madvise(MADV_DONTNEED)

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Test cases for madvise(2) system call, advise value as "MADV_MADV_DONTNEED":
> 1. After a successful MADV_DONTNEED operation, it will result in
> zero-fill-on-demand pages for anonymous private mappings
> 2. MADV_DONTNEED cannot be applied to Huge TLB pages
> 
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
>  runtest/syscalls                              |  1 +
>  testcases/kernel/syscalls/madvise/.gitignore  |  1 +
>  testcases/kernel/syscalls/madvise/madvise11.c | 87 +++++++++++++++++++

At the moment we do not have madvise03.c and madvise04.c, why don't we
use them first?

>  3 files changed, 89 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/madvise/madvise11.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 61a7b7677..a8ed9d65e 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -946,6 +946,7 @@ madvise07 madvise07
>  madvise08 madvise08
>  madvise09 madvise09
>  madvise10 madvise10
> +madvise11 madvise11
> 
>  newuname01 newuname01
> 
> diff --git a/testcases/kernel/syscalls/madvise/.gitignore b/testcases/kernel/syscalls/madvise/.gitignore
> index 002d8e5d9..6e5b92ab7 100644
> --- a/testcases/kernel/syscalls/madvise/.gitignore
> +++ b/testcases/kernel/syscalls/madvise/.gitignore
> @@ -6,3 +6,4 @@
>  /madvise08
>  /madvise09
>  /madvise10
> +/madvise11
> diff --git a/testcases/kernel/syscalls/madvise/madvise11.c b/testcases/kernel/syscalls/madvise/madvise11.c
> new file mode 100644
> index 000000000..358c07d3a
> --- /dev/null
> +++ b/testcases/kernel/syscalls/madvise/madvise11.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) Huawei Technologies Co., Ltd. 2022. All rights reserved.
> + * Author: Zhao Gongyi <zhaogongyi@huawei.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Test cases for madvise(2) system call, advise value as "MADV_MADV_DONTNEED":
> + * 1. After a successful MADV_DONTNEED operation, it will result in
> + *    zero-fill-on-demand pages for anonymous private mappings
> + * 2. MADV_DONTNEED cannot be applied to Huge TLB pages
> + */
> +
> +#include "tst_test.h"
> +
> +#define MAP_SIZE (8 * 1024)
> +
> +static char *addr;

There is no need for this to be a global variable.

> +static void test_madvise01(void)
> +{
> +	int i;
> +
> +	addr = SAFE_MMAP(NULL, MAP_SIZE,
> +			PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS,
> +			-1, 0);
> +	memset(addr, 1, MAP_SIZE);
> +
> +	TEST(madvise(addr, MAP_SIZE, MADV_DONTNEED));
> +	if (TST_RET == -1) {
> +		tst_brk(TBROK | TTERRNO, "madvise(%p, %d, 0x%x) failed",
> +			addr, MAP_SIZE, MADV_DONTNEED);
> +	}
> +
> +	for (i = 0; i < MAP_SIZE; i++) {
> +		if (addr[i]) {
> +			tst_res(TFAIL,
> +				"There are no zero-fill-on-demand pages "
> +				"for anonymous private mappings");
> +			break;

I would do goto ret; here insetad and point ret just before the unmap at
the end, that way there would be no reason to add the if (i == MAP_SIZE)
before the TPASS message.

> +		}
> +	}
> +
> +	if (i == MAP_SIZE) {
> +		tst_res(TPASS,
> +			"There are zero-fill-on-demand pages "
> +			"for anonymous private mappings");
> +	}
> +
> +	SAFE_MUNMAP(addr, MAP_SIZE);
> +}
> +
> +static void test_madvise02(void)
> +{
> +	int mapsz = tst_get_hugepage_size();
> +
> +	addr = SAFE_MMAP(NULL, mapsz,
> +			PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
> +			-1, 0);
> +
> +	TEST(madvise(addr, mapsz, MADV_DONTNEED));
> +	if (TST_RET != -1) {
> +		tst_res(TFAIL, "madvise(%p, %d, 0x%x) succeed unexpectedly",
> +			addr, mapsz, MADV_DONTNEED);
> +	} else {
> +		tst_res(TPASS, "madvise test for 'MADV_DONTNEED' passed");
> +	}

Please make use of TST_EXP_FAIL()

> +	SAFE_MUNMAP(addr, mapsz);
> +}
> +
> +static void test_madvise(void)
> +{
> +	test_madvise01();
> +	test_madvise02();
> +}
> +
> +static struct tst_test test = {
> +	.test_all = test_madvise,
> +	.needs_root = 1,
> +	.hugepages = {1, TST_NEEDS},

This would mean that the first case would be skipped in case where
hugepages are not supported or if the allocation failed. The best option
here would be splitting the test into two so that the functional test is
not disabled when hugepages are not supported.

> +};
> +
> --
> 2.17.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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