diff mbox series

elf: Do not check for loader mmap on tst-decorate-maps (BZ 31553)

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

Commit Message

Adhemerval Zanella March 26, 2024, 1:47 p.m. UTC
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.
---
 elf/tst-decorate-maps.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Simon Chopin April 22, 2024, 10:55 a.m. UTC | #1
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
>
Adhemerval Zanella April 22, 2024, 1:11 p.m. UTC | #2
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
>>
Simon Chopin April 22, 2024, 2:58 p.m. UTC | #3
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 mbox series

Patch

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);