Menu
▾
▴
Re: [LTP] [PATCH v3] functional testing for Kernel Samepage Merging
|
From: Garrett C. <yan...@gm...> - 2010-11-08 21:48:50
|
On Wed, Nov 3, 2010 at 11:36 PM, CAI Qian <ca...@re...> wrote:
> Basic tests were to start several programs with same and different
> memory contents and ensure only to merge the ones with the same
> contents. When changed the content of one of merged pages in a
> process and to the mode "unmerging", it should discard all merged
> pages there. Also tested it is possible to disable KSM. There are
> also command-line options to specify the memory allocation size, and
> number of processes have same memory contents so it is possible to
> test more advanced things like KSM + OOM etc. KSM statistics numbers
> look like hard to predict, so only check the ranges.
Can you provide documentation that describes the functionality under test?
> Signed-off-by: CAI Qian <ca...@re...>
> ---
> v3: fixed expected values based on the feedback from KSM developers;
> fixed madvise return code; replaced malloc with mmap calls to better
> align page boundaries; added additional memory content verification
> after each run.
>
> v2: fixed stupid mistakes about memory content assignment.
>
> testcases/kernel/mem/ksm/Makefile | 22 ++
> testcases/kernel/mem/ksm/ksm01.c | 634 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 656 insertions(+), 0 deletions(-)
> create mode 100644 testcases/kernel/mem/ksm/Makefile
> create mode 100644 testcases/kernel/mem/ksm/ksm01.c
>
> diff --git a/testcases/kernel/mem/ksm/Makefile b/testcases/kernel/mem/ksm/Makefile
> new file mode 100644
> index 0000000..d3eb6ef
> --- /dev/null
> +++ b/testcases/kernel/mem/ksm/Makefile
> @@ -0,0 +1,22 @@
> +#
> +# Copyright (c) International Business Machines Corp., 2001
Please fix the copyright date and owner.
...
> + * 1) ksm and ksmtuned daemons need to be disabled. Otherwise, it could
> + * distrub the testing as they also change some ksm tunables depends
*disturb
> + * on current workloads.
How does one do this manually? Documentation?
> + * The test steps are:
> + * - Check ksm feature and backup current run setting.
> + * - Change run setting to 1 - merging.
> + * - 3 memory allocation programs have the memory contents that 2 of
> + * them are all 'a' and one is all 'b'.
> + * - Check ksm statistics and verify the content.
> + * - 1 program changes the memory content from all 'a' to all 'b'.
> + * - Check ksm statistics and verify the content.
> + * - All programs change the memory content to all 'd'.
> + * - Check ksm statistics and verify the content.
> + * - Change one page of a process.
> + * - Check ksm statistics and verify the content.
> + * - Change run setting to 2 - unmerging.
> + * - Check ksm statistics and verify the content.
> + * - Change run setting to 0 - stop.
...
> + /* Check looping state if -i option given */
> + for (lc = 0; TEST_LOOPING(lc); lc++) {
> + /* Reset Tst_count in case we are looping. */
> + Tst_count = 0;
> +
> + TEST(ksmtest());
There's no sense in running this through the TEST macro if you aren't
going to use TEST_RETURN or TEST_ERRNO.
> + }
> + cleanup();
> + return 0;
...
> + memory = malloc(num * sizeof(**memory));
Why are you referencing memory twice and then allocating heap memory
to the pointer to memory? Seems really leaky.
> + if (memory == NULL)
> + tst_brkm(TBROK, cleanup, "malloc");
> +
> + if ((child[0] = fork()) == -1)
> + tst_brkm(TBROK|TERRNO, cleanup, "fork");
> + else if (child[0] == 0) {
> + tst_resm(TINFO, "child 0 stops.");
> + if (raise(SIGSTOP) == -1)
a.
switch ((child[0] = fork()) {
case -1:
/* BUSTED */
break;
case 0:
/* CHILD */
break;
default:
/* PARENT */
break;
}
Is generally a bit easier to follow.
b. Why not use kill(2)?
> + tst_brkm(TBROK|TERRNO, cleanup, "kill");
> +
> + tst_resm(TINFO, "child 0 continues...");
> +
> + /* Avoid THP allocation which can't ksm ATM. */
What does THP mean and why not just spell out ATM as "at the moment"?
Less acronyms -> less confusion :).
> + tst_resm(TINFO,
> + "child 0 allocates %d MB filled with 'c'.",
> + size);
> +
> + memory[0] = malloc(size * sizeof(*memory));
More leaky memory?
> + if (memory[0] == NULL)
> + tst_brkm(TBROK|TERRNO, cleanup, "malloc");
> +
> + for (j = 0; j < size; j++) {
> + memory[0][j] = mmap(NULL, MB,
> + PROT_READ|PROT_WRITE,
> + MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> + if (memory[0][j] == NULL)
I think you meant MAP_FAILED, not NULL.
> + tst_brkm(TBROK|TERRNO, cleanup, "mmap");
> + if (madvise(memory[0][j], MB, MADV_MERGEABLE)
> + == -1)
> + tst_brkm(TBROK|TERRNO, cleanup,
> + "madvise");
> + for (i = 0; i < MB; i++)
> + memory[0][j][i] = 'c';
You could just use memset.
> + }
> + tst_resm(TINFO, "child 0 stops.");
> + if (raise(SIGSTOP) == -1)
> + tst_brkm(TBROK|TERRNO, cleanup, "kill");
> +
> + tst_resm(TINFO, "child 0 continues...");
> + verify('c', 0, 0, size, 0, MB);
> + tst_resm(TINFO,
> + "child 0 changes memory content to 'd'.");
> +
> + for (j = 0; j < size; j++) {
> + for (i = 0; i < MB; i++)
> + memory[0][j][i] = 'd';
memset here too.
> + }
> + /* Unmerge. */
> + tst_resm(TINFO, "child 0 stops.");
> + if (raise(SIGSTOP) == -1)
kill? here too.
> + tst_brkm(TBROK|TERRNO, cleanup, "kill");
> +
> + tst_resm(TINFO, "child 0 continues...");
> + verify('d', 0, 0, size, 0, MB);
> +
> + /* Stop. */
> + tst_resm(TINFO, "child 0 stops.");
> + if (raise(SIGSTOP) == -1)
kill? here too.
> + tst_brkm(TBROK|TERRNO, cleanup, "kill");
> +
> + tst_resm(TINFO, "child 0 continues...");
> +
> + exit(0);
> + }
> + if ((child[1] = fork()) == -1)
> + tst_brkm(TBROK|TERRNO, cleanup, "fork");
> + else if (child[1] == 0) {
> + tst_resm(TINFO, "child 1 stops.");
> + if (raise(SIGSTOP) == -1)
Same comments as above about kill and fork-switch.
...
> + if (waitpid(child[k], &status, WUNTRACED) == -1)
Do WUNTRACED | WNOHANG and sleep each iteration in the loop to avoid
chewing up the CPU?
> + tst_brkm(TBROK|TERRNO, cleanup, "waitpid");
> + if (!WIFSTOPPED(status))
> + tst_brkm(TBROK, cleanup,
> + "child %d was not stopped.", k);
> + }
> +
> + tst_resm(TINFO, "resume all children.");
> + for (k = 0; k < num; k++) {
> + if (kill(child[k], SIGCONT) == -1)
> + tst_brkm(TBROK|TERRNO, cleanup,
> + "kill child[%d]", k);
> + }
> + sleep(5);
> + tst_resm(TINFO, "check!");
> + check("run", NULL, 1);
> + check("pages_shared", NULL, 2);
> + check("pages_sharing", "pages_volatile", size * num * 256 - 2);
> + check("pages_unshared", NULL, 0);
> + check("sleep_millisecs", NULL, 0);
> + check("pages_to_scan", NULL, size * 256 * num);
A lot of this logic is unnecessarily duplicated below; please move it
out into a function.
...
> + TEST_CLEANUP;
> + tst_rmdir();
Do you really need a temporary test directory? If not, I'd just drop
cleanup and set all references to it in tst_brkm to NULL.
> + tst_exit();
> +}
...
> + tst_brkm(TBROK, NULL, "stat");
TBROK | TERRNO please.
...
> + tst_resm(TINFO, "child %d verifies memory content.", proc);
> + for (j = start; j < end; j++)
> + for (i = start2; i < end2; i++)
> + if (memory[proc][j][i] != value)
> + tst_resm(TFAIL,
> + "child %d has %c at %d,%d,%d.",
> + proc, memory[proc][j][i], proc,
> + j, i);
memcmp'ing a fixed buffer should be considerably faster verifying
values byte by byte.
Thanks,
-Garrett
|