Skip to content
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.

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

refactor(s2n-quic-core): simplify IO traits #1734

Merged
merged 1 commit into from
May 2, 2023
Merged

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Apr 28, 2023

Description of changes:

While implementing the AF_XDP IO provider, I wanted to make a generic event loop implementation so I didn't have to copy/paste the one from the tokio provider.

In this change, the core IO traits are refactored to make it possible to use in the generic IO provider event loop.

Call-outs:

The previous versions of the IO traits assumed that all of the incoming and outgoing packets were addressable through a slice. This was done to integrate with something like rayon to perform packet crypto in parallel. The problem is that assumption isn't really the case with AF_XDP, where we're putting descriptors in a ring buffer and they're not dereferenceable without the UMEM. I also think there's other ways to perform packet encryption in parallel that don't force the IO provider to adhere to this strict interface, so it's best to keep it simple and put the complexity elsewhere (especially since it's not even being used right now.)

I've also opened #1742 to highlight a better interface using GATs, which are only available in rust >=1.65. We can change the API once the MSRV bumps.

Testing:

Since this is just shuffling things around and simplifying traits, the existing tests should cover the changes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft camshaft force-pushed the camshaft/io-refactor branch 3 times, most recently from 8eca936 to a146e18 Compare April 28, 2023 23:50
@camshaft camshaft marked this pull request as ready for review May 1, 2023 15:56
@camshaft camshaft marked this pull request as draft May 1, 2023 17:57
@camshaft
Copy link
Contributor Author

camshaft commented May 1, 2023

Running into an integration issue with needing generic associated lifetimes... Going to see if I can rework it to avoid it.

@camshaft
Copy link
Contributor Author

camshaft commented May 1, 2023

This gist outlines the issue and the potential work-around. Going to investigate if this is safe or not.

@camshaft
Copy link
Contributor Author

camshaft commented May 2, 2023

I think we're OK to use the method as outlined above. I will include a comment and open an issue.

@camshaft camshaft marked this pull request as ready for review May 2, 2023 03:15
@camshaft camshaft merged commit 73a8687 into main May 2, 2023
@camshaft camshaft deleted the camshaft/io-refactor branch May 2, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants