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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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.

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.

> > --- 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.

> 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 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.

> >      }
> >  }

Sebastian