multithreading broken in Cygwin 3.5.3

multithreading broken in Cygwin 3.5.3

[Bruno Haible]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

Hi,

In Cygwin 3.5.3, on different machines, I see 3 Gnulib tests failing by
timeout that worked perfectly fine in Cygwin 3.4.6 and older:
  FAIL: test-call_once2.exe
  FAIL: test-lock.exe
  FAIL: test-pthread-once2.exe

Find here attached a simplified version of test-pthread-once2.c.
Compile and run:
  $ x86_64-pc-cygwin-gcc -Wall foo.c
  $ ./a

Expected behaviour: Termination within 1 minute.
Actual behaviour:   Terminates by timeout after 10 minutes.

When I change
  #define ENABLE_DEBUGGING 0
to
  #define ENABLE_DEBUGGING 1
the test does lots of output and terminates within 20 seconds.
Therefore I can't really tell where the problem comes from.
But I do see some changes in
  $ git diff cygwin-3.4.6 cygwin-3.5.3 winsup/cygwin/thread.cc
  $ git diff cygwin-3.4.6 cygwin-3.5.3 winsup/testsuite/winsup.api/pthread

Bruno

[-- Attachment #2: foo.c --]
[-- Type: text/x-csrc, Size: 6564 bytes --]

/* Whether to enable locking.
   Uncomment this to get a test program without locking, to verify that
   it crashes.  */
#define ENABLE_LOCKING 1

/* Whether to help the scheduler through explicit sched_yield().
   Uncomment this to see if the operating system has a fair scheduler.  */
#define EXPLICIT_YIELD 1

/* Whether to print debugging messages.  */
#define ENABLE_DEBUGGING 0

/* Number of simultaneous threads.  */
#define THREAD_COUNT 10

/* Number of operations performed in each thread.
   This is quite high, because with a smaller count, say 5000, we often get
   an "OK" result even without ENABLE_LOCKING (on Linux/x86).  */
#define REPEAT_COUNT 50000

#include <assert.h>
#include <pthread.h>
#include <signal.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#if EXPLICIT_YIELD
# include <sched.h>
#endif


#if ENABLE_DEBUGGING
# define dbgprintf printf
#else
# define dbgprintf if (0) printf
#endif

#if EXPLICIT_YIELD
# define yield() sched_yield ()
#else
# define yield()
#endif

/* Returns a reference to the current thread as a pointer, for debugging.  */
#define pthread_self_pointer() ((void *) (uintptr_t) pthread_self ())


/* ------------------------ Test once-only execution ------------------------ */

/* Test once-only execution by having several threads attempt to grab a
   once-only task simultaneously (triggered by releasing a read-write lock).  */

static pthread_once_t fresh_once = PTHREAD_ONCE_INIT;
static int ready[THREAD_COUNT];
static pthread_mutex_t ready_lock[THREAD_COUNT];
#if ENABLE_LOCKING
static pthread_rwlock_t fire_signal[REPEAT_COUNT];
#else
static volatile int fire_signal_state;
#endif
static pthread_once_t once_control;
static int performed;
static pthread_mutex_t performed_lock;

static void
once_execute (void)
{
  assert (pthread_mutex_lock (&performed_lock) == 0);
  performed++;
  assert (pthread_mutex_unlock (&performed_lock) == 0);
}

static void *
once_contender_thread (void *arg)
{
  int id = (int) (intptr_t) arg;
  int repeat;

  for (repeat = 0; repeat <= REPEAT_COUNT; repeat++)
    {
      /* Tell the main thread that we're ready.  */
      assert (pthread_mutex_lock (&ready_lock[id]) == 0);
      ready[id] = 1;
      assert (pthread_mutex_unlock (&ready_lock[id]) == 0);

      if (repeat == REPEAT_COUNT)
        break;

      dbgprintf ("Contender %p waiting for signal for round %d\n",
                 pthread_self_pointer (), repeat);
#if ENABLE_LOCKING
      /* Wait for the signal to go.  */
      assert (pthread_rwlock_rdlock (&fire_signal[repeat]) == 0);
      /* And don't hinder the others (if the scheduler is unfair).  */
      assert (pthread_rwlock_unlock (&fire_signal[repeat]) == 0);
#else
      /* Wait for the signal to go.  */
      while (fire_signal_state <= repeat)
        yield ();
#endif
      dbgprintf ("Contender %p got the     signal for round %d\n",
                 pthread_self_pointer (), repeat);

      /* Contend for execution.  */
      assert (pthread_once (&once_control, once_execute) == 0);
    }

  return NULL;
}

static void
test_once (void)
{
  int i, repeat;
  pthread_t threads[THREAD_COUNT];

  /* Initialize all variables.  */
  for (i = 0; i < THREAD_COUNT; i++)
    {
      pthread_mutexattr_t attr;

      ready[i] = 0;
      assert (pthread_mutexattr_init (&attr) == 0);
      assert (pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_NORMAL) == 0);
      assert (pthread_mutex_init (&ready_lock[i], &attr) == 0);
      assert (pthread_mutexattr_destroy (&attr) == 0);
    }
#if ENABLE_LOCKING
  for (i = 0; i < REPEAT_COUNT; i++)
    assert (pthread_rwlock_init (&fire_signal[i], NULL) == 0);
#else
  fire_signal_state = 0;
#endif

#if ENABLE_LOCKING
  /* Block all fire_signals.  */
  for (i = REPEAT_COUNT-1; i >= 0; i--)
    assert (pthread_rwlock_wrlock (&fire_signal[i]) == 0);
#endif

  /* Spawn the threads.  */
  for (i = 0; i < THREAD_COUNT; i++)
    assert (pthread_create (&threads[i], NULL,
                            once_contender_thread, (void *) (intptr_t) i)
            == 0);

  for (repeat = 0; repeat <= REPEAT_COUNT; repeat++)
    {
      /* Wait until every thread is ready.  */
      dbgprintf ("Main thread before synchronizing for round %d\n", repeat);
      for (;;)
        {
          int ready_count = 0;
          for (i = 0; i < THREAD_COUNT; i++)
            {
              assert (pthread_mutex_lock (&ready_lock[i]) == 0);
              ready_count += ready[i];
              assert (pthread_mutex_unlock (&ready_lock[i]) == 0);
            }
          if (ready_count == THREAD_COUNT)
            break;
          yield ();
        }
      dbgprintf ("Main thread after  synchronizing for round %d\n", repeat);

      if (repeat > 0)
        {
          /* Check that exactly one thread executed the once_execute()
             function.  */
          if (performed != 1)
            abort ();
        }

      if (repeat == REPEAT_COUNT)
        break;

      /* Preparation for the next round: Initialize once_control.  */
      memcpy (&once_control, &fresh_once, sizeof (pthread_once_t));

      /* Preparation for the next round: Reset the performed counter.  */
      performed = 0;

      /* Preparation for the next round: Reset the ready flags.  */
      for (i = 0; i < THREAD_COUNT; i++)
        {
          assert (pthread_mutex_lock (&ready_lock[i]) == 0);
          ready[i] = 0;
          assert (pthread_mutex_unlock (&ready_lock[i]) == 0);
        }

      /* Signal all threads simultaneously.  */
      dbgprintf ("Main thread giving signal for round %d\n", repeat);
#if ENABLE_LOCKING
      assert (pthread_rwlock_unlock (&fire_signal[repeat]) == 0);
#else
      fire_signal_state = repeat + 1;
#endif
    }

  /* Wait for the threads to terminate.  */
  for (i = 0; i < THREAD_COUNT; i++)
    assert (pthread_join (threads[i], NULL) == 0);
}


