Edge-triggered epoll events #3243

ppopth · 2023-12-05T10:29:51Z

EDITED: I have already implemented this feature for tcp sockets, udp sockets, unix domain sockets, pipes, eventfds.

stevenengler · 2023-12-05T23:08:36Z

The reason why you didn't want to integrate this as you mentioned in #2673 is that Shadow already "does the right thing", but, without this feature, I have to fork the upstream repo and convince the libp2p folks to use the forked repo instead.

Just to clarify, we would like to better follow what Linux does, but we just didn't have time to work on it (look in-depth into the Linux behaviour, consider the best way to design it, any performance implications, etc). If Shadow can better model the Linux behaviour, have a bunch of tests for it, and not significantly slow down the simulator, then I think it would be great to have this behaviour. IIRC the old PR didn't pass all of the tests, and Shadow's old C epoll code was a bit difficult to follow, so we also were more worried about introducing bugs. Now with the epoll code in rust, we can hopefully be more confident in any changes.

I'll leave a few comments that I have, but I'm not as knowledgeable about epoll so I'll let other people weigh in on this PR.

I implemented it only for eventfd and legacy tcp

I think before merging we would want this to be consistent across Shadow for all file types, unless Linux does differently. And this is where tests would be useful, to make sure all of the file types behave this way. IIRC pipes also have this edge-triggered behaviour on Linux.

src/main/host/descriptor/epoll/entry.rs

src/main/host/descriptor/mod.rs

src/main/host/descriptor/eventfd.rs

src/main/host/descriptor/socket.c

robgjansen · 2023-12-07T23:09:09Z

I totally agree with @stevenengler here. We added epoll in rust but I never got around to adding the tests to help fuzz test the epoll api. Those fuzz tests would help us make sure we are following the Linux behavior, even if the Linux behavior is not the same as what is documented in the man pages. If we have the tests that show the Linux behavior and Shadow behaves exactly the same, that will give us a strong argument to merge.

stevenengler · 2023-12-08T01:09:52Z

I'd expect that this causes more state updates, resulting in the event source running more callbacks, but the current version of this PR doesn't seem to effect tor simulation runtime performance.

ppopth · 2023-12-10T15:44:57Z

I think before merging we would want this to be consistent across Shadow for all file types, unless Linux does differently. And this is where tests would be useful, to make sure all of the file types behave this way. IIRC pipes also have this edge-triggered behaviour on Linux.

Totally agree, however, since this patch is quite hacky and we don't know yet how we should implement it. I will implement the rest when we know it, otherwise the effort will be wasted.

ppopth · 2023-12-11T05:26:41Z

@robgjansen @stevenengler Are you okay with the FileState::INPUT_BUFFER_PARITY? I feel that it's probably not the most proper way to do, but I guess the most proper way may require a big change.

If you are okay to merge it with that, I will do all the necessary work on that to get it merged: fuzz tests, other tests, and edge-triggered event for all the file types.

My current feeling is that, if we don't want FileState::INPUT_BUFFER_PARITY and the proper way is too big, all the effort will be wasted.

Honestly, I'm not sure if the proper way will be really too big, I just feel that it will be. Let me think a moment to find a better way with small change.

stevenengler · 2023-12-13T07:54:30Z

@robgjansen @stevenengler Are you okay with the FileState::INPUT_BUFFER_PARITY? I feel that it's probably not the most proper way to do, but I guess the most proper way may require a big change.

I think my opinion is that it's okay. In that case the flag should have a comment explaining why it's done this way, and that it's an exception. Alternatively, what do you think of the design in #3250? Do you think this covers everything that you'd need to support epoll? It adds a new argument for listeners so that files can emit signals and listeners can opt to receive them. It might be overkill for this, but we might want this ability to emit other types of signals in the future as well.

robgjansen · 2023-12-13T13:59:47Z

I would prefer proper signal listener support from #3250 over using FileState::INPUT_BUFFER_PARITY as an exceptional case. If #3250 is merged, then this PR becomes much easier to implement using the new signaling features from #3250.

stevenengler · 2023-12-19T20:24:20Z

We've now merged #3250, so it should be ready to use in this PR.

So what we're looking for before we can merge this PR is:

