Implement an eventfd-based ping source for Linux. #92

detly · 2022-04-03T02:36:52Z

This separates and reorganises the underlying mechanics for the ping source, and implements an eventfd-based ping for Linux.

~~Currently a draft, but close to final.~~ Partly addresses #15, maybe there's an analogue for BSD-ishes?

Eventfd has a bit of a quirk I didn't think about until I got halfway through this. It gives you one fd instead of two like a pipe. This means that if you close that fd, eg. through CloseOnDrop, the receiving end's fd becomes invalid and so does the Generic source wrapping it. In practice you just get some EBADFD errors, but theoretically if the fd is reassigned you could get some extreme weirdness.

I solved this by (a) adding a dup() system call at creation to create separate reading and writing fds. Closing a duplicated fd does not affect others that are open, solving the first problem.

The other problem is that the source's fd has no way of signalling that the other fd has been closed. Adding another event source to handle this seemed like it would cancel out the performance/complexity benefits of using eventfd in the first place. I have attempted to solve this by using an AtomicBool flag wrapped in an Arc, keeping the sending handle thread-safe and cloneable.

This sort of begged the question for me though, how does the pipe-based ping handle this? The event-source-code seems to suggest that reading zero bytes from the pipe after it becomes readable means that the other end was closed. But I can't find any documentation saying that closing a pipe end will make the other end become readable in the first place. Anyway, it might be the case that there's no foolproof way to handle this. I'd welcome any suggestions.

Other notes:

I marked some functions as #[inline], not so much for performance reasons but more to indicate that they're just used as interchangeable pieces in the other helper functions.
There's a common structure between the pipe and eventfd files (send_ping(), drain_ping() etc) even though these are private functions. It's not a suggestion that this structure be enforced. It's just that breaking those functions out of the pipe impl helped me build them back up for the eventfd impl. If you prefer them back in the event source, that's fine by me.
I will add some of these notes in as comments, but perhaps after feedback in case there are any fundamental changes.
This PR adds cfg-if as a dependency. It should have no runtime cost. I think it helps the implementation switching a bit.

Docs/links:

codecov · 2022-04-03T02:41:39Z

Codecov Report

Merging #92 (b223766) into master (7835e53) will decrease coverage by 0.01%.
The diff coverage is 93.91%.

❗ Current head b223766 differs from pull request most recent head 4a442f2. Consider uploading reports for the commit 4a442f2 to get more accurate results

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
- Coverage   89.75%   89.74%   -0.02%     
==========================================
  Files          15       17       +2     
  Lines        1582     1658      +76     
==========================================
+ Hits         1420     1488      +68     
- Misses        162      170       +8

