[V2,2/2] malloc: add multi-threaded tests for aligned_alloc/calloc/malloc

Message ID cfb087a39b9546c3818b08e461ee65f72c9b841c.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
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test 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
  Improve aligned_alloc/calloc/malloc test coverage by adding
multi-threaded tests with random memory allocations and with/without
cross-thread memory deallocations.

Perform a number of memory allocation calls with a randomly sized,
but limited to 0xffff, requests.

Use the existing DSO ('malloc/tst-aligned_alloc-lib.c') to randomize
allocator selection.

The multi-threaded allocation/deallocation is staged as described below:

- Stage 1: Half of the threads will be allocating memory and the
  other half will be waiting for them to finish the allocation.
- Stage 2: Half of the threads will be allocating memory and the
  other half will be deallocating memory.
- Stage 3: Half of the threads will be deallocating memory and the
  second half waiting on the to finish.

Add 'malloc/tst-aligned-alloc-random-thread.c' where each thread will
deallocate only the memory that was previously allocated by itself.

Add 'malloc/tst-aligned-alloc-random-thread-cross.c' where each thread
will deallocate memory that was previously allocated by another thread.

The intention is to be able to utilize existing malloc testing to ensure
that similar allocation APIs are also exposed to the same rigors.
---
 malloc/Makefile                               |   8 +
 .../tst-aligned-alloc-random-thread-cross.c   |  19 +++
 malloc/tst-aligned-alloc-random-thread.c      | 150 ++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 malloc/tst-aligned-alloc-random-thread-cross.c
 create mode 100644 malloc/tst-aligned-alloc-random-thread.c
  

Comments

Arjun Shankar July 12, 2024, 4:50 p.m. UTC | #1
Hi Miguel,

Thanks for working on this! Overall, this looks good to me. I have a
few comments below.

> Improve aligned_alloc/calloc/malloc test coverage by adding
> multi-threaded tests with random memory allocations and with/without
> cross-thread memory deallocations.
>
> Perform a number of memory allocation calls with a randomly sized,
> but limited to 0xffff, requests.

I suggest rewording to "... allocation calls with random sizes limited
to 0xffff".

> Use the existing DSO ('malloc/tst-aligned_alloc-lib.c') to randomize
> allocator selection.
>
> The multi-threaded allocation/deallocation is staged as described below:
>
> - Stage 1: Half of the threads will be allocating memory and the
>   other half will be waiting for them to finish the allocation.
> - Stage 2: Half of the threads will be allocating memory and the
>   other half will be deallocating memory.
> - Stage 3: Half of the threads will be deallocating memory and the
>   second half waiting on the to finish.

Typo; should be "them to finish".

> Add 'malloc/tst-aligned-alloc-random-thread.c' where each thread will
> deallocate only the memory that was previously allocated by itself.
>
> Add 'malloc/tst-aligned-alloc-random-thread-cross.c' where each thread
> will deallocate memory that was previously allocated by another thread.
>
> The intention is to be able to utilize existing malloc testing to ensure
> that similar allocation APIs are also exposed to the same rigors.

OK.

> ---
>  malloc/Makefile                               |   8 +
>  .../tst-aligned-alloc-random-thread-cross.c   |  19 +++
>  malloc/tst-aligned-alloc-random-thread.c      | 150 ++++++++++++++++++
>  3 files changed, 177 insertions(+)
>  create mode 100644 malloc/tst-aligned-alloc-random-thread-cross.c
>  create mode 100644 malloc/tst-aligned-alloc-random-thread.c
>
> diff --git a/malloc/Makefile b/malloc/Makefile
> index 02aff1bd1d..98d507a6eb 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -28,6 +28,8 @@ tests := \
>    mallocbug \
>    tst-aligned-alloc \
>    tst-aligned-alloc-random \
> +  tst-aligned-alloc-random-thread \
> +  tst-aligned-alloc-random-thread-cross \

OK. Two new tests.

>    tst-alloc_buffer \
>    tst-calloc \
>    tst-free-errno \
> @@ -151,6 +153,8 @@ ifeq ($(have-GLIBC_2.23)$(build-shared),yesyes)
>  # the tests expect specific internal behavior that is changed due to linking to
>  # libmcheck.a.
>  tests-exclude-mcheck = \
> +  tst-aligned-alloc-random-thread \
> +  tst-aligned-alloc-random-thread-cross \

