Fix over synchronization of epoll

[FrankReh]

Fixes #3944.

The clock used by epoll is now per event generated, rather than by the epoll's ready_list.

The same epoll tests that existed before are unchanged and still pass. Also the tokio test case we had worked on last week still passes with this change.

This change does beg the question of how the epoll event states should change. Perhaps rather than expose public crate bool fields, so setters should be provided that include a clock parameter or an optional clock parameter. Also should all the epoll event possibilities have their clock sync tested the way these commit lay out testing. In this first go around, only the pipe's EPOLLIN is tested. The EPOLLOUT might deserve testing too, as would the eventfd. Any future source of epoll events would also fit into that category.

[FrankReh]

This left as a draft for at least two reasons. There is an open question why the fix doesn't work for the test case as it was outlined in the issue. That open question is explained in the test cases.

Then there are some field names that can be simplified, to conform to existing names, but while working on this file, I prefer having field names that are unique so it is easy to move around the file based on where those fields are used.

I guess a third reason. Before final review, the open test question will have been resolved and the tests will likely be reduced.

[FrankReh]

I wonder if the problem I’m seeing is cause by my assumption that the source thread should have its clock incremented just once. The one time is to be part of the happens-before relationship between this thread and any thread receiving the event. But to create a happens after clock, the clock could simply be incremented one more time as part of the cleanup to creating one or more epoll events. Then my tests would pass as expected and the question I had is solved.

[FrankReh]

I have found a way to fix the problem I was seeing. It involves incrementing the clock twice when the pipe's buffer is being written to. Once to prepare the clock for the happens-before relationship. And once again to represent a happens-after relationship for writes by the same thread that happen after this write. If the write to the global static also updated the clock on its own, this probably wouldn't be necessary.

This begs new questions.

Should the pipe's read side also be epoll-able? Right now there is no such event created.

And does the eventfd, which also supports epoll, suffer from this problem of syncing clocks through epoll but then not advancing the clock, thereby keeping UB with static global writes afterwards from being detected?

[RalfJung]

Should the pipe's read side also be epoll-able? Right now there is no such event created.

Both ends are already epoll-able, I think.

[RalfJung]

Let's fix #3947 (in a different PR) before attempting this.

[bors]

☔ The latest upstream changes (presumably #3951) made this pull request unmergeable. Please resolve the merge conflicts.

[FrankReh]

The clock field could also be an Option. Maybe less work when clocks aren't enabled.

[RalfJung]

Default clocks are trivial, so no that shouldn't be necessary, and I don't think it helps the code either.

[RalfJung]

Not sure what the status here is, so
@rustbot author

[FrankReh]

Not sure what the status here is, so
@rustbot author

I'm not familiar with this step. Am I the author being asked for the status or is this for a maintainer?
It's marked ready for review. It has a test for before and a test for after showing the improvement. Is there something else for me to do?

[RalfJung]

Sorry, I should have explained - If it is ready for review, you are expected to do @rustbot ready

[RalfJung]

@rustbot author

[FrankReh]

@rustbot ready

[RalfJung]

Looks great, thanks!

@bors r+

[bors]

📌 Commit 4e4af8d has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 4e4af8d with merge e0bd116...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing e0bd116 to master...