Re: [LTP] [LTP PATCH v1 1/1] syscalls/madvise11: Add some memory page soft-offlining control

[Petr Vorel <pvorel@suse.cz>]

Hi William,

Just few quick notes before somebody get to proper review:

There should be a record in runtest/syscalls:
madvise11 madvise11

...
> +++ b/testcases/kernel/syscalls/madvise/Makefile
> +
> +/*\
> + * [Description]
> + * Stress the VMM and soft-offline code by spawning N threads which
> + * allocate memory pages and soft-offline them repeatedly.
nit: if you intend this get formatted as paragraph, insert spaces:

> + * Control that soft-offlined pages get correctly replaced: with the
> + * same content and without SIGBUS generation when accessed.
and here

> + * Could be used for example as a regression test-case for the
> + * poisoned soft-offlined pages case fixed by upstream commit
> + * d4ae9916ea29 (mm: soft-offline: close the race against page allocation)
Instead of this I'd put .tags record into struct tst_test.
https://github.com/linux-test-project/ltp/wiki/C-Test-API#138-test-tags

Also there are some formatting errors:
$ make check-madvise11
CHECK testcases/kernel/syscalls/madvise/madvise11.c
madvise11.c:46: ERROR: do not initialise statics to 0
madvise11.c:51: WARNING: Missing a blank line after declarations
madvise11.c:55: ERROR: open brace '{' following function definitions go on the next line
madvise11.c:213: ERROR: "foo* bar" should be "foo *bar"
madvise11.c:254: WARNING: Missing a blank line after declarations
madvise11.c:295: ERROR: return is not a function, parentheses are not required
madvise11.c:303: ERROR: "foo* bar" should be "foo *bar"
madvise11.c:316: ERROR: space required after that ',' (ctx:VxV)
madvise11.c:338: ERROR: space required after that ',' (ctx:VxV)

I'd also consider which comments are really useful. Most of them is, but at
least some document what is obvious, e.g.: 
/* Number of threads to create */
/* Success! */ => you specified return at the comment above the function
allocate_offline(), also this return code is pretty obvious:

...
> +	fd = SAFE_OPEN("/dev/kmsg", O_WRONLY);
> +	if (fd < 0) {
> +		tst_res(TCONF | TERRNO,"/dev/kmsg not available for writing");
> +		return;
> +	}
If check is not needed, because SAFE_OPEN() exits the test with
tst_brk(TBROK, "open(...) failed"

Also fd might not get unclosed, which could hit too many open files.

...

Also trying Fedora once more:
Running single iteration (./madvise11) or two iterations (./madvise11 -i2)
it fails in the end:

# ./madvise11 -i2
...
madvise11.c:136: TINFO: Thread [59]: allocate_offline() returned 0, succeeded.  Thread exiting
madvise11.c:175: TPASS: soft-offline/alloc stress test finished successfully
madvise11.c:316: TINFO: Restore 900 Soft-offlined pages
tst_test.c:1583: TBROK: Test killed by SIGSEGV!

Summary:
passed   2
failed   0
broken   1
skipped  0
warnings 0

But running more, i.e. 5 iterations, I don't get SIGSEGV.

Kind regards,
Petr

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