OK. Both are excluded from running mcheck variants because of the
mcheck issue you just filed:

https://sourceware.org/bugzilla/show_bug.cgi?id=31957

>    tst-compathooks-off \
>    tst-compathooks-on \
>    tst-malloc-backtrace \
> @@ -415,7 +419,11 @@ $(objpfx)tst-mallocstate: $(objpfx)libc_malloc_debug.so
>  $(objpfx)tst-mallocstate-malloc-check: $(objpfx)libc_malloc_debug.so
>
>  $(objpfx)tst-aligned-alloc-random.out: $(objpfx)tst-aligned_alloc-lib.so
> +$(objpfx)tst-aligned-alloc-random-thread.out: $(objpfx)tst-aligned_alloc-lib.so
> +$(objpfx)tst-aligned-alloc-random-thread-cross.out: $(objpfx)tst-aligned_alloc-lib.so
>  $(objpfx)tst-malloc-random.out: $(objpfx)tst-aligned_alloc-lib.so

OK. Test execution depends on the DSO.

>
>  tst-aligned-alloc-random-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
> +tst-aligned-alloc-random-thread-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
> +tst-aligned-alloc-random-thread-cross-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
>  tst-malloc-random-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so

OK. Preload the library to randomize allocators.

> diff --git a/malloc/tst-aligned-alloc-random-thread-cross.c b/malloc/tst-aligned-alloc-random-thread-cross.c
> new file mode 100644
> index 0000000000..360ecc56ee
> --- /dev/null
> +++ b/malloc/tst-aligned-alloc-random-thread-cross.c
> @@ -0,0 +1,19 @@
> +/* multi-threaded memory allocation and cross-thread deallocation test.

OK.

> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public License as
> +   published by the Free Software Foundation; either version 2.1 of the
> +   License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; see the file COPYING.LIB.  If
> +   not, see <https://www.gnu.org/licenses/>.  */
> +#define CROSS_THREAD_DEALLOC
> +#include "tst-aligned-alloc-random-thread.c"

OK. Sharing code with the other test. This define should make the
deallocations cross-threaded.

> diff --git a/malloc/tst-aligned-alloc-random-thread.c b/malloc/tst-aligned-alloc-random-thread.c
> new file mode 100644
> index 0000000000..ca7b1588d4
> --- /dev/null
> +++ b/malloc/tst-aligned-alloc-random-thread.c
> @@ -0,0 +1,150 @@
> +/* multi-threaded memory allocation/deallocation test.

OK.

> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public License as
> +   published by the Free Software Foundation; either version 2.1 of the
> +   License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; see the file COPYING.LIB.  If
> +   not, see <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/xthread.h>
> +#include <support/test-driver.h>
> +#include <sys/sysinfo.h>
> +#include <unistd.h>
> +
> +#ifndef ITERATIONS
> +#  define ITERATIONS 16
> +#endif
> +
> +#ifndef NUM_THREADS
> +#  define NUM_THREADS 8
> +#endif
> +
> +#ifndef NUM_ALLOCATIONS
> +#  define NUM_ALLOCATIONS 2048
> +#endif
> +
> +static pthread_barrier_t barrier;
> +
> +__thread unsigned int seed;
> +
> +typedef struct
> +{
> +  int id;
> +  pthread_t thread;
> +} thread;
> +
> +thread threads[NUM_THREADS];
> +
> +void *allocations[NUM_THREADS][NUM_ALLOCATIONS];

OK.

> +
> +void
> +run_thread_dealloc (int id)
> +{
> +  for (int i = 0; i < NUM_ALLOCATIONS; i++)
> +    {
> +      free (allocations[id][i]);
> +      allocations[id][i] = NULL;
> +    }
> +}

OK. The allocations of a thread are associated with its ID, and this
deallocates everything allocated by thread# ID.

> +
> +void
> +run_thread_alloc (int id)
> +{
> +  size_t size;
> +  for (int i = 0; i < NUM_ALLOCATIONS; i++)
> +    {
> +      size = rand_r (&seed) & 0xffff;

This seems to tend to lead to lots of mostly large allocations. 75% of
allocations are over 16K, with 50% over 32K size. I'm wondering if we
should be changing this up a bit to reduce the tendency for such large
allocations.

> +      allocations[id][i] = malloc (size);

OK. The allocator will actually be randomly changed out from under
here by the preloaded library.

> +      TEST_VERIFY_EXIT (allocations[id][i] != NULL);
> +    }
> +}

> +
> +void *
> +run_allocations (void *arg)
> +{
> +  int id = *((int *) arg);
> +  seed = time (NULL) + id;

OK. Considering we have the ID here, this looks like a reasonable seed init.

> +
> +  /* Stage 1: First half o the threads allocating memory and the second
> +   * half waiting for them to finish
> +   */
> +  if (id < NUM_THREADS / 2)
> +    run_thread_alloc (id);

