| Message ID | 87a7e2p01f.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Linux: Use mmap instead of malloc in dirent/tst-getdents64 | expand |
On 28/06/2019 05:49, Florian Weimer wrote: > malloc dirties the entire allocated memory region due to M_PERTURB > in the test harness. > > 2019-06-28 Florian Weimer <fweimer@redhat.com> > > * sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks): > Use mmap instead of malloc. malloc with M_PERTURB writes to the > entire allocated memory range. LGTM. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c > index 24e77e04d8..8a28e6c67c 100644 > --- a/sysdeps/unix/sysv/linux/tst-getdents64.c > +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c > @@ -27,6 +27,7 @@ > #include <support/check.h> > #include <support/support.h> > #include <support/xunistd.h> > +#include <sys/mman.h> > #include <unistd.h> > > /* Called by large_buffer_checks below. */ > @@ -53,8 +54,13 @@ large_buffer_checks (int fd) > size_t large_buffer_size; > if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size)) > { > - char *large_buffer = malloc (large_buffer_size); > - if (large_buffer == NULL) > + int flags = MAP_ANONYMOUS | MAP_PRIVATE; > +#ifdef MAP_NORESERVE > + flags |= MAP_NORESERVE; > +#endif > + void *large_buffer = mmap (NULL, large_buffer_size, > + PROT_READ | PROT_WRITE, flags, -1, 0); > + if (large_buffer == MAP_FAILED) Should we really skip the test instead of using xmmap? The only case I think of that it might fail is if kernel can't allocate bookeeping memory for the page table itself, but I really think it unlikely (sanitizer usually allocates a lot of VMA as well). > printf ("warning: could not allocate %zu bytes of memory," > " subtests skipped\n", large_buffer_size); > else > @@ -65,8 +71,8 @@ large_buffer_checks (int fd) > large_buffer_check (fd, large_buffer, UINT_MAX); > large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 1); > large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 2); > + xmunmap (large_buffer, large_buffer_size); > } > - free (large_buffer); > } > } > >
* Adhemerval Zanella: > On 28/06/2019 05:49, Florian Weimer wrote: >> malloc dirties the entire allocated memory region due to M_PERTURB >> in the test harness. >> >> 2019-06-28 Florian Weimer <fweimer@redhat.com> >> >> * sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks): >> Use mmap instead of malloc. malloc with M_PERTURB writes to the >> entire allocated memory range. > > LGTM. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> Thanks! >> diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c >> index 24e77e04d8..8a28e6c67c 100644 >> --- a/sysdeps/unix/sysv/linux/tst-getdents64.c >> +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c >> @@ -27,6 +27,7 @@ >> #include <support/check.h> >> #include <support/support.h> >> #include <support/xunistd.h> >> +#include <sys/mman.h> >> #include <unistd.h> >> >> /* Called by large_buffer_checks below. */ >> @@ -53,8 +54,13 @@ large_buffer_checks (int fd) >> size_t large_buffer_size; >> if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size)) >> { >> - char *large_buffer = malloc (large_buffer_size); >> - if (large_buffer == NULL) >> + int flags = MAP_ANONYMOUS | MAP_PRIVATE; >> +#ifdef MAP_NORESERVE >> + flags |= MAP_NORESERVE; >> +#endif >> + void *large_buffer = mmap (NULL, large_buffer_size, >> + PROT_READ | PROT_WRITE, flags, -1, 0); >> + if (large_buffer == MAP_FAILED) > > Should we really skip the test instead of using xmmap? The only case I think of > that it might fail is if kernel can't allocate bookeeping memory for the page > table itself, but I really think it unlikely (sanitizer usually allocates a > lot of VMA as well). MAP_NORESERVE is a misnomer; it does not do what it says. It disables the overcommit heuristics for vm.overcommit_memory=0, but not for vm.overcommit_memory=2. mmap can also fail due to resource limits on address space, not residential memory. So I think the code above is the best we can do. Thanks, Florian
On 28/06/2019 09:04, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 28/06/2019 05:49, Florian Weimer wrote: >>> malloc dirties the entire allocated memory region due to M_PERTURB >>> in the test harness. >>> >>> 2019-06-28 Florian Weimer <fweimer@redhat.com> >>> >>> * sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks): >>> Use mmap instead of malloc. malloc with M_PERTURB writes to the >>> entire allocated memory range. >> >> LGTM. >> >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > Thanks! > >>> diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c >>> index 24e77e04d8..8a28e6c67c 100644 >>> --- a/sysdeps/unix/sysv/linux/tst-getdents64.c >>> +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c >>> @@ -27,6 +27,7 @@ >>> #include <support/check.h> >>> #include <support/support.h> >>> #include <support/xunistd.h> >>> +#include <sys/mman.h> >>> #include <unistd.h> >>> >>> /* Called by large_buffer_checks below. */ >>> @@ -53,8 +54,13 @@ large_buffer_checks (int fd) >>> size_t large_buffer_size; >>> if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size)) >>> { >>> - char *large_buffer = malloc (large_buffer_size); >>> - if (large_buffer == NULL) >>> + int flags = MAP_ANONYMOUS | MAP_PRIVATE; >>> +#ifdef MAP_NORESERVE >>> + flags |= MAP_NORESERVE; >>> +#endif >>> + void *large_buffer = mmap (NULL, large_buffer_size, >>> + PROT_READ | PROT_WRITE, flags, -1, 0); >>> + if (large_buffer == MAP_FAILED) >> >> Should we really skip the test instead of using xmmap? The only case I think of >> that it might fail is if kernel can't allocate bookeeping memory for the page >> table itself, but I really think it unlikely (sanitizer usually allocates a >> lot of VMA as well). > > MAP_NORESERVE is a misnomer; it does not do what it says. It disables > the overcommit heuristics for vm.overcommit_memory=0, but not for > vm.overcommit_memory=2. mmap can also fail due to resource limits on > address space, not residential memory. So I think the code above is the > best we can do. Alright, but my point is for usual 64-bit architecture (which the test are actually exercised) mapping 4GB of memory shouldn't really be a problem. The change looks ok, I would just like to make is more explicit that the tests passed, but not all sub-tests ran.
* Adhemerval Zanella: > Alright, but my point is for usual 64-bit architecture (which the test > are actually exercised) mapping 4GB of memory shouldn't really be a > problem. Our z/VM guests have 2 GiB RAM assigned to them. > The change looks ok, I would just like to make is more explicit > that the tests passed, but not all sub-tests ran. It's just a warning, not FAIL_UNSUPPORTED: if (large_buffer == MAP_FAILED) printf ("warning: could not allocate %zu bytes of memory," " subtests skipped\n", large_buffer_size); If you don't like that, we'd have to split it into a separate test, I think. Thanks, Florian
On 28/06/2019 11:52, Florian Weimer wrote: > * Adhemerval Zanella: > >> Alright, but my point is for usual 64-bit architecture (which the test >> are actually exercised) mapping 4GB of memory shouldn't really be a >> problem. > > Our z/VM guests have 2 GiB RAM assigned to them. > >> The change looks ok, I would just like to make is more explicit >> that the tests passed, but not all sub-tests ran. > > It's just a warning, not FAIL_UNSUPPORTED: > > if (large_buffer == MAP_FAILED) > printf ("warning: could not allocate %zu bytes of memory," > " subtests skipped\n", large_buffer_size); > > If you don't like that, we'd have to split it into a separate test, I > think. I think we are ok for now, maybe in future we might want to add a new test return code (SKIP or something).
28.06.2019 10:49 Florian Weimer <fweimer@redhat.com> wrote: > > malloc dirties the entire allocated memory region due to M_PERTURB > in the test harness. > > 2019-06-28 Florian Weimer <fweimer@redhat.com> > > * sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks): > Use mmap instead of malloc. malloc with M_PERTURB writes to the > entire allocated memory range. Thank you, Florian. I would like to confirm that now my make check runs correctly. Regards, Rafal
diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c index 24e77e04d8..8a28e6c67c 100644 --- a/sysdeps/unix/sysv/linux/tst-getdents64.c +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c @@ -27,6 +27,7 @@ #include <support/check.h> #include <support/support.h> #include <support/xunistd.h> +#include <sys/mman.h> #include <unistd.h> /* Called by large_buffer_checks below. */ @@ -53,8 +54,13 @@ large_buffer_checks (int fd) size_t large_buffer_size; if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size)) { - char *large_buffer = malloc (large_buffer_size); - if (large_buffer == NULL) + int flags = MAP_ANONYMOUS | MAP_PRIVATE; +#ifdef MAP_NORESERVE + flags |= MAP_NORESERVE; +#endif + void *large_buffer = mmap (NULL, large_buffer_size, + PROT_READ | PROT_WRITE, flags, -1, 0); + if (large_buffer == MAP_FAILED) printf ("warning: could not allocate %zu bytes of memory," " subtests skipped\n", large_buffer_size); else @@ -65,8 +71,8 @@ large_buffer_checks (int fd) large_buffer_check (fd, large_buffer, UINT_MAX); large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 1); large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 2); + xmunmap (large_buffer, large_buffer_size); } - free (large_buffer); } }