Menu
▾
▴
Re: [LTP] [PATCH v2] mmap/munmap /dev/zero
|
From: CAI Q. <ca...@re...> - 2010-10-13 04:22:02
|
----- "Garrett Cooper" <yan...@gm...> 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, <ca...@re...> 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 <ca...@re...>
>
> ...
>
> > +#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
|