OK.

> +
> +  xpthread_barrier_wait (&barrier);

OK. Half the threads do their allocation work, then everyone synchronizes here.

> +
> +  /* Stage 2: Half of the threads allocationg memory and the other
> +   * half deallocating:
> +   * - In the non cross-thread dealloc scenario the first half will be
> +   *   deallocating the memory allocated by themselves in stage 1 and the
> +   *   second half will be allocating memory.
> +   * - In the cross-thread dealloc scenario the first half will continue
> +   *   to allocate memory and the second half will deallocate the memory
> +   *   allocated by the first half in stage 1.
> +   */
> +  if (id < NUM_THREADS / 2)

OK. This bit is going to be about the first half of threads (which
just did allocations in stage 1).

> +#ifndef CROSS_THREAD_DEALLOC
> +    run_thread_dealloc (id);

OK. They deallocate their own blocks.

> +#else
> +    run_thread_alloc (id + NUM_THREADS / 2);
> +#endif

OK. In cross-thread deallocation mode, they do more allocations into
unallocated blocks "belonging" to the second half of the thread pool.

> +  else

OK. This next bit is about what the second half of threads do.

> +#ifndef CROSS_THREAD_DEALLOC
> +    run_thread_alloc (id);

OK. Normally, they do their own allocations.

> +#else
> +    run_thread_dealloc (id - NUM_THREADS / 2);
> +#endif

OK. In cross-thread deallocation mode, they deallocate what their
counterpart threads in the first half of the thread pool allocated in
stage 1.

> +
> +  xpthread_barrier_wait (&barrier);

OK. Then everyone synchronizes.

> +
> +  // Stage 3: Second half of the threads deallocating and the first half
> +  // waiting for them to finish.
> +  if (id >= NUM_THREADS / 2)

OK. We are in stage 3 now, and this is the *second* half of threads.

> +    run_thread_dealloc (id);

OK. They deallocate whatever was allocated into their own blocks.

In regular deallocation mode, this means they will deallocated what
they themselves allocated in stage 2. But in cross-thread deallocation
mode, this means they will be deallocating what the first half of the
thread pool allocated into the second half's pointer array in stage 2.

> +
> +  return NULL;
> +}

This should probably just return nothing and have a void return type.

