[V2,1/2] malloc: avoid global locks in tst-aligned_alloc-lib.c

Message ID 34dfe9283ec91c98d6a6c721251d5f1546e7c06c.1719994082.git.mmartinv@redhat.com
State Superseded
Headers
Series malloc: add multi-threaded tests for aligned_alloc/calloc/malloc |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed

Commit Message

Miguel Martín July 3, 2024, 8:11 a.m. UTC
  Make sure the DSO used by aligned_alloc/calloc/malloc tests does not get
a global lock on multithreaded tests.
---
 malloc/tst-aligned_alloc-lib.c | 39 +++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 19 deletions(-)
  

Comments

Arjun Shankar July 11, 2024, 5:41 p.m. UTC | #1
Hi Miguel,

Thanks! This looks good to me. I'll review patch #2 of the series
shortly. For this one:

Reviewed-by: Arjun Shankar <arjun@redhat.com>

Review follows:

> Make sure the DSO used by aligned_alloc/calloc/malloc tests does not get
> a global lock on multithreaded tests.

OK. The intent here is that the DSO does no other locking other than
what's needed and internally managed by the memory allocator. Earlier,
it was locking on "random" which takes a global lock just to generate
a random number, thus serializing any potential multi-threaded testing
on that lock.

> ---
>  malloc/tst-aligned_alloc-lib.c | 39 +++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/malloc/tst-aligned_alloc-lib.c b/malloc/tst-aligned_alloc-lib.c
> index 0205df5acf..9ef1f839c1 100644
> --- a/malloc/tst-aligned_alloc-lib.c
> +++ b/malloc/tst-aligned_alloc-lib.c
> @@ -17,37 +17,38 @@
>     License along with the GNU C Library; see the file COPYING.LIB.  If
>     not, see <https://www.gnu.org/licenses/>.  */
>
> -#include <array_length.h>

OK. We won't need this because we drop the function that uses it.

>  #include <libc-symbols.h>
>  #include <stdlib.h>
> +#include <time.h>

OK. We will use this to generate seeds.

>
>  extern void *__libc_malloc (size_t size);
>  extern void *__libc_calloc (size_t n, size_t size);
>
> +__thread unsigned int seed = 0;
> +

OK. Thread-local per-thread seed.

>  int aligned_alloc_count = 0;
>  int libc_malloc_count = 0;
>  int libc_calloc_count = 0;
>
> -/* Get a random alignment value.  Biased towards the smaller values.  Must be
> -   a power of 2. */
> -static size_t get_random_alignment (void)
> -{
> -  size_t aligns[] = {
> -    1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384
> -  };
> -
> -  return aligns[random () % array_length (aligns)];
> -}
> -

OK. Removed function. This function was (1) using a somewhat
unnecessary array, (2) calling "random" which takes a global lock.

> -static void *get_random_alloc (size_t size)
> +static void *
> +get_random_alloc (size_t size)

OK. Indent fix.

>  {
>    void *retval;
>    size_t align;
> +  struct timespec tp;
> +
> +  if (seed == 0)
> +    {
> +      clock_gettime (CLOCK_REALTIME, &tp);
> +      seed = tp.tv_nsec;
> +    }

OK. If this thread doesn't yet have a seed, we use the nanoseconds
time to generate one.

>
> -  switch (random() % 3)
> -  {
> +  switch (rand_r (&seed) % 3)
> +    {

OK. rand_r is reentrant and we use a thread-local seed. The brace
indent fix is good too.

>      case 1:
> -      align = get_random_alignment ();
> +      /* Get a random alignment value.  Biased towards the smaller
> +       * values up to 16384. Must be a power of 2. */
> +      align = 1 << rand_r (&seed) % 15;

OK. We dropped get_random_alignment. We now use rand_r, and we
continue to use the same range of alignments, and the exponential
growth means that we do indeed bias towards smaller values.

>        retval = aligned_alloc (align, size);
>        aligned_alloc_count++;
>        break;
> @@ -59,13 +60,13 @@ static void *get_random_alloc (size_t size)
>        retval = __libc_malloc (size);
>        libc_malloc_count++;
>        break;
> -  }
> +    }

OK. Brace indent fix.

>
>    return retval;
>  }
>
> -
> -void * __random_malloc (size_t size)
> +void *
> +__random_malloc (size_t size)

OK. Indent fix.

>  {
>    return get_random_alloc (size);
>  }
  

Patch

diff --git a/malloc/tst-aligned_alloc-lib.c b/malloc/tst-aligned_alloc-lib.c
index 0205df5acf..9ef1f839c1 100644
--- a/malloc/tst-aligned_alloc-lib.c
+++ b/malloc/tst-aligned_alloc-lib.c
@@ -17,37 +17,38 @@ 
    License along with the GNU C Library; see the file COPYING.LIB.  If
    not, see <https://www.gnu.org/licenses/>.  */
 
-#include <array_length.h>
 #include <libc-symbols.h>
 #include <stdlib.h>
+#include <time.h>
 
 extern void *__libc_malloc (size_t size);
 extern void *__libc_calloc (size_t n, size_t size);
 
+__thread unsigned int seed = 0;
+
 int aligned_alloc_count = 0;
 int libc_malloc_count = 0;
 int libc_calloc_count = 0;
 
-/* Get a random alignment value.  Biased towards the smaller values.  Must be
-   a power of 2. */
-static size_t get_random_alignment (void)
-{
-  size_t aligns[] = {
-    1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384
-  };
-
-  return aligns[random () % array_length (aligns)];
-}
-
-static void *get_random_alloc (size_t size)
+static void *
+get_random_alloc (size_t size)
 {
   void *retval;
   size_t align;
+  struct timespec tp;
+
+  if (seed == 0)
+    {
+      clock_gettime (CLOCK_REALTIME, &tp);
+      seed = tp.tv_nsec;
+    }
 
-  switch (random() % 3)
-  {
+  switch (rand_r (&seed) % 3)
+    {
     case 1:
-      align = get_random_alignment ();
+      /* Get a random alignment value.  Biased towards the smaller
+       * values up to 16384. Must be a power of 2. */
+      align = 1 << rand_r (&seed) % 15;
       retval = aligned_alloc (align, size);
       aligned_alloc_count++;
       break;
@@ -59,13 +60,13 @@  static void *get_random_alloc (size_t size)
       retval = __libc_malloc (size);
       libc_malloc_count++;
       break;
-  }
+    }
 
   return retval;
 }
 
-
-void * __random_malloc (size_t size)
+void *
+__random_malloc (size_t size)
 {
   return get_random_alloc (size);
 }