/* -------------------------------------------------------------------------- */

int
main ()
{
  /* Declare failure if test takes too long, by using default abort
     caused by SIGALRM.  */
  int alarm_value = 600;
  signal (SIGALRM, SIG_DFL);
  alarm (alarm_value);

  {
    pthread_mutexattr_t attr;

    assert (pthread_mutexattr_init (&attr) == 0);
    assert (pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_NORMAL) == 0);
    assert (pthread_mutex_init (&performed_lock, &attr) == 0);
    assert (pthread_mutexattr_destroy (&attr) == 0);
  }

  printf ("Starting test_once ..."); fflush (stdout);
  test_once ();
  printf (" OK\n"); fflush (stdout);

  return 0;
}

Re: multithreading broken in Cygwin 3.5.3

[Takashi Yano]

On Thu, 23 May 2024 20:09:09 +0200
Bruno Haible wrote:
> In Cygwin 3.5.3, on different machines, I see 3 Gnulib tests failing by
> timeout that worked perfectly fine in Cygwin 3.4.6 and older:
>   FAIL: test-call_once2.exe
>   FAIL: test-lock.exe
>   FAIL: test-pthread-once2.exe
> 
> Find here attached a simplified version of test-pthread-once2.c.
> Compile and run:
>   $ x86_64-pc-cygwin-gcc -Wall foo.c
>   $ ./a
> 
> Expected behaviour: Termination within 1 minute.
> Actual behaviour:   Terminates by timeout after 10 minutes.
> 
> When I change
>   #define ENABLE_DEBUGGING 0
> to
>   #define ENABLE_DEBUGGING 1
> the test does lots of output and terminates within 20 seconds.
> Therefore I can't really tell where the problem comes from.
> But I do see some changes in
>   $ git diff cygwin-3.4.6 cygwin-3.5.3 winsup/cygwin/thread.cc
>   $ git diff cygwin-3.4.6 cygwin-3.5.3 winsup/testsuite/winsup.api/pthread

Thanks for the report.

As you mentioned in private mail to me, this seems to be a regression of
pthread::once() introduced by
commit 2c5433e5da8216aaf7458e50c63683c68fb0d3e8.

