Re: [LTP] [PATCH v1] swapping01.c: Reporting /proc/meminfo before test and at the moment of the failure

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

nit: the subject is:
"swapping01.c: Reporting /proc/meminfo before test and at the moment of the failure"

This is way too long, please make it shorter (it should be up to 72 chars),
you can write more at other lines (not everything have to be on the subject).

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/mem/swapping/swapping01.c | 24 +++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)

> diff --git a/testcases/kernel/mem/swapping/swapping01.c b/testcases/kernel/mem/swapping/swapping01.c
> index fc225e4a6..07d89f44b 100644
> --- a/testcases/kernel/mem/swapping/swapping01.c
> +++ b/testcases/kernel/mem/swapping/swapping01.c
> @@ -60,6 +60,17 @@ static long mem_over_max;
>  static pid_t pid;
>  static unsigned int start_runtime;

NOTE: we have tst_available_mem() and tst_available_swap() in
lib/tst_memutils.c. Wouldn't be they enough? Or other line from /proc/meminfo
(new function would need to be added)? I'm not sure myself.

Also, is it really needed to print both /proc/meminfo and /proc/PID/status?
And if it's needed, why other testcases/kernel/mem/ tests don't need it?
Notes below are if we decide that we want this (I'd like to hear others).

> +static void print_meminfo(char *path)

You use print_meminfo() for printing /proc/meminfo and /proc/PID/status.
Instead, there could be a generic function safe_print_file(char *path) either in
safe_macros.c or tst_print_file(char *path) in tst_safe_file_at.c (I slightly
prefer the first, there is a mess with files in lib/*.c, there are way too many
files with just few functions).
The function would also print TINFO message which file is printing.

And then, in lib/tst_memutils.c would be tst_print_meminfo() which would call
safe_print_file("/proc/meminfo") and tst_print_status(pid_t pid), which would do
sprintf(proc_status_path, "/proc/%d/status", pid) and call safe_print_file()
with the result.

NOTE: you need to add also function signature to corresponding include/*.h file.
This should be done as a separate commit, using function in swapping01.c would
be as a separate commit.

> +{
> +	FILE *file;
> +	char line[PATH_MAX];
> +
> +	file = SAFE_FOPEN(path, "r");
> +	while (fgets(line, sizeof(line), file))
> +		tst_res(TINFO, "%s", line);
> +	SAFE_FCLOSE(file);
> +}
> +
>  static void test_swapping(void)
>  {
>  #ifdef TST_ABI32
> @@ -84,6 +95,8 @@ static void test_swapping(void)
>  	switch (pid = SAFE_FORK()) {
>  	case 0:
>  		do_alloc(0);
> +		tst_res(TINFO, "Meminfo before check:");
IMHO this TINFO should be removed (TINFO in the printing function is enough).

> +		print_meminfo("/proc/meminfo");


>  		do_alloc(1);
>  		exit(0);
>  	default:
> @@ -108,9 +121,11 @@ static void do_alloc(int allow_raise)
>  	long mem_count;
>  	void *s;

> -	if (allow_raise == 1)
> +	if (allow_raise == 1) {
> +		init_meminfo();
Why this? You haven't mentioned it in the commit message.

>  		tst_res(TINFO, "available physical memory: %ld MB",
>  				mem_available_init / 1024);
> +	}
...

>  	swapped = SAFE_READ_PROC_STATUS(pid, "VmSwap:");
>  	if (swapped > mem_over_max) {
> +		tst_res(TINFO, "Heavy swapping detected, print meminfo:");
Again, I'd prefer generic message in print_meminfo() (on a single place).
IMHO it will be obvious that first print is before test and the second after.

Kind regards,
Petr
> +		print_meminfo("/proc/meminfo");
> +		tst_res(TINFO, "Heavy swapping detected, print proc status:");
> +		sprintf(proc_status_path, "/proc/%d/status", pid);
> +		print_meminfo(proc_status_path);
>  		kill(pid, SIGCONT);
>  		tst_brk(TFAIL, "heavy swapping detected: "
>  				"%ld MB swapped.", swapped / 1024);

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