Skip to content

event: update fd registration mask even if it hasn't changed. #16389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wrowe merged 7 commits into from
May 24, 2021
Merged

event: update fd registration mask even if it hasn't changed. #16389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b107f02
event: update fd registration mask even if it hasn't changed.
antoniovicente May 8, 2021
6a856ef
Remove call to dispatcher exit that is not needed
antoniovicente May 8, 2021
955b037
lint
antoniovicente May 8, 2021
ea266f1
add comment and improve implementation
antoniovicente May 20, 2021
19c59da
Merge remote-tracking branch 'upstream/main' into lost_events
antoniovicente May 20, 2021
7c53822
improve coverage
antoniovicente May 21, 2021
c2d02fb
fix spelling. activations is not a word.
antoniovicente May 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion source/common/event/file_event_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,16 @@ void FileEventImpl::assignEvents(uint32_t events, event_base* base) {

void FileEventImpl::updateEvents(uint32_t events) {
ASSERT(dispatcher_.isThreadSafe());
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like either a comment here on why this is important, or a comment in the test calling out that it's regression testing [info from PR description] just to ensure no clever person decides to improve perf by undoing this PR :-P

Copy link
Copy Markdown
Contributor Author

@antoniovicente antoniovicente May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added. Thanks for asking for further info in the code, there's a lot of subtle behavior that can be accidentally missed.

if (events == enabled_events_) {
// The update can be skipped in cases where the old and new event mask are the same if the fd is
// using Level or EmulatedEdge trigger modes, but not Edge trigger mode. When the fd is registered
// in edge trigger mode, re-registering the fd will force re-computation of the readable/writable
// state even in cases where the event mask is not changing. See
// https://github.com/envoyproxy/envoy/pull/16389 for more details.
// TODO(antoniovicente) Consider ways to optimize away event registration updates in edge trigger
// mode once setEnabled stops clearing injected_activation_events_ before calling updateEvents
// and/or implement optimizations at the Network::ConnectionImpl level to reduce the number of
// calls to setEnabled.
if (events == enabled_events_ && trigger_ != FileTriggerType::Edge) {
return;
}
auto* base = event_get_base(&raw_event_);
Expand Down
161 changes: 154 additions & 7 deletions test/common/event/file_event_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ class FileEventImplTest : public testing::Test {
ASSERT_EQ(sizeof(data), static_cast<size_t>(result.rc_));
}

void clearReadable() {
// Read the data from the socket so it is no longer readable.
char buffer[10];
struct iovec vec {
buffer, sizeof(buffer)
};
const Api::SysCallSizeResult result = os_sys_calls_.readv(fds_[0], &vec, 1);
EXPECT_LT(0, static_cast<size_t>(result.rc_));
EXPECT_GT(sizeof(buffer), static_cast<size_t>(result.rc_));
}

void TearDown() override {
os_sys_calls_.close(fds_[0]);
os_sys_calls_.close(fds_[1]);
Expand Down Expand Up @@ -253,18 +264,17 @@ TEST_F(FileEventImplTest, EdgeTrigger) {
#endif

TEST_F(FileEventImplTest, LevelTrigger) {
testing::InSequence s;
ReadyWatcher read_event;
EXPECT_CALL(read_event, ready()).Times(2);
ReadyWatcher write_event;
EXPECT_CALL(write_event, ready()).Times(2);

int count = 2;
int count = 0;
Event::FileEventPtr file_event = dispatcher_->createFileEvent(
fds_[0],
[&](uint32_t events) -> void {
if (count-- == 0) {
ASSERT(count > 0);
if (--count == 0) {
dispatcher_->exit();
return;
}
if (events & FileReadyType::Read) {
read_event.ready();
Expand All @@ -276,14 +286,43 @@ TEST_F(FileEventImplTest, LevelTrigger) {
},
FileTriggerType::Level, FileReadyType::Read | FileReadyType::Write);

// Expect events to be delivered twice since count=2 and level events are delivered on each
// iteration until the fd state changes.
EXPECT_CALL(read_event, ready());
EXPECT_CALL(write_event, ready());
EXPECT_CALL(read_event, ready());
EXPECT_CALL(write_event, ready());
count = 2;
dispatcher_->run(Event::Dispatcher::RunType::Block);

// Change the event mask to just Write and verify that only that event is delivered.
EXPECT_CALL(read_event, ready()).Times(0);
EXPECT_CALL(write_event, ready());
file_event->setEnabled(FileReadyType::Write);
count = 1;
dispatcher_->run(Event::Dispatcher::RunType::Block);

// Activate read, and verify it is delivered despite not being part of the enabled event mask.
EXPECT_CALL(read_event, ready());
EXPECT_CALL(write_event, ready());
file_event->activate(FileReadyType::Read);
count = 1;
dispatcher_->run(Event::Dispatcher::RunType::Block);

// Activate read and then call setEnabled. Verify that the read event is not delivered; setEnabled
// clears events from explicit calls to activate.
EXPECT_CALL(read_event, ready()).Times(0);
EXPECT_CALL(write_event, ready());
file_event->activate(FileReadyType::Read);
file_event->setEnabled(FileReadyType::Write);
count = 1;
dispatcher_->run(Event::Dispatcher::RunType::Block);
}

TEST_F(FileEventImplTest, SetEnabled) {
testing::InSequence s;
ReadyWatcher read_event;
EXPECT_CALL(read_event, ready()).Times(2);
ReadyWatcher write_event;
EXPECT_CALL(write_event, ready()).Times(2);

const FileTriggerType trigger = Event::PlatformDefaultTriggerType;

Expand All @@ -300,17 +339,125 @@ TEST_F(FileEventImplTest, SetEnabled) {
},
trigger, FileReadyType::Read | FileReadyType::Write);

EXPECT_CALL(read_event, ready());
file_event->setEnabled(FileReadyType::Read);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(write_event, ready());
file_event->setEnabled(FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

file_event->setEnabled(0);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(read_event, ready());
EXPECT_CALL(write_event, ready());
file_event->setEnabled(FileReadyType::Read | FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

// Run a manual event to ensure that previous expectations are satisfied before moving on.
ReadyWatcher manual_event;
EXPECT_CALL(manual_event, ready());
manual_event.ready();

clearReadable();

file_event->setEnabled(FileReadyType::Read);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(write_event, ready());
file_event->setEnabled(FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(write_event, ready());
file_event->setEnabled(FileReadyType::Read | FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

// Repeat the previous registration, verify that write event is delivered again.
EXPECT_CALL(write_event, ready());
file_event->setEnabled(FileReadyType::Read | FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

// Synthetic read events are delivered even if the active registration doesn't contain them.
EXPECT_CALL(read_event, ready());
file_event->activate(FileReadyType::Read);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

// Run a manual event to ensure that previous expectations are satisfied before moving on.
EXPECT_CALL(manual_event, ready());
manual_event.ready();

// Do a read activation followed setEnabled to verify that the activation is cleared.
EXPECT_CALL(write_event, ready());
file_event->activate(FileReadyType::Read);
file_event->setEnabled(FileReadyType::Read | FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

// Repeat the previous steps but with the same input to setEnabled to verify that the activation
// is cleared even in cases where the setEnable mask hasn't changed.
EXPECT_CALL(write_event, ready());
file_event->activate(FileReadyType::Read);
file_event->setEnabled(FileReadyType::Read | FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
}

TEST_F(FileEventImplTest, RegisterIfEmulatedEdge) {
// Test only applies if using EmulatedEdge trigger mode
if constexpr (PlatformDefaultTriggerType != FileTriggerType::EmulatedEdge) {
return;
}

testing::InSequence s;
ReadyWatcher read_event;
ReadyWatcher write_event;

const FileTriggerType trigger = Event::PlatformDefaultTriggerType;

Event::FileEventPtr file_event = dispatcher_->createFileEvent(
fds_[0],
[&](uint32_t events) -> void {
if (events & FileReadyType::Read) {
read_event.ready();
}

if (events & FileReadyType::Write) {
write_event.ready();
}
},
trigger, FileReadyType::Read | FileReadyType::Write);

EXPECT_CALL(read_event, ready()).Times(0);
EXPECT_CALL(write_event, ready()).Times(0);
file_event->unregisterEventIfEmulatedEdge(FileReadyType::Read | FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(read_event, ready());
file_event->registerEventIfEmulatedEdge(FileReadyType::Read);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(write_event, ready());
file_event->registerEventIfEmulatedEdge(FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(read_event, ready());
file_event->registerEventIfEmulatedEdge(FileReadyType::Read | FileReadyType::Write);
file_event->unregisterEventIfEmulatedEdge(FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(read_event, ready()).Times(0);
EXPECT_CALL(write_event, ready()).Times(0);
file_event->unregisterEventIfEmulatedEdge(FileReadyType::Read);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

EXPECT_CALL(read_event, ready());
EXPECT_CALL(write_event, ready());
file_event->registerEventIfEmulatedEdge(FileReadyType::Read | FileReadyType::Write);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);

// Events are delivered once due to auto unregistration after they are delivered.
EXPECT_CALL(read_event, ready()).Times(0);
EXPECT_CALL(write_event, ready()).Times(0);
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
}

} // namespace
Expand Down