[PATCH] nptl: Use a PI-aware lock for internal pthread_cond-related locking

[Adhemerval Zanella Netto]

On 09/09/25 04:32, Sebastian Andrzej Siewior wrote:
> On 2025-09-08 14:46:12 [-0300], Adhemerval Zanella Netto wrote:
>>> This change expects that PI-FUTEX is supported by the kernel. This is
>>> the case since a long time but it is possible to disable the FUTEX
>>> subsystem or the FUTEX_PI part of it while building the kernel.
>>> Is it okay to assume that PI-FUTEX is available or should there be a
>>> check somewhere during startup and in case of -ENOSYS a fallback to
>>> current implementation?
>>
>> We removed the __ASSUME_FUTEX_LOCK_PI (f5c77f78ec03363d5e550c4996deb75ee3f2e32a)
>> in favor or always check for PI support at runtime during pthread_mutex_init
>> (prio_inherit_missing).
>>
>> Since the kernel still might return ENOSYS for FUTEX_PI I think we should
>> keep probing its support as runtime.
> 
> Okay. Since that one is static, I guess I would have to make my own
> check in pthread_cond.
> 

Or move the prio_inherit_missing to its own TU and use is an internal
symbol.

> But… Now that I look at it again. The kernel has this FUTEX_PI option
> which depends on RT_MUTEXES. But RT_MUTEXES has no off switch so it must
> always be selected once FUTEX itself is enabled. The I2C subsystem
> selects RT_MUTEXES and I don't think there is a config without PI-FUTEX
> considering this.
> We _used_ to have runtime detection for PI-FUTEX support because not all
> architectures provided a cmpxchg function for futex. This is gone and
> all architectures as of v5.17 provide it. That would be commit
>    3297481d688a5 ("futex: Remove futex_cmpxchg detection")
>    https://git.kernel.org/torvalds/c/3297481d688a5
> 
> for reference. So I *think* this config option can be removed on kernel
> side and it appears to me as of v5.17 there should be no need for a
> runtime check regarding PI-futex.

Right, but at least for glibc we will need to keep the runtime check
because we still support old kernels.

> 
>>> --- a/nptl/pthread_cond_common.c
>>> +++ b/nptl/pthread_cond_common.c
>>> @@ -106,41 +106,23 @@ __condvar_fetch_xor_wseq_release (pthread_cond_t *cond, unsigned int val)
> …
>>>  static void __attribute__ ((unused))
>>>  __condvar_acquire_lock (pthread_cond_t *cond, int private)
>>>  {
> …
>>> +      int e;
>>> +
>>> +      e = __futex_lock_pi64 (&cond->__data.__pi_lock, 0, NULL, private);
>>> +      if (e != 0)
>>> +        futex_fatal_error ();
>>
>> The __futex_lock_pi64 already issues futex_fatal_error() non-expected return code;
>> so I think we should only use it futex-internal.{c,h}.
> 
> There shouldn't be any error. There might be the case where the lock
> owner is gone (ESRCH I believe) or the theoretical ENOMEM. ESRCH isn't
> handled now but it can't be recognized either. It would require to kill
> the thread owning the lock.
> So either abort the operation if the futex-op returns an error because
> "this shouldn't happen" or I don't know.
> 

On pthread_mutex_lock we handle ESRCH only for non-robust PI mutexes, and
we delay the thread indefinitely. I am not sure which is the best approach,
but I am tending for the delay one for consistency.

>> And I think we will need to handle ENOSYS here, maybe to use the old locking logic
>> if the kernel does not support FUTEX_PI. I think it should be fair to assume that
>> if your running your workload on an environment without FUTEX_PI support you are
>> not subject to the issue you raised.
> 
> Yes, that is fair to assume. Also given the above it might be a
> leftover. But let me add the fallback code and then we can remove once
> it is certain on Kernel's side.

I think we won't be able to remove because we still need to handle older
kernels that might return ENOSYS.

> 
>> I am ccing Carlos, he has helped fix some issue on cond vars recently and he
>> might have some idea if using PI aware locks does make sense here.
>>
>> Also, I think we really need regression testcases for this change.
> 
> I came up with something to test this. It required a few CPUs and task
> pinning otherwise the small window didn't trigger.

Right, I think the idea would to provide a way to over multiples runs from
multiple developers in multiple machines we can assume that this is triggered.
I agree that for such problems is really hard to come up with reliable test
cases without spending a lot of cpu time.

> 
>>>      }
>>>  }
> 
> Sebastian