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

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Zhao Gongyi via ltp <ltp@lists.linux.it> writes:

> Check that madvise(2) MADV_DONTNEED applied to shared mappings will lead to
> the resident set size(RSS) of the calling process reduced immediately.
>
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
>  runtest/syscalls                              |  1 +
>  testcases/kernel/syscalls/madvise/.gitignore  |  1 +
>  testcases/kernel/syscalls/madvise/madvise11.c | 82 +++++++++++++++++++
>  3 files changed, 84 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/madvise/madvise11.c
>
> diff --git a/runtest/syscalls b/runtest/syscalls
> index eb1910cec..296af9f9d 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -948,6 +948,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 db8ce47c1..ffd8823d1 100644
> --- a/testcases/kernel/syscalls/madvise/.gitignore
> +++ b/testcases/kernel/syscalls/madvise/.gitignore
> @@ -8,3 +8,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..0132c091c
> --- /dev/null
> +++ b/testcases/kernel/syscalls/madvise/madvise11.c
> @@ -0,0 +1,82 @@
> +// 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]
> + *
> + * Check that madvise(2) MADV_DONTNEED applied to shared mappings will lead to
> + * the resident set size(RSS) of the calling process reduced immediately.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include "tst_test.h"
> +
> +#define MAP_SIZE (8 * 1024)
> +#define BUF_SIZE 1024
> +
> +static FILE *fp;
> +static char *addr;
> +
> +static void run(void)
> +{
> +	char cmd[BUF_SIZE];
> +	char line[BUF_SIZE];
> +	char vm_area_addr[128];
> +
> +	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);
> +	}

You have a lot of madvise patches, so why not create SAFE_MADVISE?

Or there are the TST_EXP_* macros.

> +
> +	sprintf(vm_area_addr, "%p", addr);
> +	sprintf(cmd, "cat /proc/%d/smaps", getpid());
> +	fp = popen(cmd, "r");
> +
> +	/* Find the vm area */
> +	while (fgets(line, sizeof(line), fp) != NULL) {
> +		if (strstr(line, &(vm_area_addr[2])))

AFAICT this could match more than one line by matching the end of the
preceding range.

I think that in general it's better to avoid strstr if memcmp can be
easily used instead.

> +			break;
> +	}
> +
> +	/* Find Rss size of the vm area */
> +	while (fgets(line, sizeof(line), fp) != NULL) {
> +		if (strstr(line, "Rss:")) {

Same here although Rss: seems to be unique. Then again it could be added
to another field in new kernel.

Perhaps sscanf would be better?

> +			if (strstr(line, " 0 kB"))
> +				tst_res(TPASS, "RSS is released");
> +			else
> +				tst_res(TFAIL, "RSS is not released");
> +			return;
> +		}
> +	}
> +
> +	tst_brk(TBROK, "There is no 'Rss:' or vm_area %p in smaps?", addr);
> +}
> +
> +static void setup(void)
> +{
> +	addr = SAFE_MMAP(NULL, MAP_SIZE,
> +			PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS,
> +			-1, 0);
> +	memset(addr, 1, MAP_SIZE);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (addr)
> +		SAFE_MUNMAP(addr, MAP_SIZE);
> +	if (fp)
> +		pclose(fp);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};
> +
> --
> 2.17.1

Otherwise looks good.

-- 
Thank you,
Richard.

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