Add multithreaded test of sem_getvalue

[Arjun Shankar]

Hi Joseph,

This looks good to me. I've noted a very minor nitpick below but I
actually don't think it's worth a v2 at all.

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

Review below:

> Test coverage of sem_getvalue is fairly limited.  Add a test that runs
> it on threads on each CPU.  For this purpose I adapted
> tst-skeleton-thread-affinity.c; it didn't seem very suitable to use
> as-is or include directly in a different test doing things per-CPU,
> but did seem a suitable starting point (thus sharing
> tst-skeleton-affinity.c) for such testing.
>
> Tested for x86_64.

OK.

> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 527c7a5ae8..eb9c697ce5 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -668,6 +668,7 @@ ifeq ($(subdir),nptl)
>  tests += \
>    tst-align-clone \
>    tst-getpid1 \
> +  tst-sem_getvalue-affinity \
>    # tests
>
>  # tst-rseq-nptl is an internal test because it requires a definition of

OK. Add the new test.

> diff --git a/sysdeps/unix/sysv/linux/tst-sem_getvalue-affinity.c b/sysdeps/unix/sysv/linux/tst-sem_getvalue-affinity.c
> new file mode 100644
> index 0000000000..4176f67533
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-sem_getvalue-affinity.c
> @@ -0,0 +1,185 @@
> +/* Test sem_getvalue across CPUs.  Based on tst-skeleton-thread-affinity.c.

OK.

> +   Copyright (C) 2015-2024 Free Software Foundation, Inc.

OK. Copyright line from the skeleton that this file is based on.

> +   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; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <pthread.h>
> +#include <semaphore.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <support/xthread.h>
> +#include <sys/time.h>
> +
> +struct conf;
> +static bool early_test (struct conf *);

OK. You declare this function here because you define it after
including the skeleton that uses it.

> +
> +static int
> +setaffinity (size_t size, const cpu_set_t *set)
> +{
> +  int ret = pthread_setaffinity_np (pthread_self (), size, set);
> +  if (ret != 0)
> +    {
> +      errno = ret;
> +      return -1;
> +    }
> +  return 0;
> +}

OK.

> +
> +static int
> +getaffinity (size_t size, cpu_set_t *set)
> +{
> +  int ret = pthread_getaffinity_np (pthread_self (), size, set);
> +  if (ret != 0)
> +    {
> +      errno = ret;
> +      return -1;
> +    }
> +  return 0;
> +}

OK.

> +
> +#include "tst-skeleton-affinity.c"

OK. This skeleton uses the above two set/get functions along with the
setup to run a number of tests reading and manipulating CPU affinity
at various set sizes.

> +
> +static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;

OK.

> +static sem_t sem;

OK.

> +
> +static void *
> +tf (void *arg)

OK. This is a thread function...

> +{
> +  void *ret = NULL;
> +  xpthread_mutex_lock (&lock);

OK. We take a lock. As I read more, I expect I'll figure out why.

> +  int semval;
> +  if (sem_getvalue (&sem, &semval) != 0)
> +    {
> +      printf ("sem_getvalue failed: %m\n");
> +      ret = (void *) 1;
> +    }
> +  else if (semval != 12345)
> +    {
> +      printf ("sem_getvalue returned %d not 12345\n", semval);
> +      ret = (void *) 1;
> +    }

OK. We call get_semvalue and expect it to be 12345. So the lock is
presumably to ensure that the semaphore value doesn't change during
this call.

> +  xpthread_mutex_unlock (&lock);
> +  return ret;
> +}

OK. We will unlock and return. We fail and return "1" if things don't work out.

> +
> +static int
> +stop_and_join_threads (struct conf *conf, cpu_set_t *set,
> +                      pthread_t *pinned_first, pthread_t *pinned_last)

OK. Looks like an end-of-test function.

> +{
> +  int failed = 0;
> +  for (pthread_t *p = pinned_first; p < pinned_last; ++p)

OK. Presumably we are operating on an array of pthread_t's. I'm noting
that the entry at "pinned_last" is not iterated through. I'll look out
for why. I expect it's one past the end of the array.

> +    {
> +      int cpu = p - pinned_first;

OK. This is an index into the array pointing to the current entry in
the pthread_t array, *and* a CPU number.

> +      if (!CPU_ISSET_S (cpu, CPU_ALLOC_SIZE (conf->set_size), set))
> +       continue;

OK. We skip this entry if the corresponding CPU is not in the set.

> +
> +      void *retval = (void *) 1;
> +      int ret = pthread_join (*p, &retval);

OK. Otherwise, we join on this thread.
Question: Was it necessary to set retval here first? I expect it's
*always* set by pthread_join anyway.

> +      if (ret != 0)
> +       {
> +         printf ("error: Failed to join thread %d: %s\n", cpu, strerror (ret));
> +         fflush (stdout);
> +         /* Cannot shut down cleanly with threads still running.  */
> +         abort ();
> +       }