use the new signal mechanism instead of the INPUT_BUFFER_PARITY state
use the new edge-triggered epoll behaviour in all file types (udp, pipe, etc) and not just eventfd/tcp, as long as linux does as well (the tests should show which files do or don't)
does this edge-triggered behaviour occur only on the input buffer, or also the output buffer?
for the input buffer, does the edge-triggered behaviour occur when either reading or writing from/to the buffer? (Does reading from the buffer also trigger epoll?)
new syscall tests in rust that test edge-triggered epoll under these various conditions for different file types to make sure that shadow behaves the same as linux

Thanks for working on this!

Tests

We have some existing C epoll tests in src/test/epoll/test_epoll.c and Rust epoll tests in src/test/epoll/test_epoll.rs. The new tests should be written in rust, but it's up to you whether they should go in test_epoll.rs or a new file. If the tests are large, it might make sense to have these edge-triggered tests in a new file.

You can test the epoll syscalls using the libc library (ex: libc::epoll_ctl). In test_epoll.rs we used the nix library to test the syscalls, but I think that was a mistake and we should have used the libc or linux-api libraries instead. The tests are also stuck on an old version of nix see (~~#3141~~ #3265) which uses an old version of their epoll API.

For us as maintainers, we don't really know exactly how linux behaves with this edge-triggered-but-not-really-edge-triggered epoll, so the the more tests you add the easier it will be to convince us that the changes accurately follow linux and that it's good to merge.

ppopth · 2024-01-04T06:28:32Z

Does reading from the buffer also trigger epoll?

From the test I implemented in #3274, the answer is no in Linux. However, it's surprising that the answer is yes in the current main branch of Shadow.

Since I don't want to pay the effort fixing this and I don't know any application depending on that behaviour, I will not add such the test in this PR.

stevenengler · 2024-01-04T06:44:43Z

Since I don't want to pay the effort fixing this and I don't know any application depending on that behaviour, I will not add such the test in this PR.

Thanks for making that issue and writing a test case! If you want to, you can add the test to Shadow but in the ShadowTest::new("write-then-read", ...) use set![TestEnvironment::Libc] (and leave a comment referring to #3274). This indicates that the test is supposed to pass in Shadow but doesn't, and you don't need to fix it since it will be skipped when running in Shadow.

ppopth · 2024-01-04T17:35:09Z

@stevenengler What is the state of SharedBuf? Is it something you still want to use in the long term?

In order to implement edge-triggered epoll for pipe and unix sockets, it seems like I have to implement BufferSignals (just like FileSignals) in addition to BufferState for SharedBuf.

If you are planning to deprecate SharedBuf, it's probably not worth the effort. If no, I will do the BufferSignals.

ppopth · 2024-01-06T15:21:16Z

does this edge-triggered behaviour occur only on the input buffer, or also the output buffer?

I found strange behaviour on the output buffer. This edge-triggered thing occurs on the output buffer of udp sockets, but not tcp sockets or unix datagram sockets. I think it will take too much effort doing research on this, so I will not add the tests for those cases.

stevenengler · 2024-01-07T03:27:20Z

What is the state of SharedBuf? Is it something you still want to use in the long term?

I think we still want it in the long term. We need some kind of shared buffer between two files (or sometimes more if there are multiple readers or writers).

In order to implement edge-triggered epoll for pipe and unix sockets, it seems like I have to implement BufferSignals (just like FileSignals) in addition to BufferState for SharedBuf.

If you are planning to deprecate SharedBuf, it's probably not worth the effort. If no, I will do the BufferSignals.

I forgot about the SharedBuf when adding the signals. I made #3276 to add the boilerplate code needed to support signals in SharedBuf. I don't want to conflict with the code you're working on, so let me know if you've already started adding signals to SharedBuf and if you have we can use your code instead. If not, hopefully #3276 will make this PR easier.

I found strange behaviour on the output buffer. This edge-triggered thing occurs on the output buffer of udp sockets, but not tcp sockets or unix datagram sockets. I think it will take too much effort doing research on this, so I will not add the tests for those cases.

Okay thanks for checking. I think it's okay to leave the output buffer out of the scope of this PR then. But please open an issue so we can remember that this edge-triggered behaviour doesn't follow linux for all output buffers and that it needs more investigation.

ppopth · 2024-01-07T05:25:25Z

I don't want to conflict with the code you're working on, so let me know if you've already started adding signals to SharedBuf and if you have we can use your code instead

Yes, I have mine already and it looks exactly like yours in #3276 (except some in unix.rs)

ppopth · 2024-01-07T12:57:28Z

@stevenengler What is your preferred way to do write to a raw file descriptor in the tests?

Use std::fs::File::from_raw_fd just like in https://stackoverflow.com/questions/54858018/how-do-i-write-to-a-specific-raw-file-descriptor-from-rust
or, use libc::write just like https://github.com/shadow/shadow/blob/348bc4/src/test/pipe/test_pipe.rs#L185-L198

In test_epoll.rs, it uses nix::unistd::write.

stevenengler · 2024-01-07T13:54:08Z

@stevenengler What is your preferred way to do write to a raw file descriptor in the tests?

Use std::fs::File::from_raw_fd just like in https://stackoverflow.com/questions/54858018/how-do-i-write-to-a-specific-raw-file-descriptor-from-rust

or, use libc::write just like https://github.com/shadow/shadow/blob/348bc4/src/test/pipe/test_pipe.rs#L185-L198

In test_epoll.rs, it uses nix::unistd::write.

The preferred is option (2).

In the tests, if it's a syscall that you're specifically testing, then use plain libc. But you can use either libc or nix for other syscalls that you're not testing. I'd avoid using std functions in tests since it's difficult to know exactly what syscalls they're making.

So in test_epoll.rs since you're testing epoll related syscalls, use libc for the epoll syscalls (libc::epoll_wait), and you can use nix or libc for other syscalls like read or write (libc::write or nix::unistd::write).

ppopth · 2024-01-07T14:43:46Z

@stevenengler it's ready for your eyes

stevenengler

It's looking good! I still have a few more files to look through (unix.rs, the rust tcp files, and only quickly looked through test_epoll_edge.rs), but thought I'd leave the comments I have so far. They're all minor comments, and two additional tests I think would be good to have.

src/main/host/descriptor/socket/inet/udp.rs

src/main/host/descriptor/mod.rs

src/main/host/descriptor/epoll/entry.rs

src/main/host/descriptor/eventfd.rs

src/main/host/descriptor/shared_buf.rs

src/main/host/descriptor/epoll/entry.rs

@ppoth

In the old code when adding or removing data from a socket buffer, a socket.c function would update the file state (called the "status" in the C code), then a tcp.c function might also update the file state. For example `tcp_receiveUserData` would call `legacysocket_removeFromInputBuffer` which would update the state, and then `tcp_receiveUserData` would later also update the state. This would lead to situations where socket.c would remove the READABLE flag, and then tcp.c would immediately add it back. While the end result is that the flag would effectively appear unchanged, this would cause edge-triggered epoll to think that the file's state changed twice even though it effectively didn't, leading to #3274. Since the TCP code is the only C socket code left, now the socket.c code no longer updates the file state (with an exception of `legacysocket_pullOutPacket`) and the tcp.c code is responsible for updating the file state. The added test was authored by @ppoth in #3274. We should also be testing other socket/file types, but I think this will be easier once the new `test_epoll_edge.rs` tests in #3243 are merged. I did test udp and unix sockets manually and the new test passes for them as well. Closes #3274. **Edit:** The runtime performance is equivalent, but the simulation results change, so I want to take another look at this. https://github.com/shadow/benchmark-results/blob/master/tor/2024-01-07-T22-11-55/README.md **Edit 2:** I think it's expected that the simulation results would change. Before we would remove the `READABLE` flag and then add it back again, which would reset the epoll entry's priority each time a packet was pushed to a TCP socket. Now the priority is reset only when the event is collected. This means that epoll events will be processed in a different order than they previously were.

stevenengler

Looks good! I think the improved edge-triggered epoll and tests will be a big improvement. All of the requested changes should be small, and then there are two additional tests I think would be good to have. After this I'll do a last benchmark run and then I expect we can merge it.

use the new signal mechanism instead of the INPUT_BUFFER_PARITY state

Done.

use the new edge-triggered epoll behaviour in all file types (udp, pipe, etc) and not just eventfd/tcp, as long as linux does as well (the tests should show which files do or don't)

Done.

does this edge-triggered behaviour occur only on the input buffer, or also the output buffer?

@ppopth Once this is merged, can you make an issue for supporting the UDP output buffer behaviour you observed?

for the input buffer, does the edge-triggered behaviour occur when either reading or writing from/to the buffer? (Does reading from the buffer also trigger epoll?)

Done. There was a related bug #3274 where a partial read would trigger edge-triggered epoll in shadow but not Linux. This was fixed in #3277. This current PR contains a test that checks this for all sockets and pipes.

new syscall tests in rust that test edge-triggered epoll under these various conditions for different file types to make sure that shadow behaves the same as linux

Done. The new tests check tcp, udp, unix sockets (all three types), pipes (stream and packet modes), and eventfds.

src/lib/tcp/src/connection.rs

src/lib/tcp/src/lib.rs

src/main/host/descriptor/socket/inet/tcp.rs

src/main/host/descriptor/socket/unix.rs

src/test/epoll/test_epoll_edge.rs

stevenengler · 2024-01-12T23:36:59Z

Conflicting files

src/main/host/descriptor/socket.c

In #3277 I moved the legacyfile_adjustStatus((LegacyFile*)socket, STATUS_FILE_READABLE, TRUE, 0) from socket.c to tcp.c, so you'll need to move the signal code there as well to fix the conflict.

ppopth · 2024-01-17T17:51:56Z

Conflicting files

src/main/host/descriptor/socket.c

In #3277 I moved the legacyfile_adjustStatus((LegacyFile*)socket, STATUS_FILE_READABLE, TRUE, 0) from socket.c to tcp.c, so you'll need to move the signal code there as well to fix the conflict.

I resolved the conflict by merging instead of rebasing so that I would not lose the history. I will rebase it later.

ppopth · 2024-01-18T22:00:40Z

@stevenengler it's ready for your eyes

stevenengler · 2024-01-21T21:09:26Z

@stevenengler it's ready for your eyes

Looks good! Just do a rebase, then I'll approve it, run the benchmark, and merge it.

ppopth · 2024-01-24T05:34:21Z

@stevenengler it's all done. Can you please add a milestone to this PR?

stevenengler · 2024-01-25T04:56:55Z

No change to the tor benchmark. I don't think tor uses edge-triggered epoll, so this should be expected.

stevenengler · 2024-01-25T05:00:50Z

Thanks @ppopth!

ppopth · 2024-02-07T19:39:13Z

@ppopth Once this is merged, can you make an issue for supporting the UDP output buffer behaviour you observed?

The issue is created at #3295

github-actions bot assigned ppopth Dec 5, 2023

github-actions bot added the Component: Main Composing the core Shadow executable label Dec 5, 2023

ppopth mentioned this pull request Dec 5, 2023

Extra test for rust-libp2p ping protocol #3242

Open

stevenengler reviewed Dec 5, 2023

View reviewed changes

src/main/host/descriptor/epoll/entry.rs Outdated Show resolved Hide resolved

src/main/host/descriptor/mod.rs Outdated Show resolved Hide resolved

src/main/host/descriptor/eventfd.rs Outdated Show resolved Hide resolved

stevenengler reviewed Dec 5, 2023

View reviewed changes

src/main/host/descriptor/socket.c Outdated Show resolved Hide resolved

stevenengler reviewed Dec 5, 2023

View reviewed changes

src/main/host/descriptor/socket.c Outdated Show resolved Hide resolved

stevenengler mentioned this pull request Jan 7, 2024

Support listening for SharedBuf signal events #3276

Closed

ppopth force-pushed the edge-trigger2 branch from 4e69d1d to 78900fb Compare January 7, 2024 14:17

github-actions bot added Component: Libraries Support functions like LD_PRELOAD and logging Component: Testing Unit and integration tests and frameworks Component: Build Build/install tools and dependencies labels Jan 7, 2024

ppopth marked this pull request as ready for review January 7, 2024 14:20

ppopth requested a review from stevenengler January 7, 2024 14:21

stevenengler mentioned this pull request Jan 7, 2024

Don't update the file state in socket.c #3277

Merged

stevenengler reviewed Jan 11, 2024

View reviewed changes

stevenengler self-requested a review January 11, 2024 01:53

stevenengler requested changes Jan 12, 2024

View reviewed changes

ppopth force-pushed the edge-trigger2 branch from 30555a2 to 237f6a8 Compare January 18, 2024 14:18

ppopth requested a review from stevenengler January 18, 2024 21:40

ppopth force-pushed the edge-trigger2 branch from 8613fd5 to 7d0b445 Compare January 24, 2024 05:32

Edge-triggered epoll event

44e2a67

ppopth force-pushed the edge-trigger2 branch from 7d0b445 to 44e2a67 Compare January 24, 2024 05:33

stevenengler added this to the Validate and improve TCP simulation accuracy milestone Jan 24, 2024

stevenengler approved these changes Jan 24, 2024

View reviewed changes

stevenengler merged commit d957f4c into shadow:main Jan 25, 2024
23 of 24 checks passed

stevenengler mentioned this pull request Jan 25, 2024

Eventfd with epoll EPOLLET event doesn't match Linux #2673

Closed

ppopth mentioned this pull request Feb 7, 2024

UDP has a weird edge-triggered EPOLLOUT behavior in Linux #3295

Open

ppopth deleted the edge-trigger2 branch February 22, 2024 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edge-triggered epoll events #3243

Edge-triggered epoll events #3243

ppopth commented Dec 5, 2023 •

edited

Loading

stevenengler commented Dec 5, 2023

robgjansen commented Dec 7, 2023

stevenengler commented Dec 8, 2023

ppopth commented Dec 10, 2023

ppopth commented Dec 11, 2023

stevenengler commented Dec 13, 2023 •

edited

Loading

robgjansen commented Dec 13, 2023

stevenengler commented Dec 19, 2023 •

edited

Loading

ppopth commented Jan 4, 2024 •

edited

Loading

stevenengler commented Jan 4, 2024

ppopth commented Jan 4, 2024

ppopth commented Jan 6, 2024

stevenengler commented Jan 7, 2024 •

edited

Loading

ppopth commented Jan 7, 2024 •

edited

Loading

ppopth commented Jan 7, 2024 •

edited

Loading

stevenengler commented Jan 7, 2024

ppopth commented Jan 7, 2024

stevenengler left a comment

stevenengler left a comment •

edited

Loading

stevenengler commented Jan 12, 2024 •

edited

Loading

Conflicting files

ppopth commented Jan 17, 2024

Conflicting files

ppopth commented Jan 18, 2024

stevenengler commented Jan 21, 2024

ppopth commented Jan 24, 2024

stevenengler commented Jan 25, 2024

stevenengler commented Jan 25, 2024

ppopth commented Feb 7, 2024

Edge-triggered epoll events #3243

Edge-triggered epoll events #3243

Conversation

ppopth commented Dec 5, 2023 • edited Loading

stevenengler commented Dec 5, 2023

robgjansen commented Dec 7, 2023

stevenengler commented Dec 8, 2023

ppopth commented Dec 10, 2023

ppopth commented Dec 11, 2023

stevenengler commented Dec 13, 2023 • edited Loading

robgjansen commented Dec 13, 2023

stevenengler commented Dec 19, 2023 • edited Loading

Tests

ppopth commented Jan 4, 2024 • edited Loading

stevenengler commented Jan 4, 2024

ppopth commented Jan 4, 2024

ppopth commented Jan 6, 2024

stevenengler commented Jan 7, 2024 • edited Loading

ppopth commented Jan 7, 2024 • edited Loading

ppopth commented Jan 7, 2024 • edited Loading

stevenengler commented Jan 7, 2024

ppopth commented Jan 7, 2024

stevenengler left a comment

Choose a reason for hiding this comment

stevenengler left a comment • edited Loading

Choose a reason for hiding this comment

stevenengler commented Jan 12, 2024 • edited Loading

Conflicting files

ppopth commented Jan 17, 2024

Conflicting files

ppopth commented Jan 18, 2024

stevenengler commented Jan 21, 2024

ppopth commented Jan 24, 2024

stevenengler commented Jan 25, 2024

stevenengler commented Jan 25, 2024

ppopth commented Feb 7, 2024

ppopth commented Dec 5, 2023 •

edited

Loading

stevenengler commented Dec 13, 2023 •

edited

Loading

stevenengler commented Dec 19, 2023 •

edited

Loading

ppopth commented Jan 4, 2024 •

edited

Loading

stevenengler commented Jan 7, 2024 •

edited

Loading

ppopth commented Jan 7, 2024 •

edited

Loading

ppopth commented Jan 7, 2024 •

edited

Loading

stevenengler left a comment •

edited

Loading

stevenengler commented Jan 12, 2024 •

edited

Loading