> +
> +static int
> +do_test (void)
> +{
> +  xpthread_barrier_init (&barrier, NULL, NUM_THREADS);

OK.

> +
> +  for (int i = 0; i < ITERATIONS; i++)

OK. We run through the stages many times.

> +    {
> +      for (int t = 0; t < NUM_THREADS; t++)
> +       {
> +         threads[t].id = t;
> +         threads[t].thread
> +             = xpthread_create (NULL, run_allocations, &threads[t].id);
> +       }
> +
> +      for (int t = 0; t < NUM_THREADS; t++)
> +       {
> +         xpthread_join (threads[t].thread);
> +
> +         // Check everything was freed
> +         for (int a = 0; a < NUM_ALLOCATIONS; a++)
> +           TEST_VERIFY_EXIT (allocations[t][a] == NULL);

I'd say we don't need this check. If this fails, it wouldn't be
because of the allocator but rather something very seriously wrong
elsewhere.

> +       }
> +    }
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> --
> Miguel Martín
> mmartinv@redhat.com
>


--
Arjun Shankar
he/him/his
  
Miguel Martín July 15, 2024, 9:03 a.m. UTC | #2
On 2024-07-12, Arjun Shankar wrote:
> Hi Miguel,
> 
> Thanks for working on this! Overall, this looks good to me. I have a
> few comments below.
> 
> > Improve aligned_alloc/calloc/malloc test coverage by adding
> > multi-threaded tests with random memory allocations and with/without
> > cross-thread memory deallocations.
> >
> > Perform a number of memory allocation calls with a randomly sized,
> > but limited to 0xffff, requests.
> 
> I suggest rewording to "... allocation calls with random sizes limited
> to 0xffff".
> 

OK. I will reword it in the next version.

> > Use the existing DSO ('malloc/tst-aligned_alloc-lib.c') to randomize
> > allocator selection.
> >
> > The multi-threaded allocation/deallocation is staged as described below:
> >
> > - Stage 1: Half of the threads will be allocating memory and the
> >   other half will be waiting for them to finish the allocation.
> > - Stage 2: Half of the threads will be allocating memory and the
> >   other half will be deallocating memory.
> > - Stage 3: Half of the threads will be deallocating memory and the
> >   second half waiting on the to finish.
> 
> Typo; should be "them to finish".
> 

OK. I will fix it in the next version.

> > Add 'malloc/tst-aligned-alloc-random-thread.c' where each thread will
> > deallocate only the memory that was previously allocated by itself.
> >
> > Add 'malloc/tst-aligned-alloc-random-thread-cross.c' where each thread
> > will deallocate memory that was previously allocated by another thread.
> >
> > The intention is to be able to utilize existing malloc testing to ensure
> > that similar allocation APIs are also exposed to the same rigors.
> 
> OK.
> 
> > ---
> >  malloc/Makefile                               |   8 +
> >  .../tst-aligned-alloc-random-thread-cross.c   |  19 +++
> >  malloc/tst-aligned-alloc-random-thread.c      | 150 ++++++++++++++++++
> >  3 files changed, 177 insertions(+)
> >  create mode 100644 malloc/tst-aligned-alloc-random-thread-cross.c
> >  create mode 100644 malloc/tst-aligned-alloc-random-thread.c
> >
> > diff --git a/malloc/Makefile b/malloc/Makefile
> > index 02aff1bd1d..98d507a6eb 100644
> > --- a/malloc/Makefile
> > +++ b/malloc/Makefile
> > @@ -28,6 +28,8 @@ tests := \
> >    mallocbug \
> >    tst-aligned-alloc \
> >    tst-aligned-alloc-random \
> > +  tst-aligned-alloc-random-thread \
> > +  tst-aligned-alloc-random-thread-cross \
> 
> OK. Two new tests.
> 
> >    tst-alloc_buffer \
> >    tst-calloc \
> >    tst-free-errno \
> > @@ -151,6 +153,8 @@ ifeq ($(have-GLIBC_2.23)$(build-shared),yesyes)
> >  # the tests expect specific internal behavior that is changed due to linking to
> >  # libmcheck.a.
> >  tests-exclude-mcheck = \
> > +  tst-aligned-alloc-random-thread \
> > +  tst-aligned-alloc-random-thread-cross \
> 
> OK. Both are excluded from running mcheck variants because of the
> mcheck issue you just filed:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=31957
> 
> >    tst-compathooks-off \
> >    tst-compathooks-on \
> >    tst-malloc-backtrace \
> > @@ -415,7 +419,11 @@ $(objpfx)tst-mallocstate: $(objpfx)libc_malloc_debug.so
> >  $(objpfx)tst-mallocstate-malloc-check: $(objpfx)libc_malloc_debug.so
> >
> >  $(objpfx)tst-aligned-alloc-random.out: $(objpfx)tst-aligned_alloc-lib.so
> > +$(objpfx)tst-aligned-alloc-random-thread.out: $(objpfx)tst-aligned_alloc-lib.so
> > +$(objpfx)tst-aligned-alloc-random-thread-cross.out: $(objpfx)tst-aligned_alloc-lib.so
> >  $(objpfx)tst-malloc-random.out: $(objpfx)tst-aligned_alloc-lib.so
> 
> OK. Test execution depends on the DSO.
> 
> >
> >  tst-aligned-alloc-random-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
> > +tst-aligned-alloc-random-thread-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
> > +tst-aligned-alloc-random-thread-cross-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
> >  tst-malloc-random-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
> 
> OK. Preload the library to randomize allocators.
> 
> > diff --git a/malloc/tst-aligned-alloc-random-thread-cross.c b/malloc/tst-aligned-alloc-random-thread-cross.c
> > new file mode 100644
> > index 0000000000..360ecc56ee
> > --- /dev/null
> > +++ b/malloc/tst-aligned-alloc-random-thread-cross.c
> > @@ -0,0 +1,19 @@
> > +/* multi-threaded memory allocation and cross-thread deallocation test.
> 
> OK.
> 
> > +   Copyright (C) 2024 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public License as
> > +   published by the Free Software Foundation; either version 2.1 of the
> > +   License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; see the file COPYING.LIB.  If
> > +   not, see <https://www.gnu.org/licenses/>.  */
> > +#define CROSS_THREAD_DEALLOC
> > +#include "tst-aligned-alloc-random-thread.c"
> 
> OK. Sharing code with the other test. This define should make the
> deallocations cross-threaded.
> 
> > diff --git a/malloc/tst-aligned-alloc-random-thread.c b/malloc/tst-aligned-alloc-random-thread.c
> > new file mode 100644
> > index 0000000000..ca7b1588d4
> > --- /dev/null
> > +++ b/malloc/tst-aligned-alloc-random-thread.c
> > @@ -0,0 +1,150 @@
> > +/* multi-threaded memory allocation/deallocation test.
> 
> OK.
> 
> > +   Copyright (C) 2024 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public License as
> > +   published by the Free Software Foundation; either version 2.1 of the
> > +   License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; see the file COPYING.LIB.  If
> > +   not, see <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <support/check.h>
> > +#include <support/support.h>
> > +#include <support/xthread.h>
> > +#include <support/test-driver.h>
> > +#include <sys/sysinfo.h>
> > +#include <unistd.h>
> > +
> > +#ifndef ITERATIONS
> > +#  define ITERATIONS 16
> > +#endif
> > +
> > +#ifndef NUM_THREADS
> > +#  define NUM_THREADS 8
> > +#endif
> > +
> > +#ifndef NUM_ALLOCATIONS
> > +#  define NUM_ALLOCATIONS 2048
> > +#endif
> > +
> > +static pthread_barrier_t barrier;
> > +
> > +__thread unsigned int seed;
> > +
> > +typedef struct
> > +{
> > +  int id;
> > +  pthread_t thread;
> > +} thread;
> > +
> > +thread threads[NUM_THREADS];
> > +
> > +void *allocations[NUM_THREADS][NUM_ALLOCATIONS];
> 
> OK.
> 
> > +
> > +void
> > +run_thread_dealloc (int id)
> > +{
> > +  for (int i = 0; i < NUM_ALLOCATIONS; i++)
> > +    {
> > +      free (allocations[id][i]);
> > +      allocations[id][i] = NULL;
> > +    }
> > +}
> 
> OK. The allocations of a thread are associated with its ID, and this
> deallocates everything allocated by thread# ID.
> 
> > +
> > +void
> > +run_thread_alloc (int id)
> > +{
> > +  size_t size;
> > +  for (int i = 0; i < NUM_ALLOCATIONS; i++)
> > +    {
> > +      size = rand_r (&seed) & 0xffff;
> 
> This seems to tend to lead to lots of mostly large allocations. 75% of
> allocations are over 16K, with 50% over 32K size. I'm wondering if we
> should be changing this up a bit to reduce the tendency for such large
> allocations.
> 

How to choose the random size is taken from the non-threaded test
(tst-aligned-alloc-random.c). I am open to change this if you want but I
would need you to be a bit more specific, i.e.:
- What sizes are considered "small", "medium" and/or "large"?
- What's the desired distribution of allocations (percentages for each
  small/medium/large allocation)

> > +      allocations[id][i] = malloc (size);
> 
> OK. The allocator will actually be randomly changed out from under
> here by the preloaded library.
> 
> > +      TEST_VERIFY_EXIT (allocations[id][i] != NULL);
> > +    }
> > +}
> 
> > +
> > +void *
> > +run_allocations (void *arg)
> > +{
> > +  int id = *((int *) arg);
> > +  seed = time (NULL) + id;
> 
> OK. Considering we have the ID here, this looks like a reasonable seed init.
> 
> > +
> > +  /* Stage 1: First half o the threads allocating memory and the second
> > +   * half waiting for them to finish
> > +   */
> > +  if (id < NUM_THREADS / 2)
> > +    run_thread_alloc (id);
> 
> OK.
> 
> > +
> > +  xpthread_barrier_wait (&barrier);
> 
> OK. Half the threads do their allocation work, then everyone synchronizes here.
> 
> > +
> > +  /* Stage 2: Half of the threads allocationg memory and the other
> > +   * half deallocating:
> > +   * - In the non cross-thread dealloc scenario the first half will be
> > +   *   deallocating the memory allocated by themselves in stage 1 and the
> > +   *   second half will be allocating memory.
> > +   * - In the cross-thread dealloc scenario the first half will continue
> > +   *   to allocate memory and the second half will deallocate the memory
> > +   *   allocated by the first half in stage 1.
> > +   */
> > +  if (id < NUM_THREADS / 2)
> 
> OK. This bit is going to be about the first half of threads (which
> just did allocations in stage 1).
> 
> > +#ifndef CROSS_THREAD_DEALLOC
> > +    run_thread_dealloc (id);
> 
> OK. They deallocate their own blocks.
> 
> > +#else
> > +    run_thread_alloc (id + NUM_THREADS / 2);
> > +#endif
> 
> OK. In cross-thread deallocation mode, they do more allocations into
> unallocated blocks "belonging" to the second half of the thread pool.
> 
> > +  else
> 
> OK. This next bit is about what the second half of threads do.
> 
> > +#ifndef CROSS_THREAD_DEALLOC
> > +    run_thread_alloc (id);
> 
> OK. Normally, they do their own allocations.
> 
> > +#else
> > +    run_thread_dealloc (id - NUM_THREADS / 2);
> > +#endif
> 
> OK. In cross-thread deallocation mode, they deallocate what their
> counterpart threads in the first half of the thread pool allocated in
> stage 1.
> 
> > +
> > +  xpthread_barrier_wait (&barrier);
> 
> OK. Then everyone synchronizes.
> 
> > +
> > +  // Stage 3: Second half of the threads deallocating and the first half
> > +  // waiting for them to finish.
> > +  if (id >= NUM_THREADS / 2)
> 
> OK. We are in stage 3 now, and this is the *second* half of threads.
> 
> > +    run_thread_dealloc (id);
> 
> OK. They deallocate whatever was allocated into their own blocks.
> 
> In regular deallocation mode, this means they will deallocated what
> they themselves allocated in stage 2. But in cross-thread deallocation
> mode, this means they will be deallocating what the first half of the
> thread pool allocated into the second half's pointer array in stage 2.
> 
> > +
> > +  return NULL;
> > +}
> 
> This should probably just return nothing and have a void return type.
> 

