----- "Garrett Cooper" <[email protected]> wrote: > 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 It is important to test this scenarios because it is a common way of malloc()/free() anonymous memory on Solaris. > > 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)? OK, I am not familiar with this internal variable. > > > + 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. OK. > > > + 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 ... I'll explain in v4. > > > + } 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. v4 followed. > 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
