> Can you provide documentation that describes the functionality under
> test?
In kernel docs,
http://lxr.linux.no/#linux+v2.6.35/Documentation/vm/ksm.txt
> How does one do this manually? Documentation?
In Fedora or similar systems, do,
# services ksm stop
# service ksmtuned stop
> There's no sense in running this through the TEST macro if you aren't
> going to use TEST_RETURN or TEST_ERRNO.
OK.
> > + memory = malloc(num * sizeof(**memory));
>
> Why are you referencing memory twice and then allocating heap memory
> to the pointer to memory? Seems really leaky.
This is a memory pointer to identify per process, MB, and byte like
memory[process No.][MB No.][byte No.]. Here, it is to allocate the memory
(memory[MB No.][byte No.]) for total number of processes. Then, it will
allocate memory for No. of MB per process and No. of bytes per MB. It is an
index to find out a single byte of memory content for a given process, so it is
possible to modify any single byte.
>
> > + 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.
OK.
> b. Why not use kill(2)?
raise() is simper. Does it matter?
>
> > + 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 :).
OK. THP is TransparentHugePage,
http://lwn.net/Articles/413083/
>
> > + tst_resm(TINFO,
> > + "child 0 allocates %d MB filled with 'c'.",
> > + size);
> > +
> > + memory[0] = malloc(size * sizeof(*memory));
>
> More leaky memory?
As mentioned above, this is the index for process 0 with No. of MB.
> > + 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)
OK
>
> I think you meant MAP_FAILED, not NULL.
Yes.
>
> > + 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.
It might be better to be consistent here, since there are times as you can see
in the later part of code to change the last byte which might still need the
direct modification here. In addition, memset() is likely to add more code
including return code checking and grateful exit etc.
>
> > + }
> > + 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.
Reasons given above.
>
> > + }
> > + /* Unmerge. */
> > + tst_resm(TINFO, "child 0 stops.");
> > + if (raise(SIGSTOP) == -1)
>
> kill? here too.
Reason given above.
>
> > + 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.
Same here.
>
> > + 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.
Yes, will fix 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?
Hmm, not sure about WNOHANG. The purpose here is to wait for every child to
complete, so it won't break the patent/child processing ordering that the test
depends on. The test code was designed, if there was a child not exit for some
reasons, it needs to further debugging to find out why.
> > + 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.
OK.
>
> ...
>
> > + 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.
OK.
>
> > + tst_exit();
> > +}
>
> ...
>
> > + tst_brkm(TBROK, NULL, "stat");
>
> TBROK | TERRNO please.
OK.
>
> ...
>
> > + 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.
OK. Thanks.
CAI Qian
------------------------------------------------------------------------------
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