Actually not, xpthread_create expects the run_allocations function to
return a void *

> > +
> > +static int
> > +do_test (void)
> > +{
> > +  xpthread_barrier_init (&barrier, NULL, NUM_THREADS);
> 
> OK.
> 
> > +
> > +  for (int i = 0; i < ITERATIONS; i++)
> 
> OK. We run through the stages many times.
> 
> > +    {
> > +      for (int t = 0; t < NUM_THREADS; t++)
> > +       {
> > +         threads[t].id = t;
> > +         threads[t].thread
> > +             = xpthread_create (NULL, run_allocations, &threads[t].id);
> > +       }
> > +
> > +      for (int t = 0; t < NUM_THREADS; t++)
> > +       {
> > +         xpthread_join (threads[t].thread);
> > +
> > +         // Check everything was freed
> > +         for (int a = 0; a < NUM_ALLOCATIONS; a++)
> > +           TEST_VERIFY_EXIT (allocations[t][a] == NULL);
> 
> I'd say we don't need this check. If this fails, it wouldn't be
> because of the allocator but rather something very seriously wrong
> elsewhere.
> 

OK. I will remove it in the next version

> > +       }
> > +    }
> > +
> > +  return 0;
> > +}
> > +
> > +#include <support/test-driver.c>
> > --
> > Miguel Martín
> > mmartinv@redhat.com
> >
> 
> 
> --
> Arjun Shankar
> he/him/his
>
  
