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

Enable packet info and related flags for quinn-udp on Android #1758

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

conectado
Copy link
Contributor

We're using the info obtained with PKT_INFO, particularly the destination IP, on multiple platforms including on android.

Most of the socket options used for linux, save for GRO, are available on android, so it's simply adding it to the conditional compilation.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks! A few notes:

  • Do we need to add android to the case on line 49 too?
  • Are GSO/GRO ever available on Android?
  • Would love to have CI coverage for this, if you're up for it.

@Ralith
Copy link
Collaborator

Ralith commented Mar 10, 2024

@conectado do you plan to do any follow-up work here?

@thomaseizinger
Copy link
Contributor

@Ralith Unfortunately, your force-push just broke our CI because we are depending on this commit hash. If you'd like to update the PR, we'd prefer to do it via merge commits only.

@thomaseizinger
Copy link
Contributor

We do plan on following up here but we are preparing the product launch at the moment so there hasn't yet been time for this! :)

@thomaseizinger
Copy link
Contributor

@Ralith Unfortunately, your force-push just broke our CI because we are depending on this commit hash. If you'd like to update the PR, we'd prefer to do it via merge commits only.

I'll make a dedicated fork for now to un-break CI.

@thomaseizinger
Copy link
Contributor

  • Are GSO/GRO ever available on Android?

We haven't tested this specifically but at least the destination IP is available with this PR.

  • Would love to have CI coverage for this, if you're up for it.

How do you envision to test this in CI? I guess we'd have to use something like QEMU to emulate common Android CPU architectures and then run the tests on it?

@Ralith
Copy link
Collaborator

Ralith commented Mar 11, 2024

We haven't tested this specifically but at least the destination IP is available with this PR.

GSO/GRO are unrelated to destination IPs, but are a big CPU efficiency win. They're well-established on Linux so I'd be surprised if they're absent from Android. Even on old Android, the existing graceful degradation mechanisms should be able to handle their absence at runtime. Not a blocker for this PR, but maybe something you'd want to double-check if you're targeting that platform.

How do you envision to test this in CI? I guess we'd have to use something like QEMU to emulate common Android CPU architectures and then run the tests on it?

I'm not familiar with Android workflows, so I'm not sure. Some sort of emulator makes sense. Maybe crib from e.g. https://github.com/rustls/rustls-platform-verifier/blob/main/.github/workflows/ci.yml? CI coverage is particularly important here as we're not Android experts ourselves.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Mar 11, 2024

We haven't tested this specifically but at least the destination IP is available with this PR.

GSO/GRO are unrelated to destination IPs, but are a big CPU efficiency win. They're well-established on Linux so I'd be surprised if they're absent from Android. Even on old Android, the existing graceful degradation mechanisms should be able to handle their absence at runtime. Not a blocker for this PR, but maybe something you'd want to double-check if you're targeting that platform.

Yeah, I am aware of the efficiency win and we are already leverage GRO but not yet GSO (only verified on Linux desktops). For us, having access to the destination IP is critical whereas GRO and GSO on Android would just be a performance bonus that can be looked into later as well.

How do you envision to test this in CI? I guess we'd have to use something like QEMU to emulate common Android CPU architectures and then run the tests on it?

I'm not familiar with Android workflows, so I'm not sure. Some sort of emulator makes sense. Maybe crib from e.g. rustls/rustls-platform-verifier@main/.github/workflows/ci.yml? CI coverage is particularly important here as we're not Android experts ourselves.

I am also not familiar with testing native code for Android in CI. Could we defer that as a follow-up and open an issue about it given that it is also not present on main and no claims are being made that this works on Android?

@Ralith
Copy link
Collaborator

Ralith commented Mar 11, 2024

Sure. The only blocking concern is updating the size check on line 49.

@thomaseizinger
Copy link
Contributor

Sure. The only blocking concern is updating the size check on line 49.

Awesome, thank you! I'll make those changes and open an issue about adding Android CI. Eventually, we want a decent Android CI for the project so once we have to acquire that knowledge, I might be able to port some of it to quinn.

@thomaseizinger
Copy link
Contributor

Sure. The only blocking concern is updating the size check on line 49.

Awesome, thank you! I'll make those changes and open an issue about adding Android CI. Eventually, we want a decent Android CI for the project so once we have to acquire that knowledge, I might be able to port some of it to quinn.

I opened an issue and linked it on the rust-lang forum to be included in next week's TWIR. Maybe somebody volunteers :)

I'll nudge @conectado to push a fix for the size check!

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

@conectado
Copy link
Contributor Author

@thomaseizinger thanks for taking over! :)

@Ralith Pushed the fix.

Also, it will probably just work enabling GRO/GSO in android since the socket options exists in the kernel I can try it on a follow up PR when I have a bit more availability.

@Ralith Ralith enabled auto-merge (squash) March 11, 2024 19:51
@Ralith Ralith merged commit ffac4a3 into quinn-rs:main Mar 11, 2024
8 checks passed
github-merge-queue bot pushed a commit to firezone/firezone that referenced this pull request Mar 12, 2024
quinn-rs/quinn#1758 was merged, meaning we don't need to maintain our
fork

Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
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.

4 participants