Re: [PATCH 1/3] Use reliable sem_wait interruption in nptl/tst-sem6.

[Rich Felker <dalias at libc dot org>]

On Tue, Dec 09, 2014 at 07:24:49PM +0100, Torvald Riegel wrote:
> > Which does not answer my objection. What extra bugs could this test catch,
> > compared to say tst-sem2? If there is no such bug you could just delete
> > that file.
> 
> tst-sem2 tests that spurious wake-ups and such don't return anything but
> -1 and errno==EINTR, in particular that 0 isn't returned.
> 
> After the patch, tst-sem6 tests that a signal handler that posts a token
> will make sem_wait return.  It *also* allows for sem_wait to return -1
> and errno==EINTR in that case.
> 
> Thus, one possible error that the patched tst-sem6 will catch is if the
> sem_wait itself just retries the futex_wait after the futex_wait
> returned EINTR, instead of looking for whether there is an available
> token.

This would not be a bug. Simply retrying the futex_wait would result
in EAGAIN, since the futex value would no longer match.

> > > Also, please review the actual background for this change.  See patch
> > > 3/3 for that.  The change to the test is not ideal, but please see the
> > > trade-offs.
> > 
> > The rarity of problem bugged me, as to trigger that behaviour one would
> > need run also highly parallel program so it looked unlikely that anybody
> > would report that. As it couldn't detect some bugs it previously could
> > its hard to see what is better. 
> 
> Let me try to summarize the background behind this change again:
> 
> 1) Linux documents futex_wait to return EINTR on signals *or* on
> spurious wake-ups.

No, the man pages document this, and they're wrong. I have not seen
any other "Linux documentation" claiming it.

> 2) If we treat 1) as true -- which we should to unless getting
> confirmation otherwise -- sem_wait must not return EINTR to the caller
> anymore if futex_wait returned EINTR.
> 3) Because of 2), the behavior that is tested in tst-sem6 before my
> patch cannot be implemented anymore.

These (2) and (3) are based on false assumptions. I agree this is a
positive change (EINTR is generally undesirable) and it may be
necessary if you want to support old kernels where the futex syscall
could fail with EINTR even when the signal handler was SA_RESTART
type, but I don't think glibc supports those kernels.

> Thus, we need to do *something*.  I proposed this patch, and variations.
> If you don't see other alternatives, then I guess we'll have to pick
> from the options I gave.
> 
> If you disagree with 2), then please comment on Patch 3/3, because
> that's where this belongs.

The most important thing to do is get clarification from the kernel
side that the man page is wrong and that there is no intent to
overload EINTR or allow incorrect/spurious EINTR from futex. This will
possibly affect other interfaces now and in the future, e.g.
aio_suspend. I agree that making sem_[timed]wait ignore EINTR is
desirable too, but I don't think there's any actual current problem
being fixed - the kernel is not generating spurious EINTR, and we just
need to keep it that way.

Rich