Add Flush to manually unblock Read/ReadInto#1491
Conversation
brycekahle
force-pushed
the
bryce.kahle/perf-flush-end
branch
2 times, most recently
from
88a6afe to
9782078
Compare
florianl
left a comment
florianl
left a comment
There was a problem hiding this comment.
Thanks for the update! 🙏
| p.flushEvent.read() | ||
| events = slices.Delete(events, i, i+1) | ||
| n -= 1 | ||
| continue |
There was a problem hiding this comment.
So the behaviour is that Wait will return 0, nil for a flush. But what about a case where another ring becomes ready simultaneously with flushEvent? In that case we'd get two events from epoll, would discard one and then return 1, nil. In that case we drop an ErrFlushed from Reader.Read which the user is expecting.
How about removing the event here, and doing return n, ErrFlushed from Wait instead? Then ringbuf and perf re-export epoll.ErrFlushed and the rest can mostly stay the same.
There was a problem hiding this comment.
In that case we drop an ErrFlushed from Reader.Read which the user is expecting.
I don't follow. Flush is the one that sets up a pending error of ErrFlushed, which will still be returned after all the rings have been read.
The n return value from Wait isn't even used. Only if an error is returned does a different code path happen. We want Flush to behave as if a regular ring epoll event happened, which is what it is doing.
There was a problem hiding this comment.
I think we actually want to ensure we read all the events in case there is both a Flush and Close in the same wakeup.
brycekahle
force-pushed
the
bryce.kahle/perf-flush-end
branch
from
e5f3960 to
6255d47
Compare
|
The test failures on Go 1.21 do not seem to be related to my PR |
yeah i think it's due to sometimes getting a slow GH runner or something along those lines. are you able to re-run failed runs? if not we might need to fiddle with permissions. |
It didn't seem like I could. |
brycekahle
force-pushed
the
bryce.kahle/perf-flush-end
branch
from
201d1d0 to
e5d823f
Compare
|
@brycekahle both Flush implementations were racing with Read. I've pushed a commit which fixes this, by returning ErrFlushed from the poller as I mentioned. PTAL. |
|
Looks good |
Add a method Flush which interrupts a perf or ringbuf reader and causes it to read all data from the ring. This is very similar to the logic we use to check for data in the ring when a deadline is expired. The semantics of the Read function change slightly: a caller is now guaranteed to receive an os.ErrDeadlineExceeded which wasn't the case when the ring contained data. Signed-off-by: Bryce Kahle <[email protected]> Co-authored-by: Lorenz Bauer <[email protected]>
Reader has two separate locks which guard separate data. Re-organise the fields so that it's clearer which lock protects which fields. Signed-off-by: Lorenz Bauer <[email protected]>
lmb
force-pushed
the
bryce.kahle/perf-flush-end
branch
from
4f6ad84 to
c052c45
Compare
3 participants
When using
WakeupEventsfor perf buffers orBPF_RB_NO_WAKEUPfor ring buffers, you can enqueue data that does not immediately trigger a reader blocked inRead/ReadIntoto wakeup.This PR adds a function
Flushto manually cause those blocked readers to wakeup. Further, a sentinel errorFlushCompleteErroris returned when all the rings have been read, indicating all the data already queued whenFlushwas called has been read.