I'll submit a patch for that issue shortly.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: multithreading broken in Cygwin 3.5.3

[Bruno Haible]

Takashi Yano wrote:
> As you mentioned in private mail to me, this seems to be a regression of
> pthread::once() introduced by
> commit 2c5433e5da8216aaf7458e50c63683c68fb0d3e8.
> 
> I'll submit a patch for that issue shortly.

My workaround implementation of pthread_once (in gnulib) looks like this:

  /* This would be the code, for
       typedef struct
         {
           pthread_mutex_t mutex;
           _Atomic unsigned int num_threads;
           _Atomic unsigned int done;
         }
       pthread_once_t;
   */
  if (once_control->done == 0)
    {
      once_control->num_threads += 1;
      pthread_mutex_lock (&once_control->mutex);
      if (once_control->done == 0)
        {
          (*initfunction) ();
          once_control->done = 1;
        }
      pthread_mutex_unlock (&once_control->mutex);
      if ((once_control->num_threads -= 1) == 0)
        pthread_mutex_destroy (&once_control->mutex);
    }

It makes sure that
  - The last thread that had been dealing with the mutex deletes
    the mutex.
  - Once the mutex is deleted, is it never again accessed. The
    entry test of the 'done' boolean ensures this.

Bruno

Re: multithreading broken in Cygwin 3.5.3

[Takashi Yano]

On Wed, 29 May 2024 12:26:31 +0200
Bruno Haible wrote:
> Takashi Yano wrote:
> > As you mentioned in private mail to me, this seems to be a regression of
> > pthread::once() introduced by
> > commit 2c5433e5da8216aaf7458e50c63683c68fb0d3e8.
> > 
> > I'll submit a patch for that issue shortly.
> 
> My workaround implementation of pthread_once (in gnulib) looks like this:
> 
>   /* This would be the code, for
>        typedef struct
>          {
>            pthread_mutex_t mutex;
>            _Atomic unsigned int num_threads;
>            _Atomic unsigned int done;
>          }
>        pthread_once_t;
>    */
>   if (once_control->done == 0)
>     {
>       once_control->num_threads += 1;
>       pthread_mutex_lock (&once_control->mutex);
>       if (once_control->done == 0)
>         {
>           (*initfunction) ();
>           once_control->done = 1;
>         }
>       pthread_mutex_unlock (&once_control->mutex);
>       if ((once_control->num_threads -= 1) == 0)
>         pthread_mutex_destroy (&once_control->mutex);
>     }
> 
> It makes sure that
>   - The last thread that had been dealing with the mutex deletes
>     the mutex.
>   - Once the mutex is deleted, is it never again accessed. The
>     entry test of the 'done' boolean ensures this.

Thanks for the advice.

The problem is that we cannot change the type of pthread_once_t
for binary compatibility. My patch stops to use
pthread_mutex_t mutex in pthread_once_t however it cannot be
deleted despite it is not used at all.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: multithreading broken in Cygwin 3.5.3

[Bruno Haible]

Takashi Yano wrote:
> > My workaround implementation of pthread_once (in gnulib) looks like this:
> > 
> >   /* This would be the code, for
> >        typedef struct
> >          {
> >            pthread_mutex_t mutex;
> >            _Atomic unsigned int num_threads;
> >            _Atomic unsigned int done;
> >          }
> >        pthread_once_t;
> >    */
> >   if (once_control->done == 0)
> >     {
> >       once_control->num_threads += 1;
> >       pthread_mutex_lock (&once_control->mutex);
> >       if (once_control->done == 0)
> >         {
> >           (*initfunction) ();
> >           once_control->done = 1;
> >         }
> >       pthread_mutex_unlock (&once_control->mutex);
> >       if ((once_control->num_threads -= 1) == 0)
> >         pthread_mutex_destroy (&once_control->mutex);
> >     }
> > 
> > It makes sure that
> >   - The last thread that had been dealing with the mutex deletes
> >     the mutex.
> >   - Once the mutex is deleted, is it never again accessed. The
> >     entry test of the 'done' boolean ensures this.
> 
> Thanks for the advice.
> 
> The problem is that we cannot change the type of pthread_once_t
> for binary compatibility. My patch stops to use
> pthread_mutex_t mutex in pthread_once_t however it cannot be
> deleted despite it is not used at all.

This problem can be solved, by using
  - the upper 31 bits of 'state' as 'num_threads' and
  - the lowest bit of 'state' as 'done' bit.

I did this in gnulib:
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/pthread-once.c;h=069e77e3f3fc2a6e6564f73ba1a3f86ddf9ba531;hb=HEAD#l41
and I have tested it with the test program 'test-pthread-once2' that
I mentioned in my original report.

You can take this code, since I licensed it under LGPLv2+. You can
also surely simplify it, since you are an expert with the Interlocked*
atomic operations (whereas I only wanted to use the GCC built-in
__sync* atomics).

Bruno