Event Manager: Make one-shot a per-registration property
ClosedPublic

Authored by bgamari on Oct 17 2014, 10:06 AM.

Details

Summary

Currently the event manager has a global flag for whether to create
epoll-like notifications as one-shot (e.g. EPOLLONESHOT, where an fd
will be deactivated after its first event) or standard multi-shot
notifications.

Unfortunately this means that the event manager may export either
one-shot or multi-shot semantics to the user. Even worse, the user has
no way of knowing which semantics are being delivered. This resulted in
breakage in the usb[1] library which deadlocks after notifications on
its fd are disabled after the first event is delivered. This patch
reworks one-shot event support to allow the user to choose whether
one-shot or multi-shot semantics are desired on a per-registration
basis. The event manager can then decide whether to use a one-shot or
multi-shot epoll.

A registration is now defined by a set of Events (as before) as well as
a Lifetime (either one-shot or multi-shot). We lend monoidal structure
to Lifetime choosing OneShot as the identity. This allows us to combine
Lifetime/Event pairs of an fd to give the longest desired lifetime of
the registration and the full set of Events for which we want
notification.

[1] https://github.com/basvandijk/usb/issues/7

Test Plan

Add more test cases and validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bgamari updated this revision to Diff 894.Oct 17 2014, 10:06 AM
bgamari retitled this revision from to Event Manager: Make one-shot a per-registration property.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
bgamari added a reviewer: austin.
bgamari updated this revision to Diff 896.Oct 17 2014, 12:06 PM
bgamari edited edge metadata.

Fixed import cycle and various warnings

bgamari updated this revision to Diff 897.Oct 17 2014, 12:10 PM
  • Kill partition definition

Andreas, Johan, please take a look over this when you get a chance.

tibbe edited edge metadata.Oct 18 2014, 2:47 AM

This only affects users who use GHC.Event directly, correct? Users who just use the standard blocking calls (implemented using threadWaitRead/Write) are unaffected, right?

Currently this patch should have no effect on the semantics of any externally visible interfaces. registerFd uses one-shot semantics when possible just as before. For users that need control over the lifetime of the registration the patch adds registerFd'.

I'm not entirely pleased with the fact that we now export three registerFd variants so perhaps we want to simply add a lifetime parameter to registerFd and update the existing users (GHC.Event.Thread, which provides threadWait* is one). What do others think?

bgamari updated this revision to Diff 968.Oct 23 2014, 11:35 PM
bgamari edited edge metadata.
  • Event: Add comment
  • Add missing INLINEs
ezyang removed a subscriber: ezyang.Oct 27 2014, 2:04 PM

What's the plan here? At this point I'd be alright with just merging for 7.10. In this case I'd probably want to drop the new registerFd variant.

libraries/base/GHC/Event/Internal.hs
100

Can you add one more sentence of commentary here, explaining that the Int inside EventLifetime encodes the event type in the low 3 bits (using same encoding as Event) and uses bit 4 to encode lifetime.

111

Nitpick: go isn't the clearest name. How about something like multiShotBit?

libraries/base/GHC/Event/Manager.hs
381

Will this make two passes over the fdds? Would the following be more efficient:

foldl' (\e fdd -> e `mappend` fdEvents fdd) mempty fdds
libraries/base/GHC/Event/Manager.hs
336

eventsOf occurs several times in this function, including twice in this expression here. I wonder if we can simplify and maybe even improve performance a tad...

At the least, we could introduce let prevEvs = eventsOf prev in lines 337 and 338.

We could go a bit further, since the same computation occurs in line 322. How about we introduce prevEvs = maybe mempty eventsOf oldFdd just above line 321. Then el' = mappend el prevEvs, line 337 becomes I.elEvent prevEvs and line 338 becomes I.elEvent el'.

In fact, this simplifies further, since now newEvs = I.elEvent el' and oldEvs = I.elEvent prevEvs in both branches of the case expression. Therefore, we could eliminate the case expression entirely and define modify = prevEvs /= el' and then fix up line 341.

Thanks for the review; I'll take care of these shortly.

libraries/base/GHC/Event/Manager.hs
484

This looks like a de-optimization over the previous code. Previously (see lines 461-463) the code avoided executing any command on the backend in the case that nothing needs to be re-armed and the files were registered with one shot mode, which I expect will be by far the common case. (Actually the old code looks like it has a bug in this function when haveOneShot is True and the manager is started in non one shot mode, but your patch fixes this :) ).

To keep the same optimization in the new code, we should do something like:

case I.elLifetime savedEls of 
 OneShot | haveOneShot -> 
  unless (OneShot == I.elLifetime (eventsOf triggered) && mempty == savedEls) $ 
   I.modifyFdOnce (emBackend mgr) fd (I.elEvent savedEls)
 _ -> ... as before ...
libraries/base/GHC/Event/Manager.hs
381

I wonder how well GHC deals with applications of eventsOf to the empty list? In other words, would it help to add a clause for []:

eventsOf [] = mempty

selectCallbacks computes this (line 481 in particular) so it may be important.

libraries/base/GHC/Event/Manager.hs
14

Typo: delete "set of"?

In D347#12085, @bgamari wrote:

What's the plan here? At this point I'd be alright with just merging for 7.10. In this case I'd probably want to drop the new registerFd variant.

I would also be in favor of dropping the old registerFd behavior and renaming your new registerFd' to registerFd, and fixing GHC.Event.Thread.

Also, can we make GHC.Event.Manager.registerFd_ non-exported? GHC.Event.Thread does not use it, but it is exported from GHC.Event. The modify return value from registerFd_ seems like an internal detail of the library; I'd rather not expose that to clients.

bgamari updated this revision to Diff 1663.Nov 22 2014, 3:23 PM
bgamari edited edge metadata.
  • Event: Add comments
  • Event: Rework handling of one-shot events
  • Address Andreas's comments
  • registerFd now takes Lifetime
  • Don't export registerFd_
bgamari updated this revision to Diff 1667.Nov 22 2014, 5:14 PM
bgamari edited edge metadata.
  • Fix comment typo
  • Avoid unnecessarily modifying fd

Oh dear, it seems you need to submit a comment before in-line comments are made visible. Here there are.

libraries/base/GHC/Event/Internal.hs
100

Sure.

111

lifetimeBit it is.

libraries/base/GHC/Event/Manager.hs
336

Yep, looks good to me.

381

I was thinking this would fuse but I have not tested this.

484

Hmm, I had intended to do this but I guess it got lost at some point. Thanks for catching this.

I believe this should be ready to go. I'm willing to fix the lint issue if people really think that it would be an improvement.

@austin, what is the status of this? Are we alright for 7.10?

@austin, is anything needed from me here?

austin accepted this revision.Jan 12 2015, 4:02 AM
austin edited edge metadata.

OK, sorry this took so long. LGTM; @bgamari, push when you want.

This revision is now accepted and ready to land.Jan 12 2015, 4:02 AM

No worries! Thanks!

This revision was automatically updated to reflect the committed changes.

Today I realized that there are unfortunately a number of issues with this patch. Most notably, the patch does not properly save callbacks for multishot registrations (or for that matter one-shot registrations that were not triggered). For better or for worse it doesn't appear to be very common to have multiple registrations against the same fd so it's unlikely that these will otherwise quite serious bugs will be manifested in users' code. I have opened Trac #10317 to track this and D849 with a fix.

I also believe that using eventsOf triggered in the criterion for avoiding the call to modifyFdOnce isn't quite right; this should be eventsOf allEls.