Dead code in pthread_cond_wait() for spin-wait

Dead code in pthread_cond_wait() for spin-wait

[Olivier Dion]

Hi,

Commit ed19993b5b0d05d62cc883571519a67dae481a14 from 2016 introduce "a
new implementation of condition variables".  This new implementation
seems to introduce a spin-wait fast-path to avoid taking the underlying
futex when doing a pthread_cond_wait().

However, looking at the following snippet in nptl/pthread_cond_wait.c:

 static __always_inline int
 __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
     clockid_t clockid, const struct __timespec64 *abstime)
 {
   const int maxspin = 0;
   [...]
           /* Spin-wait first. [...] */
	   unsigned int spin = maxspin;
	   while (signals == 0 && spin > 0) {
             [...]
           }
   [...]
 }

it seems to me that the try-spinning portion is dead code in
pthread_cond_wait_common(). I was wondering why this was the case?

I also think that there would maybe some interest in using the Userspace
RCU wait algorithm [0] for this.

 [0] https://github.com/urcu/userspace-rcu/blob/master/src/urcu-wait.h

Thanks,

Olivier

-- 
Olivier Dion
EfficiOS Inc.
https://www.efficios.com

Re: Dead code in pthread_cond_wait() for spin-wait

[Mathieu Desnoyers]

On 2023-10-11 14:30, Olivier Dion wrote:
> Hi,
> 
> Commit ed19993b5b0d05d62cc883571519a67dae481a14 from 2016 introduce "a
> new implementation of condition variables".  This new implementation
> seems to introduce a spin-wait fast-path to avoid taking the underlying
> futex when doing a pthread_cond_wait().

Adding libc-alpha, Carlos and Florian in CC.

Thanks,

Mathieu

> 
> However, looking at the following snippet in nptl/pthread_cond_wait.c:
> 
>   static __always_inline int
>   __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
>       clockid_t clockid, const struct __timespec64 *abstime)
>   {
>     const int maxspin = 0;
>     [...]
>             /* Spin-wait first. [...] */
> 	   unsigned int spin = maxspin;
> 	   while (signals == 0 && spin > 0) {
>               [...]
>             }
>     [...]
>   }
> 
> it seems to me that the try-spinning portion is dead code in
> pthread_cond_wait_common(). I was wondering why this was the case?
> 
> I also think that there would maybe some interest in using the Userspace
> RCU wait algorithm [0] for this.
> 
>   [0] https://github.com/urcu/userspace-rcu/blob/master/src/urcu-wait.h
> 
> Thanks,
> 
> Olivier
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: Dead code in pthread_cond_wait() for spin-wait

[Adhemerval Zanella Netto]

On 11/10/23 15:30, Olivier Dion via Libc-help wrote:
> Hi,
> 
> Commit ed19993b5b0d05d62cc883571519a67dae481a14 from 2016 introduce "a
> new implementation of condition variables".  This new implementation
> seems to introduce a spin-wait fast-path to avoid taking the underlying
> futex when doing a pthread_cond_wait().
> 
> However, looking at the following snippet in nptl/pthread_cond_wait.c:
> 
>  static __always_inline int
>  __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
>      clockid_t clockid, const struct __timespec64 *abstime)
>  {
>    const int maxspin = 0;
>    [...]
>            /* Spin-wait first. [...] */
> 	   unsigned int spin = maxspin;
> 	   while (signals == 0 && spin > 0) {
>              [...]
>            }
>    [...]
>  }
> 

Afaik it was added a placeholder for future extension, so fell free to
send a patch to delete it.

> it seems to me that the try-spinning portion is dead code in
> pthread_cond_wait_common(). I was wondering why this was the case?
> 
> I also think that there would maybe some interest in using the Userspace
> RCU wait algorithm [0] for this.
> 
>  [0] https://github.com/urcu/userspace-rcu/blob/master/src/urcu-wait.h
> 
> Thanks,
> 
> Olivier
> 

Re: Dead code in pthread_cond_wait() for spin-wait

[Olivier Dion]

On Wed, 11 Oct 2023, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
> On 11/10/23 15:30, Olivier Dion via Libc-help wrote:

[...]

>> However, looking at the following snippet in nptl/pthread_cond_wait.c:
>> 
>>  static __always_inline int
>>  __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
>>      clockid_t clockid, const struct __timespec64 *abstime)
>>  {
>>    const int maxspin = 0;
>>    [...]
>>            /* Spin-wait first. [...] */
>> 	   unsigned int spin = maxspin;
>> 	   while (signals == 0 && spin > 0) {
>>              [...]
>>            }
>>    [...]
>>  }
>> 
>
> Afaik it was added a placeholder for future extension, so fell free to
> send a patch to delete it.

[...]

What about making it work instead?  Would this be something to consider?
IMHO it would be worth it.  Having a fast-path that spins instead of
taking the futex directly can greatly impact performance in some cases.

In the meantime, I think it is safe to remove it.  I'll send a patch on
libc-alpha for that.

Thanks,

Olivier
-- 
Olivier Dion
EfficiOS Inc.
https://www.efficios.com

Re: Dead code in pthread_cond_wait() for spin-wait

[Florian Weimer]

* Olivier Dion:

> On Wed, 11 Oct 2023, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
>> On 11/10/23 15:30, Olivier Dion via Libc-help wrote:
>
> [...]
>
>>> However, looking at the following snippet in nptl/pthread_cond_wait.c:
>>> 
>>>  static __always_inline int
>>>  __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
>>>      clockid_t clockid, const struct __timespec64 *abstime)
>>>  {
>>>    const int maxspin = 0;
>>>    [...]
>>>            /* Spin-wait first. [...] */
>>> 	   unsigned int spin = maxspin;
>>> 	   while (signals == 0 && spin > 0) {
>>>              [...]
>>>            }
>>>    [...]
>>>  }
>>> 
>>
>> Afaik it was added a placeholder for future extension, so fell free to
>> send a patch to delete it.
>
> [...]
>
> What about making it work instead?  Would this be something to consider?
> IMHO it would be worth it.  Having a fast-path that spins instead of
> taking the futex directly can greatly impact performance in some cases.

Changes to the condvar code are very difficult for us.  There's an
open bug with patches that have been under review for years:

  pthread_cond_signal failed to wake up pthread_cond_wait due to a bug
  in undoing stealing
  <https://sourceware.org/bugzilla/show_bug.cgi?id=25847>