Page MenuHomePhabricator

[lldb] [MainLoop] Add kevent() EINTR handling
ClosedPublic

Authored by mgorny on Feb 14 2019, 4:37 AM.

Details

Summary

Add missing EINTR handling for kevent() calls. If the call is
interrupted, return from Poll() as if zero events were returned and let
the polling resume on next iteration. This fixes test flakiness
on NetBSD.

Includes a test case suggested by Pavel Labath on D42206.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Feb 14 2019, 4:37 AM

We already had a patch for this, but it kind of got forgotten while we were figuring out how to test it. I had made a test for the behavior here https://reviews.llvm.org/D42206#980045. Could you please try it out and incorporate it into the patch (or write some other kind of test for this fix)?

For EINTR we shall use llvm::sys::RetryAfterSignal

For EINTR we shall use llvm::sys::RetryAfterSignal

kevent() man page indicates:

All changes contained in the changelist are applied before any pending events are read from the queue.

Also:

[EINTR] A signal was delivered before the timeout expired and before any events were placed on the kqueue for return.

So while it's not stated explicitly, I think in_events is always consumed, even if EINTR is returned. In which case, llvm::sys::RetryAfterSignal would be wrong whenever in_events is not empty.

mgorny updated this revision to Diff 186915.Feb 14 2019, 1:57 PM
mgorny edited the summary of this revision. (Show Details)

Added the test case.

For EINTR we shall use llvm::sys::RetryAfterSignal

kevent() man page indicates:

All changes contained in the changelist are applied before any pending events are read from the queue.

Also:

[EINTR] A signal was delivered before the timeout expired and before any events were placed on the kqueue for return.

So while it's not stated explicitly, I think in_events is always consumed, even if EINTR is returned. In which case, llvm::sys::RetryAfterSignal would be wrong whenever in_events is not empty.

Please bring it to tech-kern@ to clear this and improve documentation. I think that we need to use llvm::sys::RetryAfterSignal.

labath accepted this revision.Feb 14 2019, 11:12 PM

The change looks fine to me. I don't understand all the intricacies of the RetryAfterSignal discussion (AFAICT, it does not matter what you do there, since RunImpl::Poll is called in a loop anyway), so I'll let you sort that out with Kamil.

This revision is now accepted and ready to land.Feb 14 2019, 11:12 PM

@mgorny let's go through tech-kern@ and later checking FreeBSD/Darwin/OpenBSD. I think it's worth to clarify this in the documentation.

As an intermediate version we can land this patch.

FTR, I've just verified this against NetBSD kernel sources and -- as I suspected and as I believe any reasonable implementation would do -- the only place where EINTR could be returned is when kevent() is sleeping, waiting for new events to appear in the event queue. The changelist is always consumed.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 4:13 AM