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

[zhaogongyi]

Hi Cyril,

I have submit a new version patches as your suggestions, pleas see:
https://patchwork.ozlabs.org/project/ltp/patch/20221011121607.55575-2-zhaogongyi@huawei.com/
https://patchwork.ozlabs.org/project/ltp/patch/20221011121607.55575-3-zhaogongyi@huawei.com/
https://patchwork.ozlabs.org/project/ltp/patch/20221011121607.55575-4-zhaogongyi@huawei.com/

Thanks,
Gongyi


> 
> 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