Skip to content

Switch epoll and kqueue to level triggers#2655

Merged
thhous-msft merged 5 commits intomainfrom
thadhouse/epollkqueueleveltriggers
May 7, 2022
Merged

Switch epoll and kqueue to level triggers#2655
thhous-msft merged 5 commits intomainfrom
thadhouse/epollkqueueleveltriggers

Conversation

@thhous-msft
Copy link
Copy Markdown
Contributor

Description

kqueue and epoll datapaths have a race condition upon pended sends whereas they might not get trigger notifications. By switching to level triggers, we solve this issue.

Closes #2654

Testing

Existing tests cover the change.

Documentation

N/A

@thhous-msft thhous-msft requested a review from a team as a code owner April 22, 2022 23:48
@thhous-msft
Copy link
Copy Markdown
Contributor Author

Don't merge this yet, I want to do more local testing. Its just easy for CI to test everything as well.

anrossi
anrossi previously approved these changes Apr 25, 2022
@nibanks
Copy link
Copy Markdown
Collaborator

nibanks commented Apr 26, 2022

I still have my reservations about making changes here, because we've never seen an issue with the current code. Is there any test we can write (maybe at the datapath unit test layer) to prove/disprove this change?

@thhous-msft
Copy link
Copy Markdown
Contributor Author

I did a lot more research, and some independent testing, and at least on epoll, as long as pending doesn't happen with a basically empty kernel buffer, the event will still be triggered. Its a test that was only reproducable using TCP (It was the only way I could actually force EAGAIN or EWOULDBLOCK to occur), so adding it to the tests isn't really feasable.

However, if we ever want to implement PSN, It does require this change. PSN is well defined in its behavior here, and different from epoll and kqueue. Also the code is written as if level shifting was the mode we were using.

@nibanks
Copy link
Copy Markdown
Collaborator

nibanks commented Apr 26, 2022

How do we validate this with testing though?

@thhous-msft
Copy link
Copy Markdown
Contributor Author

Well we validate that it works, and doesn't break performance. If it was going to be problematic, it would have triggered an issue with those cases. And using a TCP test, validated that the logic holds.

The issue is its basically impossible to trigger EAGAIN on write for UDP. I've tried looking if theres a way to force it, but I can't find any.

nibanks
nibanks previously approved these changes Apr 27, 2022
@nibanks
Copy link
Copy Markdown
Collaborator

nibanks commented Apr 27, 2022

Ok. I leave you in charge of the final decision!

@thhous-msft thhous-msft dismissed stale reviews from nibanks and anrossi via cca5f1b April 27, 2022 20:56
@thhous-msft
Copy link
Copy Markdown
Contributor Author

The discussion created in #2662 is another reason for doing this change. Level triggered mode is required to make it so the entire receive buffer does not have to be read every time an event is received.

nibanks
nibanks previously approved these changes Apr 27, 2022
anrossi
anrossi previously approved these changes Apr 27, 2022
@thhous-msft thhous-msft dismissed stale reviews from anrossi and nibanks via e376e8a April 28, 2022 16:02
@thhous-msft thhous-msft enabled auto-merge (squash) April 29, 2022 16:56
@thhous-msft thhous-msft merged commit 9f22efb into main May 7, 2022
@thhous-msft thhous-msft deleted the thadhouse/epollkqueueleveltriggers branch May 7, 2022 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Poll based datapaths can hang sends in race condition with kernel

3 participants