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

Improve AEAD performance through use of the fusion AES-GCM engine #359

Merged
merged 15 commits into from
Jun 19, 2020

Conversation

kazuho
Copy link
Member

@kazuho kazuho commented Jun 17, 2020

Pioctls now has an AES-GCM engine optimized for QUIC: h2o/picotls#310.

This pull request does the following:

  • use fusion in the cli command, when and only when possible add option to specify fusion in the cli command
  • for QUIC token protection, change how the internal API of picotls is being abused
  • let both AEAD encryption and header protection be the responsibility of quicly_crypto_engine_t. This allows users to use a fused crypto that does both AEAD protection and header protection, without creating a fake AEAD context that does nothing.

Benchmark:
CPU consumption when congesting 5gbe (linux 5 6 5; Core i5-9400@4GHz, X550 NIC)
The numbers that do not state "fusion" use openssl 1.1.1g as the crypto backend. TCP does 64KB GSO. For comparison, quicly numbers doing 40-packet GSO with mtu1500, or doing 7-packet GSO with mtu9000 should be used.

When congesting a 5gbe link with mtu9000, quicly with openssl performs slightly better than picotls (TLS-over-TCP, using openssl). However, quicly (gso40,mtu1500) burns 28% additional CPU cycles compared to picotls (mtu1500). By changing the crypto to fusion, the overhead decreases to 5% with quicly (gso40,mtu1500,fusion).

See https://github.com/h2o/quicly/wiki/Benchmarking-CPU-usage for how the benchmark numbers were collected (note: now that this PR has been merged, all the numbers that you would see when using up-to-date quicly/cli command built on an x86-64 system would use the fusion crypto).

Closes #334.

@kazuho
Copy link
Member Author

kazuho commented Jun 17, 2020

@AloysAugustin @MathiasRaoul Pinging, as we plan to change the crypto engine API in this pull request.

Previously, quicly was sealing the packets in two step: 1) call ptls_aead_encrypt passing the AEAD context and the packet number, then 2) invoke the finalize_send_packet callback of the crypto engine, passing the AEAD context and the header protection context.

This PR merges the them into a single step. Now, the callback of the crypto engine is renamed as encrypt_packet, and is also (always) responsible for doing the AEAD encryption. The new callback receives the packet number too.

I think that this is a simplification for people doing crypto-offload. There is no longer a need to record the packet number in the fake AEAD callback. I kind of feel guilty for not adopting this type of design from the beginning...

Copy link
Collaborator

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

A couple of tiny suggestions, but this is exciting!

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
include/quicly.h Outdated Show resolved Hide resolved
include/quicly.h Outdated Show resolved Hide resolved
@janaiyengar
Copy link
Collaborator

You may want to note that this PR addresses #334.

kazuho and others added 2 commits June 18, 2020 17:27
Co-authored-by: Jana Iyengar <jiyengar@fastly.com>
Co-authored-by: Jana Iyengar <jiyengar@fastly.com>
Copy link
Collaborator

@hfujita hfujita left a comment

Choose a reason for hiding this comment

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

Looks good, excited to see fusion in quicly and h2o!

@kazuho kazuho merged commit bfc80b7 into master Jun 19, 2020
@kazuho
Copy link
Member Author

kazuho commented Jun 19, 2020

@janaiyengar @hfujita Thank you for the reviews. Merged.

@AloysAugustin
Copy link

Hi @kazuho , thank you for pinging us on this PR. It took us a bit of time, but we have now integrated this in VPP, and it results in cleaner / less code :-)

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.

Reducing crypto overhead
4 participants