OK. Error out if we can't join.

> +      if (retval != NULL)
> +       failed = 1;
> +    }
> +  return failed;
> +}

OK. Return 0 usually, but if any of the threads ended in error, we return 1.

> +
> +static bool
> +early_test (struct conf *conf)

OK. This is a setup function called by the skeleton before it starts
manipulating CPU affinity. This is actually the meat of this test.

> +{
> +  int ret;
> +  ret = sem_init (&sem, 0, 12345);

OK. The number I was looking for.

> +  if (ret != 0)
> +    {
> +      printf ("error: sem_init failed: %m\n");
> +      return false;
> +    }

OK.

> +  xpthread_mutex_lock (&lock);

OK. We take a lock.

> +  pthread_t *pinned_threads
> +    = calloc (conf->last_cpu + 1, sizeof (*pinned_threads));

OK. We allocate enough pthread_t's to account for CPUs counted from 0
to last_cpu (inclusive).
Nitpick: we have xcalloc. But maybe it's not worth a v2. We check for
a couple of failed allocations below anyway.

> +  cpu_set_t *initial_set = CPU_ALLOC (conf->set_size);
> +  cpu_set_t *scratch_set = CPU_ALLOC (conf->set_size);

OK.

> +
> +  if (pinned_threads == NULL || initial_set == NULL || scratch_set == NULL)
> +    {
> +      puts ("error: Memory allocation failure");
> +      return false;
> +    }

OK.

> +  if (getaffinity (CPU_ALLOC_SIZE (conf->set_size), initial_set) < 0)
> +    {
> +      printf ("error: pthread_getaffinity_np failed: %m\n");
> +      return false;
> +    }

OK.

> +
> +  pthread_attr_t attr;
> +  ret = pthread_attr_init (&attr);
> +  if (ret != 0)
> +    {
> +      printf ("error: pthread_attr_init failed: %s\n", strerror (ret));
> +      return false;
> +    }
> +  support_set_small_thread_stack_size (&attr);

OK.

> +
> +  /* Spawn a thread pinned to each available CPU.  */
> +  for (int cpu = 0; cpu <= conf->last_cpu; ++cpu)

OK. We did allocate enough space for all these pthread_t's.

> +    {
> +      if (!CPU_ISSET_S (cpu, CPU_ALLOC_SIZE (conf->set_size), initial_set))
> +       continue;

OK.

> +      CPU_ZERO_S (CPU_ALLOC_SIZE (conf->set_size), scratch_set);
> +      CPU_SET_S (cpu, CPU_ALLOC_SIZE (conf->set_size), scratch_set);

OK. Only have *this* CPU in the affinity set we are about to use
during thread creation.

> +      ret = pthread_attr_setaffinity_np
> +       (&attr, CPU_ALLOC_SIZE (conf->set_size), scratch_set);
> +      if (ret != 0)
> +       {
> +         printf ("error: pthread_attr_setaffinity_np for CPU %d failed: %s\n",
> +                 cpu, strerror (ret));
> +         stop_and_join_threads (conf, initial_set,
> +                                pinned_threads, pinned_threads + cpu);
> +         return false;
> +       }

OK.

> +      ret = pthread_create (pinned_threads + cpu, &attr,
> +                           tf, (void *) (uintptr_t) cpu);
> +      if (ret != 0)
> +       {
> +         printf ("error: pthread_create for CPU %d failed: %s\n",
> +                 cpu, strerror (ret));
> +         stop_and_join_threads (conf, initial_set,
> +                                pinned_threads, pinned_threads + cpu);
> +         return false;
> +       }

OK. Returning false from early_test leads to tests not being run and a
failure log.

> +    }

> +
> +  /* Main thread.  */
> +  xpthread_mutex_unlock (&lock);

OK. This lets all the threads kick off. Each will call sem_getvalue,
verify the result, and return.

> +  int failed = stop_and_join_threads (conf, initial_set,
> +                                     pinned_threads,
> +                                     pinned_threads + conf->last_cpu + 1);

OK. Wait for the threads to finish. Also, the +1 lines up with the
loop in stop_and_join_threads.

> +
> +  printf ("info: Main thread ran on %d CPU(s) of %d available CPU(s)\n",
> +         CPU_COUNT_S (CPU_ALLOC_SIZE (conf->set_size), scratch_set),
> +         CPU_COUNT_S (CPU_ALLOC_SIZE (conf->set_size), initial_set));
> +
> +  pthread_attr_destroy (&attr);
> +  CPU_FREE (scratch_set);
> +  CPU_FREE (initial_set);
> +  free (pinned_threads);
> +  return failed == 0;
> +}

OK. After this function completes execution, the skeleton will run a
bunch of affinity tests.

Cheers!
-- 
Arjun Shankar
he/him/his