| Message ID | 20240326134742.526200-1-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | 25b191f6d33cda5770a18fd18be86cce0ebb3228 |
| Headers | show |
| Series | elf: Do not check for loader mmap on tst-decorate-maps (BZ 31553) | expand |
On mar. 26 mars 2024 10:47:42, Adhemerval Zanella wrote: > On some architectures and depending on the page size, the loader can > also allocate some memory during dependencies loading and it will be > marked as 'loader malloc'. However, if the system page size is > large enough, the initial data page will be enough for all required > allocation and there will be no extra loader mmap. To avoid false > negatives, the test does not check for such pages. > > Checked on powerpc64le-linux-gnu with 64k pagesize. Tested in similar conditions. Altogether it looks good, I just have one minor whitespace nitpick. > --- > elf/tst-decorate-maps.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/elf/tst-decorate-maps.c b/elf/tst-decorate-maps.c > index 85ba5ce939..6d04344ba2 100644 > --- a/elf/tst-decorate-maps.c > +++ b/elf/tst-decorate-maps.c > @@ -56,7 +56,6 @@ struct proc_maps_t > int n_user_threads; > int n_arenas; > int n_malloc_mmap; > - int n_loader_malloc_mmap; OK > }; > > static struct proc_maps_t > @@ -82,8 +81,12 @@ read_proc_maps (void) > r.n_arenas++; > else if (strstr (line, "[anon: glibc: malloc]") != NULL) > r.n_malloc_mmap++; > - else if (strstr (line, "[anon: glibc: loader malloc]") != NULL) > - r.n_loader_malloc_mmap++; > + /* On some architectures and depending on the page size, the loader can > + also allocate some memory during dependencies loading and it will be > + marked as 'loader malloc'. However, if the system page size is large > + enough, the initial data page will be enough for all required > + allocation and there will be no extra loader mmap. To avoid false > + negatives, the test does not check for such pages. */ Whitespace mixup here? The first line of the comment block appears to be using a different indentation to the rest of the block. Good call on leaving an explicit comment! > } > free (line); > xfclose (f); > @@ -148,8 +151,6 @@ do_test_threads (bool set_guard) > TEST_COMPARE (r.n_user_threads, num_user_threads); > TEST_COMPARE (r.n_arenas, expected_n_arenas); > TEST_COMPARE (r.n_malloc_mmap, 1); > - /* On some architectures the loader might use more than one page. */ > - TEST_VERIFY (r.n_loader_malloc_mmap >= 1); OK > } > > /* Let the threads finish. */ > @@ -164,7 +165,6 @@ do_test_threads (bool set_guard) > TEST_COMPARE (r.n_user_threads, 0); > TEST_COMPARE (r.n_arenas, expected_n_arenas); > TEST_COMPARE (r.n_malloc_mmap, 1); > - TEST_VERIFY (r.n_loader_malloc_mmap >= 1); OK > } > > free (p); > -- > 2.34.1 >
On 22/04/24 07:55, Simon Chopin wrote: > On mar. 26 mars 2024 10:47:42, Adhemerval Zanella wrote: >> On some architectures and depending on the page size, the loader can >> also allocate some memory during dependencies loading and it will be >> marked as 'loader malloc'. However, if the system page size is >> large enough, the initial data page will be enough for all required >> allocation and there will be no extra loader mmap. To avoid false >> negatives, the test does not check for such pages. >> >> Checked on powerpc64le-linux-gnu with 64k pagesize. > > Tested in similar conditions. > Altogether it looks good, I just have one minor whitespace nitpick. > >> --- >> elf/tst-decorate-maps.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/elf/tst-decorate-maps.c b/elf/tst-decorate-maps.c >> index 85ba5ce939..6d04344ba2 100644 >> --- a/elf/tst-decorate-maps.c >> +++ b/elf/tst-decorate-maps.c >> @@ -56,7 +56,6 @@ struct proc_maps_t >> int n_user_threads; >> int n_arenas; >> int n_malloc_mmap; >> - int n_loader_malloc_mmap; > > OK > >> }; >> >> static struct proc_maps_t >> @@ -82,8 +81,12 @@ read_proc_maps (void) >> r.n_arenas++; >> else if (strstr (line, "[anon: glibc: malloc]") != NULL) >> r.n_malloc_mmap++; >> - else if (strstr (line, "[anon: glibc: loader malloc]") != NULL) >> - r.n_loader_malloc_mmap++; >> + /* On some architectures and depending on the page size, the loader can >> + also allocate some memory during dependencies loading and it will be >> + marked as 'loader malloc'. However, if the system page size is large >> + enough, the initial data page will be enough for all required >> + allocation and there will be no extra loader mmap. To avoid false >> + negatives, the test does not check for such pages. */ > > Whitespace mixup here? The first line of the comment block appears to be > using a different indentation to the rest of the block. Hum, I used whitespace because it does not really fit for the indentation of the 'while'. The rest of patch use tab because it then allows it. > > Good call on leaving an explicit comment! > >> } >> free (line); >> xfclose (f); >> @@ -148,8 +151,6 @@ do_test_threads (bool set_guard) >> TEST_COMPARE (r.n_user_threads, num_user_threads); >> TEST_COMPARE (r.n_arenas, expected_n_arenas); >> TEST_COMPARE (r.n_malloc_mmap, 1); >> - /* On some architectures the loader might use more than one page. */ >> - TEST_VERIFY (r.n_loader_malloc_mmap >= 1); > > OK > >> } >> >> /* Let the threads finish. */ >> @@ -164,7 +165,6 @@ do_test_threads (bool set_guard) >> TEST_COMPARE (r.n_user_threads, 0); >> TEST_COMPARE (r.n_arenas, expected_n_arenas); >> TEST_COMPARE (r.n_malloc_mmap, 1); >> - TEST_VERIFY (r.n_loader_malloc_mmap >= 1); > > OK > >> } >> >> free (p); >> -- >> 2.34.1 >>
On lun. 22 avril 2024 10:11:47, Adhemerval Zanella Netto wrote: > On 22/04/24 07:55, Simon Chopin wrote: > > On mar. 26 mars 2024 10:47:42, Adhemerval Zanella wrote: > >> On some architectures and depending on the page size, the loader can > >> also allocate some memory during dependencies loading and it will be > >> marked as 'loader malloc'. However, if the system page size is > >> large enough, the initial data page will be enough for all required > >> allocation and there will be no extra loader mmap. To avoid false > >> negatives, the test does not check for such pages. > >> > >> Checked on powerpc64le-linux-gnu with 64k pagesize. > > > > Tested in similar conditions. > > Altogether it looks good, I just have one minor whitespace nitpick. > > Reviewed-by: Simon Chopin <simon.chopin@canonical.com> > >> --- > >> elf/tst-decorate-maps.c | 12 ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/elf/tst-decorate-maps.c b/elf/tst-decorate-maps.c > >> index 85ba5ce939..6d04344ba2 100644 > >> --- a/elf/tst-decorate-maps.c > >> +++ b/elf/tst-decorate-maps.c > >> @@ -56,7 +56,6 @@ struct proc_maps_t > >> int n_user_threads; > >> int n_arenas; > >> int n_malloc_mmap; > >> - int n_loader_malloc_mmap; > > > > OK > > > >> }; > >> > >> static struct proc_maps_t > >> @@ -82,8 +81,12 @@ read_proc_maps (void) > >> r.n_arenas++; > >> else if (strstr (line, "[anon: glibc: malloc]") != NULL) > >> r.n_malloc_mmap++; > >> - else if (strstr (line, "[anon: glibc: loader malloc]") != NULL) > >> - r.n_loader_malloc_mmap++; > >> + /* On some architectures and depending on the page size, the loader can > >> + also allocate some memory during dependencies loading and it will be > >> + marked as 'loader malloc'. However, if the system page size is large > >> + enough, the initial data page will be enough for all required > >> + allocation and there will be no extra loader mmap. To avoid false > >> + negatives, the test does not check for such pages. */ > > > > Whitespace mixup here? The first line of the comment block appears to be > > using a different indentation to the rest of the block. > > Hum, I used whitespace because it does not really fit for the indentation > of the 'while'. The rest of patch use tab because it then allows it. Fair enough. > > > > > Good call on leaving an explicit comment! > > > >> } > >> free (line); > >> xfclose (f); > >> @@ -148,8 +151,6 @@ do_test_threads (bool set_guard) > >> TEST_COMPARE (r.n_user_threads, num_user_threads); > >> TEST_COMPARE (r.n_arenas, expected_n_arenas); > >> TEST_COMPARE (r.n_malloc_mmap, 1); > >> - /* On some architectures the loader might use more than one page. */ > >> - TEST_VERIFY (r.n_loader_malloc_mmap >= 1); > > > > OK > > > >> } > >> > >> /* Let the threads finish. */ > >> @@ -164,7 +165,6 @@ do_test_threads (bool set_guard) > >> TEST_COMPARE (r.n_user_threads, 0); > >> TEST_COMPARE (r.n_arenas, expected_n_arenas); > >> TEST_COMPARE (r.n_malloc_mmap, 1); > >> - TEST_VERIFY (r.n_loader_malloc_mmap >= 1); > > > > OK > > > >> } > >> > >> free (p); > >> -- > >> 2.34.1 > >>
diff --git a/elf/tst-decorate-maps.c b/elf/tst-decorate-maps.c index 85ba5ce939..6d04344ba2 100644 --- a/elf/tst-decorate-maps.c +++ b/elf/tst-decorate-maps.c @@ -56,7 +56,6 @@ struct proc_maps_t int n_user_threads; int n_arenas; int n_malloc_mmap; - int n_loader_malloc_mmap; }; static struct proc_maps_t @@ -82,8 +81,12 @@ read_proc_maps (void) r.n_arenas++; else if (strstr (line, "[anon: glibc: malloc]") != NULL) r.n_malloc_mmap++; - else if (strstr (line, "[anon: glibc: loader malloc]") != NULL) - r.n_loader_malloc_mmap++; + /* On some architectures and depending on the page size, the loader can + also allocate some memory during dependencies loading and it will be + marked as 'loader malloc'. However, if the system page size is large + enough, the initial data page will be enough for all required + allocation and there will be no extra loader mmap. To avoid false + negatives, the test does not check for such pages. */ } free (line); xfclose (f); @@ -148,8 +151,6 @@ do_test_threads (bool set_guard) TEST_COMPARE (r.n_user_threads, num_user_threads); TEST_COMPARE (r.n_arenas, expected_n_arenas); TEST_COMPARE (r.n_malloc_mmap, 1); - /* On some architectures the loader might use more than one page. */ - TEST_VERIFY (r.n_loader_malloc_mmap >= 1); } /* Let the threads finish. */ @@ -164,7 +165,6 @@ do_test_threads (bool set_guard) TEST_COMPARE (r.n_user_threads, 0); TEST_COMPARE (r.n_arenas, expected_n_arenas); TEST_COMPARE (r.n_malloc_mmap, 1); - TEST_VERIFY (r.n_loader_malloc_mmap >= 1); } free (p);