[PATCH v2 1/2] Add the __libc_single_threaded variable

Florian Weimer fweimer@redhat.com
Tue Jun 30 09:24:35 GMT 2020


* DJ Delorie:

>> diff --git a/elf/tst-single_threaded-pthread.c b/elf/tst-single_threaded-pthread.c
>> new file mode 100644
>> index 0000000000..c02f4047d1
>> --- /dev/null
>> +++ b/elf/tst-single_threaded-pthread.c
>> @@ -0,0 +1,174 @@
>> +/* Test support for single-thread optimizations.  With threads.
>> +   Copyright (C) 2019 Free Software Foundation, Inc.
>
> It's 2020 now.  Same throughout.

Right, I also changed http:// to https:// throughout.

>> diff --git a/elf/tst-single_threaded-static-dlopen.c b/elf/tst-single_threaded-static-dlopen.c
>> new file mode 100644
>> index 0000000000..f270cf452e
>> --- /dev/null
>> +++ b/elf/tst-single_threaded-static-dlopen.c
>> @@ -0,0 +1,56 @@

>> +
>> +static int
>> +do_test (void)
>> +{
>> +  TEST_VERIFY (__libc_single_threaded);
>> +
>> +  /* Defined in tst-single-threaded-mod1.o.  */
>> +  extern _Bool single_threaded_1 (void);
>> +  TEST_VERIFY (single_threaded_1 ());
>> +
>> +  /* Even after a failed dlopen, assume multi-threaded mode.  */
>> +  TEST_VERIFY (dlopen ("tst-single_threaded-does-not-exist.so", RTLD_LAZY)
>> +               == NULL);
>> +  TEST_VERIFY (__libc_single_threaded);
>> +  TEST_VERIFY (single_threaded_1 ());
>
> You say "assume multi-threaded" but you test for signalling single
> threaded.

Fixed.

>> diff --git a/htl/pt-create.c b/htl/pt-create.c
>> index f501a12017..7ac875cbf7 100644
>> --- a/htl/pt-create.c
>> +++ b/htl/pt-create.c
>> @@ -24,6 +24,7 @@
>>  
>>  #include <atomic.h>
>>  #include <hurd/resource.h>
>> +#include <sys/single_threaded.h>
>>  
>>  #include <pt-internal.h>
>>  #include <pthreadP.h>
>> @@ -104,6 +105,10 @@ __pthread_create_internal (struct __pthread **thread,
>>    sigset_t sigset;
>>    size_t stacksize;
>>  
>> +  /* Avoid a data race in the multi-threaded case.  */
>> +  if (__libc_single_threaded)
>> +    __libc_single_threaded = 0;
>
> The only time you don't set __libc_single_threaded to zero is if it's
> already zero, so the test isn't needed.  Just setting
> __libc_single_threaded to zero does the same thing, but faster.  Even if
> we're multi-threaded, it doesn't matter if other cpus get the old value
> (zero) or the new one (zero).
>
> The comment doesn't really enlighten the reader, IMHO.

If a variable is written in one thread and read in another thread
without synchronization, this is a data race, even if the value written
equals what's already in the variable.  Under the C11 memory model, the
condition is actually required to avoid undefined behavior (due to the
data race).

The condition also avoids polluting the cache line if the expected value
is already there, so it's probably faster anyway (although it will be
difficult to show the effect).

>> diff --git a/misc/sys/single_threaded.h b/misc/sys/single_threaded.h
>> +#ifndef _SYS_SINGLE_THREADED_H
>> +#define _SYS_SINGLE_THREADED_H
>> +
>> +#include <features.h>
>> +
>> +__BEGIN_DECLS
>> +
>> +/* If this variable is non-zero, then the current thread is the only
>> +   thread in the process image.  If it is zero, the process can be
>> +   multi-threaded.  */
>> +extern char __libc_single_threaded;
>> +
>> +__END_DECLS
>> +
>> +#endif /* _SYS_SINGLE_THREADED_H */
>
> Ok.  Suggest s/can be/might be/.  "Can" implies we're giving the user
> permission to do something, "might" indicates a condition to be
> expected.  Or "the process must act as if multiple threads are running." ?

I switched to “might”.

>> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
>>  extern struct rtld_global _rtld_local __rtld_local_attribute__;
>> +extern char __libc_single_threaded_local __rtld_local_attribute__;
>
> This isn't mentioned anywhere else...
>
>>  #  undef __rtld_local_attribute__
>>  # endif
>>  extern struct rtld_global _rtld_global __rtld_global_attribute__;
>> +extern char __libc_single_threaded __rtld_global_attribute__;
>
> Ok.
>
>> @@ -1119,6 +1121,9 @@ extern struct link_map * _dl_get_dl_main_map (void)
>>     If libpthread is not linked in, this is an empty function.  */
>>  void __pthread_initialize_minimal (void) weak_function;
>>  
>> +/* Update both copies of __libc_single_threaded.  */
>> +void _dl_single_threaded_update (char value);
>> +
>
> Nor is this mentioned elsewhere.

All changes to ldsodefs.h were spurious and leftovers from the previous
version which used a hidden variable.

I'm going to retest and post updated patches.

Thanks,
Florian



More information about the Libc-alpha mailing list