eio_linux: make it safe to share FDs across domains

[talex5]

It was previously not safe to share file descriptors between domains. The fd_sharing.md test demonstrates the problem: one domain enqueues an operation using an FD and another domain closes it first.

Eio_unix.Private.Rcfd provides a ref-counted wrapper for file descriptors to fix this problem. It's lock-free and designed to be fast in the common case where an FD is only used from one domain, but safe in the case where it's used from several.

The bench_fd benchmark is, if anything, slightly faster with this change, though I don't know why:

There is a dscheck test that tests Rcfd with all combinations of two users and two closers.

It includes an (unsafe) peek operation, as the public API already provided this. We might want to remove that at some point and provide a safe Eio_unix.FD.use operation instead.

There is support here for safely having two domains try to close an FD at once, although eio_linux itself still doesn't support that (you must close from the domain that opened it).

It could also allow us to support a try_close operation at some point.

Note that closing is now always done with Unix.close, not uring. Using uring previously avoided races in the single-domain case, but that's not necessary now that we have a general solution. I did have a remove_or_close function that would use uring in the common case, but Unix.close if an operation was in progress, but that seemed over-complicated.

I'm intending to use this for the generic eio_posix backend too.

[talex5]

Notes:

• It's easiest to review the eio_linux.ml changes with whitespace off. It's mostly just moving the extraction of the FD out of the scheduler context and into the fiber context (where it's easier to deal with exceptions too).
• The is_tty check is no longer lazy, since lazy isn't domain-safe.
• The pool of never-freed eventfds has gone :-)

[haesbaert]

On a preliminary review it seems like a sane approach, but I keep wondering, would it not make it easier to condensce both states (the reference counting and the Open/Close) into one Atomic.t ? This way you don't need to do get/put dances, something like:

type state =
  | Open of (int * Unix.file_desc)
  | Closing of (int * (unit -> unit))

On use you would Atomic.compare_and_set (Open (x, fd)) (Open (succ x, fd), and closing would be the inverse dance. Open -> Close would only happen on actual close and would carry the count to the new state.

[talex5]

I didn't benchmark that, but doing two allocations on every use seemed bad. The current code is only updating an integer and reading a shared location, so no allocations.

Also, I think compare_and_set is generally slower than Atomic.incr, though I don't have figures.

[haesbaert]

I didn't benchmark that, but doing two allocations on every use seemed bad. The current code is only updating an integer and reading a shared location, so no allocations.

Also, I think compare_and_set is generally slower than Atomic.incr, though I don't have figures.

Another idea to avoid it is to use the most significant bit of the reference counting as the Closing state, so you don't need any allocations. I'm mentioning since it's way easier to reason about a protocol with only one atomic word instead of two.

[haesbaert]

I think I've found a race, see if I'm making any sense

[haesbaert]

Assume Domain A is here, it did the 1->0 transition, did not see t.ops be incremented and proceeded to call no_users.

[haesbaert]

Domain B gets here, meanwhile Domain A already decided to close with no_users.
Domain B sees Closing since the atomics are ordered, it aborts by doing put on line 97.
B sees the same sequence of events of A in put, and ends up calling no_users as well, so you end up with a double close. The assumption is that the no_close of A would be carried over to B, which I think doesn't hold here, both end up calling it.

[talex5]

put always does a CAS to Closing ignore before calling no_users, so the real function can only get called once.

[haesbaert]

ahA! indeed, I've missed the ignore in fully_closed

[talex5]

Another idea to avoid it is to use the most significant bit of the reference counting as the Closing state, so you don't need any allocations.

That might work... let me give it a try...

[talex5]

After more thought, I don't think using one bit of ops to indicate "closing" helps much.

t.fd still needs to be atomic because we still need to store the callback function (at least for remove). We can't have two closers overwriting each other's callbacks, and we can't have two put calls both calling the same callback.

So, it's still two atomic locations, but working with t.ops becomes harder as it's not just an int any more.

I didn't rewrite it all to check, but suspect it won't be much/any simpler (though you're welcome to have a go - it might turn out OK).

[haesbaert]

Ah indeed, the callback complicates it.
I wanna have a final review later today but I think it's fine as it is.

I understand this solves the re-use race which is dangerous, do you also have something in mind to "cancel the operation on other domains ?"

The way it is after this change. If multiple domains block on accept on the same fd, if one issues a close, the actual close operation will pend until every domain receives one connection.

[talex5]

I understand this solves the re-use race which is dangerous, do you also have something in mind to "cancel the operation on other domains ?" The way it is after this change. If multiple domains block on accept on the same fd, if one issues a close, the actual close operation will pend until every domain receives one connection.

The idea is you use Eio's cancellation system to cancel, not close. This is similar to Unix's behaviour too (calling close(2) in one thread will not abort an accept(2) performed on the same FD by another thread), just one level up.

Normally, if you're using things in the obvious way, the structured concurrency will have cancelled all operations by the time the switch finishes and closes the FD.