On Wed, Nov 3, 2010 at 11:36 PM, CAI Qian <[email protected]> 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 <[email protected]>
> ---
> 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

------------------------------------------------------------------------------
The Next 800 Companies to Lead America's Growth: New Video Whitepaper
David G. Thomson, author of the best-selling book "Blueprint to a 
Billion" shares his insights and actions to help propel your 
business during the next growth cycle. Listen Now!
http://p.sf.net/sfu/SAP-dev2dev
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Reply via email to