epoll: iterate through output buffer then fetch an event from ready list #3818
Conversation
src/shims/unix/linux/epoll.rs
Outdated
| if let Some(des) = array_iter.next(this)? { | ||
| while let Some(des) = array_iter.next(this)? { | ||
| // Fetch an event from the ready list. | ||
| if let Some((epoll_key, epoll_return)) = ready_list.pop_first() { |
There was a problem hiding this comment.
This will cause empty entries whenever a FD was already closed. You'll need to use a for loop here and exit it only if you actually write an entry.
This will cause us to finish iterating over the array_iter and never doing anything once the ready list is empty. That's fine, but we could also ensure we continue to the outer loop (by giving it a label) and unconditionally breaking after the ready list loop
There was a problem hiding this comment.
(fwiw: this is why we abandoned this approach before, but forgot that in abandoning it we'd not fix this issue)
There was a problem hiding this comment.
Indeed, this needs to be a loop getting the next entry from the ready list that refers to an FD that is still open.
(I assume there's a test covering this case? That test would generate an interesting event on an FD, and then close the FD, and only then call epoll_wait and expect to not see that event.)
There was a problem hiding this comment.
Right, the test that I added initially failed because there is empty entry where it shouldn't. I added a condition to check if the entry is actually written, also terminate earlier if the ready list is empty (although this won't affect correctness).
src/shims/unix/linux/epoll.rs
Outdated
| while let Some(des) = array_iter.next(this)? | ||
| && !ready_list.is_empty() | ||
| { | ||
| let mut entry_written = false; |
There was a problem hiding this comment.
I don't think using mutable state is necessary here. That makes reasoning about the code harder. The !is_empty check above also adds extra complexity that should not be needed.
Instead, I suggest you add a function like ready_list_next that returns the next item from the ready list that someone is still interested in:
while let Some((epoll_key, epoll_return)) = ready_list.pop_first() {
// Ensure we are still interested in this event. The FD might have been closed since
// the event was generated, in which case we are not interested any more.
if this.machine.epoll_interests.get_epoll_interest(epoll_key.0).is_some() {
return Some((epoll_key, epoll_return));
}
}
return None;Then you just use that function here, using a simple for loop.
There was a problem hiding this comment.
Updated, thanks for the detailed suggestion. By the way, if the for loop you meant is for while let Some(des) = array_iter.next(this)? , it might not be possible because ArrayIterator, which is the type of array_iter, doesn't implement IntoIterator.
There was a problem hiding this comment.
Ah right this has to remain while let, good point.
|
@rustbot ready |
src/shims/unix/linux/epoll.rs
Outdated
| } | ||
| } | ||
| Ok(Scalar::from_i32(num_of_events)) | ||
| } | ||
|
|
||
| /// This function takes in ready list and returns EpollEventInstance with file description | ||
| /// that is not closed. | ||
| fn ready_list_next( |
There was a problem hiding this comment.
You are adding this as a public function that can be called everywhere in Miri -- that's not the intention, as this is just a private helper. Please make the function private.
There was a problem hiding this comment.
I moved this out and turned this a free function. Alternatively, we can create a wrapper type for ready_list and make this a method of that type.
There was a problem hiding this comment.
A free function sounds good for this!
|
☔ The latest upstream changes (presumably #3813) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Looks good, thanks. :) Please squash the commits. |
RalfJung
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #3824) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Okay :) |
|
☀️ Test successful - checks-actions |
Fixes #3812