You've filled in some of the blanks, but there are some that are still missing...
On Tue, Oct 12, 2010 at 7:02 PM, <[email protected]> wrote: > v2: incorporate comments from Mike and Garrett. > > Add a new test to mmap/munmap /dev/zero. A common way of malloc()/free() > anonymous memory on Solaris. The first sentence should standalone as the summary, like: Add a new test to mmap/munmap /dev/zero [fill in reason why it's being tested -- try to summarize in less than 80 columns]. Right now it's a dangling modifier. The second sentence is a fragment; you are describing the process, but not describing the important actors or involved parties in the process (in particular on the Linux side). There's also little context between the two sentences, so it's confusing to the end reader. > Anyway, You can remove Anyway, > mmap() memory mapping > of unnecessary particle. > /dev/zero results in calling map_zero() which on RHEL5 maps the ZERO_PAGE in > every pte within that virtual address range. PTE is an acronym (please capitalize). > Since a application is also multi-threaded the subsequent munmap() of > /dev/zero results is TLB shootdowns to all other CPUs. Why isn't the test multithreaded then? How do you measure the poor performance resulting from all of the anonymous page TLB shootdowns? > When this happens thousands or millions of times the application performance > is terrible. Well, TLB shootdown hurts no matter how many times it occurs, so I wouldn't doubt that thousands or millions or times would hurt more. > The mapping ZERO_PAGE in every pte within that virtual address range was an > optimization to make the subsequent pagefault Ok. I think I have an idea of what you're trying to achieve based on this statement, but I'm not quite sure. Are you saying that anonymous mapped pages without data should have an equivalent address to a shared zeroed out page? If so how do you know what the master zeroed out page is? Why aren't any memory address comparisons being performed below? Where's the performance trend data to prove that the feature delivers as per its claimed performance benefits? > times faster on RHEL5 that. How much faster? > has been removed/changed upstream. Reference? > Signed-off-by: CAI Qian <[email protected]> ... > +#include "test.h" > +#include "usctest.h" > +#include <errno.h> > +#include <sys/mman.h> > +#include <unistd.h> > +#include <stdlib.h> > +#include <sys/wait.h> > +#include <stdio.h> It's not linux style (BSD style(9) to be exact), but could you at least sort the headers by: sys/ system headers -> non-sys/ system headers -> `user defined' headers in alphabetical order...? it's much easier to read. > +#define SIZE (5*1024*1024) Why 5MB? > +char *TCID = "mmap10"; > +int TST_TOTAL = 1; > +extern int Tst_count; > + > +/* main setup function of test */ > +void setup(void); > +/* cleanup function for the test */ > +void cleanup(void); > +int mmapzero(void); > + > +int main(int argc, char *argv[]) > +{ > + /* loop counter */ > + int lc; > + /* message returned from parse_opts */ > + char *msg; > + > + msg = parse_opts(argc, argv, (option_t *) NULL, NULL); > + if (msg != NULL) { > + tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg); > + tst_exit(); > + } > + > + /* Perform global setup for test */ > + setup(); > + > + /* 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(mmapzero()); > + > + if (TEST_RETURN != 0) > + tst_resm(TFAIL, "mmapzero() failed with %ld.", > + TEST_RETURN); TEST_RETURN is an integer, not a long integer, correct (hence the %ld could be a %d)? > + else > + tst_resm(TPASS, "mmapzero() completed successfully."); > + } > + > + cleanup(); > + return 0; > +} > + > +int mmapzero(void) > +{ > + char *x; > + int n; > + > + x = mmap("/dev/zero", SIZE+SIZE-4096, PROT_READ|PROT_WRITE, > + MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); > + if (x == MAP_FAILED) { > + tst_resm(TFAIL, "mmap: %s", strerror(errno)); > + return 1; > + } > + x[SIZE] = 0; > + n = fork(); > + if (n == -1) > + tst_brkm(TBROK, cleanup, "fork: %s", strerror(errno)); tst_brkm(TBROK|TERRNO, cleanup, "fork failed"); etc, would be better than using tst_brkm with strerror. > + else if (n == 0) { > + if (munmap(x + SIZE+4096, SIZE-4096*2) == -1) > + tst_resm(TFAIL, "munmap: %s", strerror(errno)); > + _exit(0); > + } > + n = fork(); > + if (n == -1) > + tst_brkm(TBROK, cleanup, "fork: %s", strerror(errno)); > + else if (n == 0) { > + if (munmap(x + SIZE+4096, SIZE-4096*2) == -1) > + tst_resm(TFAIL, "munmap: %s", strerror(errno)); > + _exit(0); What are you trying to achieve by munmap'ing the same section of memory in a forked process [twice] and the parent process in the end? They're 3 anonymous COW backed pages because you specified MAP_PRIVATE | MAP_ANONYMOUS ... > + } else { > + n = fork(); > + if (n == -1) > + tst_brkm(TBROK, cleanup, "fork: %s", strerror(errno)); > + else if (n == 0) { > + if (munmap(x + SIZE+4096, SIZE-4096*2) == -1) > + tst_resm(TFAIL, "munmap: %s", > strerror(errno)); > + _exit(0); > + } > + } > + if (munmap(x, SIZE+SIZE-4096) == -1) > + tst_resm(TFAIL, "munmap: %s", strerror(errno)); > + while (waitpid(-1, NULL, WNOHANG) > 0); > + return 0; This test isn't written according to the description. Please better address what you're trying to achieve so that we might be able to help you reach a conclusion. Thanks, -Garrett ------------------------------------------------------------------------------ Beautiful is writing same markup. Internet Explorer 9 supports standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3. Spend less time writing and rewriting code and more time creating great experiences on the web. Be a part of the beta today. http://p.sf.net/sfu/beautyoftheweb _______________________________________________ Ltp-list mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ltp-list
