[PATCH] nptl: futex_lock_pi deadlock detection provides valuable information but it is turned into a rather cryptic assertion failure

[Adhemerval Zanella Netto]

On 08/04/26 17:07, Moritz KLAMMLER (FERCHAU) wrote:
> Thanks for your detailed review.  Those additional test support helpers
> I haven't been aware of do help simplify the code a great amount indeed.
> 
>>> +#ifndef TST_DEADLK_MUTEX_PI
>>> +#  define TST_DEADLK_MUTEX_PI 0
>>> +#endif
>>
>> Just use the TST_DEADLK_MUTEX_PI=1 and move this test to
>> tst-deadlk-pi.c.
> 
> Ah, sorry, I forgot to explain that point in my previous message.  I had
> originally added this configurability based on the assumption that we
> might want to use it to also test that non-PI mutexes actually do
> deadlock by making these XFAIL tests, but didn't went as far as actually
> doing that.  However, based on your other feedback ...
> 
>>> The added test case currently only checks for EDEADLK to be returned,
>>> but doesn't cover the cases where we still expect the deadlock to
>>> happen.  I could add these as well, but it would introduce an
>>> (unreasonably?) long delay, waiting for the alarm clock to go off
>>> eventually.  Would you rather take this delay over the lack of test
>>> coverage?  Please also let me know whether having one test executable
>>> that loops over the various types (current patch) or one executable
>>> per type would be preferred.
>>
>> I would prefer the later (one less binary to build and run). For the
>> deadlock to happen, it would be better to spawn a new process with
>> support_capture_subprocess, trigger the deadlock, and wait it with
>> short delayed_exit value.
> 
> ... I went ahead and merged everything into a single tst-deadlk.c file
> which now checks PI and non-PI mutexes alike.  Since the number of
> combinations became quite large, I refactored the test code to remove
> the explicit list in lieu of some nested loops and a simple
> should_detect_deadlock predicate function.
> 
> I've picked 3 s for a delayed_exit value, hoping that this wold be a
> sensible choice, given that nptl/tst-eintr1.c uses the same (and seems
> to be the only other NPTL test case currently using delayed_exit).
> Alas, this makes the test register approx. 30 s on the clock.  I've
> raised the overall timeout (which should never be hit) to 60 s
> accordingly.  Are you okay which such a slow test or do we need to
> improve on this?  We could, for example, do all the waiting for all
> the subprocesses in parallel, but I'm afraid that it would add quite
> some complexity.  A lower-hanging fruit might be to reduce the delay
> from 3 s to 1 s, which has worked well for my computer even with high
> concurrent CPU load, but I'm not sure how universally acceptable it
> might be.
> 
> I found that still using alarm(3) in the subprocess and then expecting
> -SIGALRM in the parent would result in slightly simpler code compared
> to the delayed_exit but since you recommended it and I've also seen
> that other test cases have been refactored in the past specifically to
> replace alarm with delayed_exit, I assume that this is preferred way
> to go.
> 
> Looking forward to your feedback on the updated patch.
Carlos, you brought to my attention that glibc current Error Handling
guideline [1] states:

  "If it's user code invoking undefined behavior, then it should fail 
   early and catastrophically so that developers don't get the false 
   impression that their code is OK when it happens not to break the 
   use cases they test adequately."

However, this patch changes the ERRORCHECK and RECURSIVE pthread types 
and, although POSIX only states a *shall fail* for self-deadlock, it does 
permit an error return for multi-mutex deadlock in the *may fail*.

This change still has the potential to break code that does not handle
EDEADLK in such cases; however this currently aborts the process anyway.
So different than I has said (sorry if I forgot all the details on Monday's
call), this is *not* UB and I think that issuing a fatal error like
futex_fatal_error is not the course of action here. 

The assert really has not place since kernel does allow to have fast deadlock
check and POSIX allows it.

[1] https://sourceware.org/glibc/wiki/Style_and_Conventions#Error_Handling
[2] https://pubs.opengroup.org/onlinepubs/9799919799/functions/pthread_mutex_lock.html