Flag	Coverage Δ
macos-latest	`89.32% <95.77%> (-0.14%)`	⬇️
ubuntu-latest	`88.21% <91.13%> (-0.16%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/sources/ping.rs	`85.71% <50.00%> (-10.39%)`	⬇️
src/sources/ping/eventfd.rs	`92.20% <92.20%> (ø)`
src/sources/ping/pipe.rs	`97.10% <97.10%> (ø)`
src/sys/mod.rs	`90.75% <0.00%> (-0.85%)`	⬇️
src/loop_logic.rs	`88.93% <0.00%> (-0.43%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7835e53...4a442f2. Read the comment docs.

kchibisov · 2022-04-03T04:53:45Z

The event-source-code seems to suggest that reading zero bytes from the pipe after it becomes readable means that the other end was closed.

From read(2) it says "On success, the number of bytes read is returned (zero indicates end of file), and the file position is advanced by this number."

Also, the way it works is due to closing triggering wakeup due to fd event leading to read from pipe read fd, while the write end is closed resulting in getting 0, since we've reached EOF.

detly · 2022-04-03T05:05:38Z

Oh thanks, now I know! Unfortunately it doesn't apply to eventfd AFAICT. I don't think closing the dup-ed triggers an event either. I tried to test this behaviour (and its absence), but couldn't come up with a test for it.

detly · 2022-04-03T05:10:45Z

Also, the way it works is due to closing triggering wakeup due to fd event leading to read from pipe read fd, while the write end is closed resulting in getting 0, since we've reached EOF.

I think there's still an issue with this though. The read loop in the event source will drain all bytes from the pipe on an event, probably as it should. But if you imagine a sequence of events on the sender: ping, followed by drop/close, before the next dispatch cycle... I think you'll get one event, read the bytes, finish on the Ok(0) arm, break, and keep the ping source in the loop. Right?

kchibisov · 2022-04-03T05:23:42Z

I think you'll get one event, read the bytes, finish on the Ok(0) arm, break, and keep the ping source in the loop. Right?

With your new code yes, the old code handled that fine. So you should follow its behavior.

    let mut action = PostAction::Continue;
                loop {
                    match read(fd, &mut buf) {
                        Ok(0) => {
                            // The other end of the pipe was closed, mark ourselved to for removal
                            action = PostAction::Remove;
                            break;
                        }
                        Ok(_) => read_something = true,

                        Err(e) => {
                            let e: std::io::Error = e.into();

                            if e.kind() == std::io::ErrorKind::WouldBlock {
                                // nothing more to read
                                break;
                            } else {
                                // propagate error
                                return Err(e);
                            }
                        }
                    }
                }
                if read_something {
                    callback((), &mut ());
                }
                Ok(action)

As you can see it has 2 flags action and read_something, so it'll read and drop in this case.

detly · 2022-04-03T05:24:57Z

I didn't think my new code changed the functionality of the pipe-based ping source?

kchibisov · 2022-04-03T05:28:08Z

I didn't think my new code changed the functionality of the pipe-based ping source?

I mean you clearly did, since you're not dropping event source in the case you've described above, while the current ping source will drop, look at the loop it had before with match read.

detly · 2022-04-03T05:32:50Z

Okay I wrote this out and see I dropped the PostAction::Remove from L102. Let me fix that.

detly · 2022-04-03T05:40:38Z

The pipe-based ping behaviour is restored. The eventfd source still has that issue though. I think adding a final ping send to the drop handler for the sending handle might work.

kchibisov · 2022-04-03T05:50:16Z

The other problem is that the source's fd has no way of signalling that the other fd has been closed. Adding another event source to handle this seemed like it would cancel out the performance/complexity benefits of using eventfd in the first place. I have attempted to solve this by using an AtomicBool flag wrapped in an Arc, keeping the sending handle thread-safe and cloneable.

I don't see major issue with it? The point is to have less file descriptors open, no? if you share bool flag between sender and receiver it should be fine? Also, you could bench pipe and eventfd I guess, not sure what the way of doing so should be.

detly · 2022-04-03T06:06:17Z

I don't see major issue with it?

I'm not sure what detail you mean here. The problem I'm trying to avoid is event sources that don't return PostAction::Remove when all the sending handles have been dropped. The boolean flag itself is not enough, because it will not necessarily wake up the event source to read it and see that it needs removal.

Also, you could bench pipe and eventfd I guess, not sure what the way of doing so should be.

I don't really care about performance, in the vague sense of the word, but maybe Calloop and its other users have a specific idea of things they want to avoid. Ping is such a fundamental building block of the event loop that I'd really like to avoid unnecessary increases in resource usage (especially since eventfd is meant to do the opposite), whether that's processing, memory, or fds.

kchibisov · 2022-04-03T06:13:26Z

The boolean flag itself is not enough, because it will not necessarily wake up the event source to read it and see that it needs removal.

You can ping in drop right before setting bool flag, check that bool flag is set and instead of calling callback drop the source.

kchibisov · 2022-04-03T06:17:56Z

And in case you ping and drop right away you can pass bitmask along the line e.g. READ | DROP instead of plain boolean flag.

detly · 2022-04-03T07:17:49Z

And in case you ping and drop right away you can pass bitmask along the line e.g. READ | DROP instead of plain boolean flag.

I was implementing this and I realised we can do this purely with the event counter! Write 2 for "real" ping events, and 1 for drop events. Then we just have to bit-test the LSB and if it's set, we're closed.

detly · 2022-04-03T07:52:19Z

See latest for this implementation and remarks. It's still a draft because I'd really like to add a test for this. I'll have time for that in a few hours.

detly · 2022-04-03T09:07:41Z

I've added two tests for (a) ping source removal and also (b) simultaneous ping and source removal. They fail for my previous code and pass for the fixed code. No longer a draft, feedback welcome!

kchibisov · 2022-04-03T09:49:28Z

I was implementing this and I realised we can do this purely with the event counter! Write 2 for "real" ping events, and 1 for drop events. Then we just have to bit-test the LSB and if it's set, we're closed.

What if you concurrently drop 2 ping sources? You'll add 2 from 2 drop impls and it won't be dropped? I was also thinking about using counter don't get me wrong it, but I wasn't able to figure out how you'd do that in a nice way... You could probably use Weak for wake up sources and check for weak ref count, so if it's zero everything is dropped.

elinorbgr

That looks very nice, thanks a lot 👍 I just have a few nitpicks.

elinorbgr · 2022-04-03T09:50:02Z

src/sources/ping/eventfd.rs

+    // We only have one fd for the eventfd. If the sending end closes it when
+    // all copies are dropped, the receiving end will be closed as well. We can
+    // avoid this by duplicating the FD.
+    let write = dup(read)?;


If this fails we are leaking a file descriptor

Can we use Arc instead of dup here? I don't see a reason to dup it. We could have Weak<Fd> pointers in senders and check in receiver ref count and drop fd it goes to zero.

We could have Weak pointers in senders and check in receiver ref count and drop fd it goes to zero.

This would work if we can be sure the weak count (ie. number of senders) will never go to zero and then come back up again. Can anyone think of a situation where that might happen? (I can't.)

I'm not quite sure I get how your suggestion would work out @kchibisov

Do you mean checking the refcount every time the source is woken up, and drop the source if it reaches zero? It seems to me that it implies waking up the source every time a Ping is dropped, and how would we be able to distinguish that from a regular ping()? Dropping a Ping handle would cause spurious events on the event source, no?

Yeah I just tried this approach and hit this exact problem. Spurious events aren't such a big deal for me (you get a lot of them with the async executor for example, depending on what's in there), but yes, there's that. And then there's the "what kind of wakeup was this" issue, which means we'd still need the FlagOnDrop trick, which means two levels of Arc (on for sharing the fd between senders and source, and one for sharing it between just the senders).

Do I understand that the only point of using Weak here is so that the FD is closed as soon as the PingSource is dropped, rather than once the PingSource and all Pings are dropped?

Sort of. It was partly that, and also so that simply holding on to a Ping did not keep the PingSource's underlying eventfd alive.

I see your point though, the implementation would be simpler if it was Arcs all the way.

Latest version has full Arcs all the way through. I've left the intermediate commits in in case you want to compare, but I'll squash them when we're happy with it.

I don't think that's a blocking point though, that was just to make sure I understand the point.

I think I prefer the full-Arc version, it is simpler in general.

src/sources/ping/eventfd.rs

detly · 2022-04-03T10:02:04Z

What if you concurrently drop 2 ping sources? You'll add 2 from 2 drop impls and it won't be dropped?

Eventfd works as an atomic counter, not a pipe, so the numbers are added. The result of that sequence would be that the event source reads 5, and the masking does the rest.

kchibisov · 2022-04-03T10:05:12Z

Eventfd works as an atomic counter, not a pipe, so the numbers are added. The result of that sequence would be that the event source reads 5, and the masking does the rest.

That's what I'm saying. Ping + Ping + Drop + Drop is 2 + 2 + 1 + 1 == 6 is the same as Ping + Ping + Ping.

The Drop is twice, due to parallel drop of 2 sources. Should work with any even amount of sources dropped at the same time.

kchibisov · 2022-04-03T10:20:55Z

On the other side, I don't really think that automatic drop should be performed in the first place? The close should occur in the Drop impl. If the source is being removed from the event loop, the Drop will be called meaning that we can safely close it, no? There shouldn't be any ref counting in the first place.

If we've dropped all the senders we shouldn't remove source, since I think it could make sense to have ability to recreate ping wake up sources later on?

Anyway, it's more up to @vberger I guess wrt ping design, I'm mostly saying that automatic drop is a bit intense here.

detly · 2022-04-03T10:21:33Z

The Drop is twice, due to parallel drop of 2 sources. Should work with any even amount of sources dropped at the same time.

Note that the FlagOnDrop is wrapped in an Arc, so even across threads the drop can only happen once. If it didn't, there would actually be two bugs: (1) is exactly what you describe, and (2) is a double close on the wrapped fd. (2) would also apply to the pipe-based source I think due to the similar CloseOnDrop wrapper.

elinorbgr · 2022-04-03T10:26:57Z

If we've dropped all the senders we shouldn't remove source, since I think it could make sense to have ability to recreate ping wake up sources later on?

Isn't that covered by just creating a new PingSource and inserting it in the event loop? I have a feeling that if you want to keep a specific ping source (and its callback) alive, you just keep at least one copy of its associated Ping around (because you plan to use it at some point anyway)... 🤔

detly · 2022-04-03T10:56:46Z

If we've dropped all the senders we shouldn't remove source, since I think it could make sense to have ability to recreate ping wake up sources later on?

The removal via process_events() return value helps a lot with dynamically created sources and senders. I don't know if you've seen #45 but that explains the motivation for it, and it's been useful for eg. channels.

The close should occur in the Drop impl. If the source is being removed from the event loop, the Drop will be called meaning that we can safely close it, no?

Note that this is still the case: if process_events() returns Remove, the loop logic will remove it internally, drop it, and then the drop handler does the close.

elinorbgr

LGTM, maybe could you just add a note about this change in the changelog?

kchibisov · 2022-04-04T10:11:20Z

Cargo.toml

@@ -15,6 +15,7 @@ readme = "README.md"
 codecov = { repository = "Smithay/calloop" }

 [dependencies]
+cfg-if = "1.0.0"


Do we really need cfg if? Right now we're handling cases without it just fine. Not sure it worth?

Sure, it was necessary when I split out the macos pipe vs. non-macos pipe2 implementations, but probably not any more.

kchibisov · 2022-04-04T10:14:04Z

src/sources/ping/eventfd.rs

+pub struct Ping {
+    // This is an Arc because it's potentially shared with clones. The last one
+    // dropped needs to signal to the event source via the eventfd.
+    event: Arc<FlagOnDrop>,


There is no need for nested arcs? The FlagOnDrop is already an arc? Otherwise whats the purpose of nesting arcs?

If you clone the FlagOnDrop throughout all the senders, a different one will be dropped every time a sender is dropped. This will (a) wake the source up every time with send_ping() in its drop handler and (b) hit the problem you mentioned elsewhere, where the counter may be incremented by 2 (or an even number) instead of 1 if two are dropped in between loop dispatches and (c) remove the ability to even know when the last sender is dropped.

There must strictly be one FlagOnDrop in existence for any given PingSource for the signalling to work.

To put it another way: Arcs are for shared ownership.

The inner level of Arc is for shared ownership of the eventfd by both the PingSource and all Pings, original or cloned.

The outer level of Arc is for shared ownership of the single "thing that adds 1 to the eventfd counter" and "thing that knows if more than zero senders exist" amongst all the Ping clones.

They do not conceptually share ownership of the same thing, so you need two levels.

Here's a quick picture. Note how the top Arc (the one containing FlagOnDrop) is not shared with PingSource and ensures that FlagOnDrop is uniquely responsible for adding numbers to the eventfd counter.

detly · 2022-04-04T11:48:42Z

LGTM, maybe could you just add a note about this change in the changelog?

Which change? Github won't show me what this refers to.

Edit: oh you meant the whole thing, yes, good point.

kchibisov · 2022-04-04T12:01:38Z

Actually, when dropping ping, can you check the ref count. And if it is the last ping to drop you write 1, otherwise you just silently drop? I think it should work and simplify a lot avoiding wakeups.

kchibisov · 2022-04-04T12:13:50Z

Thats some pseudo code but you should get an idea

struct Ping {
    fd: Weak<EventFd>,
}
impl Ping {
    fn ping() {
        self.fd.write(2);
    }
}

impl Drop for Ping {
    fn drop(&mut self) {
        if fd.weak_count() == 1 {
            self.fd.write(1);
        }
    }
}

impl PingSource {
    fn process_events() {
        // ....
        bool should_close = false;
        match fd.read() {
            Ok(n) if n == 1 {
                should_close = true;
            }
            Ok(n) {
                callback();
                should_close = n % 2 == 0;
            }
            _ => // nothing...
        }
        if should_close {
            std::mem::drop(self.fd);
        }
        // ....
    }
}

elinorbgr · 2022-04-04T17:00:58Z

impl Drop for Ping {
    fn drop(&mut self) {
        if fd.weak_count() == 1 {
            self.fd.write(1);
        }
    }
}

I think that part is racy, if the last two Ping are dropped concurrently drop two threads, both could see weak_count == 2 and decide to not write.

kchibisov · 2022-04-04T17:52:40Z

I think that part is racy, if the last two Ping are dropped concurrently drop two threads, both could see weak_count == 2 and decide to not write.

Yeah, and I don't see a way to decrement and get value of ref count. There could be a Mutex just for drop to ensure thread safety.

Since right now if you drop from 2 threads at the same time you won't drop, since you write 0x1 from both resulting in 0x2, since it's counter.

Yeah, even if you try speculating with Arc::upgrade it won't work without Mutex...

elinorbgr · 2022-04-04T18:05:06Z

Since right now if you drop from 2 threads at the same time you won't drop, since you write 0x1 from both resulting in 0x2, since it's counter.

I'm not sure what you mean by "right now", the PR as it is currently does not have this issue, thanks to the two layers of Arc

kchibisov · 2022-04-04T18:35:23Z

Yeah, maybe double arc isn't that bad.

elinorbgr · 2022-04-04T19:19:05Z

In any case, I think this is deep into optimization land, and worst case this can always be improved in a future PR if we find a nice way to do it.

detly · 2022-04-05T00:59:03Z

Since right now if you drop from 2 threads at the same time you won't drop, since you write 0x1 from both resulting in 0x2, since it's counter.

FYI this is the promise of both an Arc vs Rc, and of Drop - in Arcs case, the count is changed atomically even from multiple threads, and for Drop, the drop handler is only ever called once (otherwise there would be a lot of unsafe code out there). So the write of 1 can only ever happen once, as long as there is a unique FlagOnDrop for every PingSource.

Re. raciness, see this note from the Arc docs:

This method by itself is safe, but using it correctly requires extra care. Another thread can change the weak count at any time, including potentially between calling this method and acting on the result.

detly · 2022-04-05T07:20:26Z

~~@vberger Do you want me to also add a note about MSRV change to 1.53 in the changelog, or would you rather keep it separate?~~

Never mind, I was reading it backwards.

detly · 2022-04-05T07:25:13Z

Changelog updated.

This separates and reorganises the underlying mechanics for the ping source, and implements an eventfd-based ping for Linux.

elinorbgr · 2022-04-05T08:31:00Z

Alright thanks, let's merge this. 👍

detly force-pushed the eventfd-ping-source branch from 3eae6f8 to 537d859 Compare April 3, 2022 05:37

detly force-pushed the eventfd-ping-source branch from 537d859 to a8f60c4 Compare April 3, 2022 07:51

detly force-pushed the eventfd-ping-source branch 2 times, most recently from 0c9cb1b to d892e39 Compare April 3, 2022 09:06

elinorbgr reviewed Apr 3, 2022

View reviewed changes

detly force-pushed the eventfd-ping-source branch 2 times, most recently from fe9306e to f3accfc Compare April 3, 2022 14:15

detly force-pushed the eventfd-ping-source branch 3 times, most recently from b223766 to d3e16d4 Compare April 4, 2022 01:54

elinorbgr approved these changes Apr 4, 2022

View reviewed changes

kchibisov reviewed Apr 4, 2022

View reviewed changes

detly force-pushed the eventfd-ping-source branch from d3e16d4 to ac059e4 Compare April 4, 2022 11:42

detly force-pushed the eventfd-ping-source branch from ac059e4 to a67260f Compare April 5, 2022 07:25

Implement an eventfd-based ping source for Linux.

4a442f2

This separates and reorganises the underlying mechanics for the ping source, and implements an eventfd-based ping for Linux.

detly force-pushed the eventfd-ping-source branch from a67260f to 4a442f2 Compare April 5, 2022 07:30

elinorbgr merged commit cb34dce into Smithay:master Apr 5, 2022

Implement an eventfd-based ping source for Linux. #92

Implement an eventfd-based ping source for Linux. #92

Conversation

detly commented Apr 3, 2022 • edited Loading

codecov bot commented Apr 3, 2022 • edited Loading

Codecov Report

kchibisov commented Apr 3, 2022

detly commented Apr 3, 2022

detly commented Apr 3, 2022

kchibisov commented Apr 3, 2022 • edited Loading

detly commented Apr 3, 2022

kchibisov commented Apr 3, 2022

detly commented Apr 3, 2022

detly commented Apr 3, 2022

kchibisov commented Apr 3, 2022

detly commented Apr 3, 2022

kchibisov commented Apr 3, 2022

kchibisov commented Apr 3, 2022

detly commented Apr 3, 2022

detly commented Apr 3, 2022

detly commented Apr 3, 2022 • edited Loading

kchibisov commented Apr 3, 2022

elinorbgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detly commented Apr 3, 2022

kchibisov commented Apr 3, 2022 • edited Loading

kchibisov commented Apr 3, 2022

detly commented Apr 3, 2022

elinorbgr commented Apr 3, 2022

detly commented Apr 3, 2022

elinorbgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detly Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

detly Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

detly commented Apr 4, 2022 • edited Loading

kchibisov commented Apr 4, 2022 • edited Loading

kchibisov commented Apr 4, 2022

elinorbgr commented Apr 4, 2022

kchibisov commented Apr 4, 2022 • edited Loading

elinorbgr commented Apr 4, 2022 • edited Loading

kchibisov commented Apr 4, 2022

elinorbgr commented Apr 4, 2022

detly commented Apr 5, 2022

detly commented Apr 5, 2022 • edited Loading

detly commented Apr 5, 2022

elinorbgr commented Apr 5, 2022

detly commented Apr 3, 2022 •

edited

Loading

codecov bot commented Apr 3, 2022 •

edited

Loading

kchibisov commented Apr 3, 2022 •

edited

Loading

detly commented Apr 3, 2022 •

edited

Loading

kchibisov commented Apr 3, 2022 •

edited

Loading

detly Apr 4, 2022 •

edited

Loading

detly Apr 4, 2022 •

edited

Loading

detly commented Apr 4, 2022 •

edited

Loading

kchibisov commented Apr 4, 2022 •

edited

Loading

kchibisov commented Apr 4, 2022 •

edited

Loading

elinorbgr commented Apr 4, 2022 •

edited

Loading

detly commented Apr 5, 2022 •

edited

Loading