Arjun Shankar July 16, 2024, 10:44 a.m. UTC | #3
Hi Miguel,

> > > +      size = rand_r (&seed) & 0xffff;
> >
> > This seems to tend to lead to lots of mostly large allocations. 75% of
> > allocations are over 16K, with 50% over 32K size. I'm wondering if we
> > should be changing this up a bit to reduce the tendency for such large
> > allocations.
> >
>
> How to choose the random size is taken from the non-threaded test
> (tst-aligned-alloc-random.c). I am open to change this if you want but I
> would need you to be a bit more specific, i.e.:
> - What sizes are considered "small", "medium" and/or "large"?
> - What's the desired distribution of allocations (percentages for each
>   small/medium/large allocation)

Good questions. I just profiled gcc compiling a simple test program
using libmemusage and found that it allocates lots of small as well as
large blocks ("small" meaning single/double digit sizes, and "large"
meaning kilobytes, sometimes tens of kilobytes -- yes I know it's a
arbitrary definition but the sizes fall into the same size range as
that chosen for this test: 1 to 65535). Some selected frequently
allocated size ranges and their frequencies from the gcc profiling
run:

0-15 - 10%
16-31 - 19%
48-63 - 19%
64-79 - 5%
192-207 - 3%
1024-1039 - 3%
2048-2063 - 3%
3200-3215 - 3%
8032-8047 - 11%

