search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.76k stars 380 forks source link

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

Closed conectado closed 6 months ago

conectado commented 7 months ago

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.

Ralith commented 6 months ago

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

thomaseizinger commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago
  • 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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

thomaseizinger commented 6 months ago

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 commented 6 months ago

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!

conectado commented 6 months ago

@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.