There were small numbers of allocations at various sizes going all the
way up to over 45K in size.

Given this, I think trying to distribute the allocation sizes evenly
across various size magnitudes should bring the test's allocation size
distribution roughly closer to the profiling run. How about this?:

/* Select a size between 1 and 65535 bytes with a log-uniform distribution.  */
msb = 1 << rand_r (&seed) % 16;
size = msb + rand_r (&seed) % msb;

> > > +
> > > +  return NULL;
> > > +}
> >
> > This should probably just return nothing and have a void return type.
> >
>
> Actually not, xpthread_create expects the run_allocations function to
> return a void *

Right. My bad.

Cheers,
Arjun
  

Patch

diff --git a/malloc/Makefile b/malloc/Makefile
index 02aff1bd1d..98d507a6eb 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -28,6 +28,8 @@  tests := \
   mallocbug \
   tst-aligned-alloc \
   tst-aligned-alloc-random \
+  tst-aligned-alloc-random-thread \
+  tst-aligned-alloc-random-thread-cross \
   tst-alloc_buffer \
   tst-calloc \
   tst-free-errno \
@@ -151,6 +153,8 @@  ifeq ($(have-GLIBC_2.23)$(build-shared),yesyes)
 # the tests expect specific internal behavior that is changed due to linking to
 # libmcheck.a.
 tests-exclude-mcheck = \
+  tst-aligned-alloc-random-thread \
+  tst-aligned-alloc-random-thread-cross \
   tst-compathooks-off \
   tst-compathooks-on \
   tst-malloc-backtrace \
@@ -415,7 +419,11 @@  $(objpfx)tst-mallocstate: $(objpfx)libc_malloc_debug.so
 $(objpfx)tst-mallocstate-malloc-check: $(objpfx)libc_malloc_debug.so
 
 $(objpfx)tst-aligned-alloc-random.out: $(objpfx)tst-aligned_alloc-lib.so
+$(objpfx)tst-aligned-alloc-random-thread.out: $(objpfx)tst-aligned_alloc-lib.so
+$(objpfx)tst-aligned-alloc-random-thread-cross.out: $(objpfx)tst-aligned_alloc-lib.so
 $(objpfx)tst-malloc-random.out: $(objpfx)tst-aligned_alloc-lib.so
 
 tst-aligned-alloc-random-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
+tst-aligned-alloc-random-thread-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
+tst-aligned-alloc-random-thread-cross-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
 tst-malloc-random-ENV = LD_PRELOAD=$(objpfx)tst-aligned_alloc-lib.so
diff --git a/malloc/tst-aligned-alloc-random-thread-cross.c b/malloc/tst-aligned-alloc-random-thread-cross.c
new file mode 100644
index 0000000000..360ecc56ee
--- /dev/null
+++ b/malloc/tst-aligned-alloc-random-thread-cross.c
@@ -0,0 +1,19 @@ 
+/* multi-threaded memory allocation and cross-thread deallocation test.
+   Copyright (C) 2024 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <https://www.gnu.org/licenses/>.  */
+#define CROSS_THREAD_DEALLOC
+#include "tst-aligned-alloc-random-thread.c"
diff --git a/malloc/tst-aligned-alloc-random-thread.c b/malloc/tst-aligned-alloc-random-thread.c
new file mode 100644
index 0000000000..ca7b1588d4
--- /dev/null
+++ b/malloc/tst-aligned-alloc-random-thread.c
@@ -0,0 +1,150 @@ 
+/* multi-threaded memory allocation/deallocation test.
+   Copyright (C) 2024 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <https://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/xthread.h>
+#include <support/test-driver.h>
+#include <sys/sysinfo.h>
+#include <unistd.h>
+
+#ifndef ITERATIONS
+#  define ITERATIONS 16
+#endif
+
+#ifndef NUM_THREADS
+#  define NUM_THREADS 8
+#endif
+
+#ifndef NUM_ALLOCATIONS
+#  define NUM_ALLOCATIONS 2048
+#endif
+
+static pthread_barrier_t barrier;
+
+__thread unsigned int seed;
+
+typedef struct
+{
+  int id;
+  pthread_t thread;
+} thread;
+
+thread threads[NUM_THREADS];
+
+void *allocations[NUM_THREADS][NUM_ALLOCATIONS];
+
+void
+run_thread_dealloc (int id)
+{
+  for (int i = 0; i < NUM_ALLOCATIONS; i++)
+    {
+      free (allocations[id][i]);
+      allocations[id][i] = NULL;
+    }
+}
+
+void
+run_thread_alloc (int id)
+{
+  size_t size;
+  for (int i = 0; i < NUM_ALLOCATIONS; i++)
+    {
+      size = rand_r (&seed) & 0xffff;
+      allocations[id][i] = malloc (size);
+      TEST_VERIFY_EXIT (allocations[id][i] != NULL);
+    }
+}
+
+void *
+run_allocations (void *arg)
+{
+  int id = *((int *) arg);
+  seed = time (NULL) + id;
+
+  /* Stage 1: First half o the threads allocating memory and the second
+   * half waiting for them to finish
+   */
+  if (id < NUM_THREADS / 2)
+    run_thread_alloc (id);
+
+  xpthread_barrier_wait (&barrier);
+
+  /* Stage 2: Half of the threads allocationg memory and the other
+   * half deallocating:
+   * - In the non cross-thread dealloc scenario the first half will be
+   *   deallocating the memory allocated by themselves in stage 1 and the
+   *   second half will be allocating memory.
+   * - In the cross-thread dealloc scenario the first half will continue
+   *   to allocate memory and the second half will deallocate the memory
+   *   allocated by the first half in stage 1.
+   */
+  if (id < NUM_THREADS / 2)
+#ifndef CROSS_THREAD_DEALLOC
+    run_thread_dealloc (id);
+#else
+    run_thread_alloc (id + NUM_THREADS / 2);
+#endif
+  else
+#ifndef CROSS_THREAD_DEALLOC
+    run_thread_alloc (id);
+#else
+    run_thread_dealloc (id - NUM_THREADS / 2);
+#endif
+
+  xpthread_barrier_wait (&barrier);
+
+  // Stage 3: Second half of the threads deallocating and the first half
+  // waiting for them to finish.
+  if (id >= NUM_THREADS / 2)
+    run_thread_dealloc (id);
+
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  xpthread_barrier_init (&barrier, NULL, NUM_THREADS);
+
+  for (int i = 0; i < ITERATIONS; i++)
+    {
+      for (int t = 0; t < NUM_THREADS; t++)
+	{
+	  threads[t].id = t;
+	  threads[t].thread
+	      = xpthread_create (NULL, run_allocations, &threads[t].id);
+	}
+
+      for (int t = 0; t < NUM_THREADS; t++)
+	{
+	  xpthread_join (threads[t].thread);
+
+	  // Check everything was freed
+	  for (int a = 0; a < NUM_ALLOCATIONS; a++)
+	    TEST_VERIFY_EXIT (allocations[t][a] == NULL);
+	}
+    }
+
+  return 0;
+}
+
+#include <support/test-driver.c>