valkey-io / valkey

A flexible distributed key-value datastore that is optimized for caching and other realtime workloads.
https://valkey.io
Other
17.42k stars 656 forks source link

Offload TLS negotiation to I/O threads #1338

Open uriyage opened 4 days ago

uriyage commented 4 days ago

TLS Negotiation Offloading to I/O Threads

Overview

This PR introduces the ability to offload TLS handshake negotiations to I/O threads, significantly improving performance under high TLS connection loads.

Key Changes

Performance Impact

Testing with 650 clients with SET commands and 160 new TLS connections per second in the background:

Throughput Impact of new TLS connections

New Connection Rate

Implementation Details

  1. Main Thread:

    • Initiates negotiation-offload jobs to I/O threads
    • Adds connections to pending-read clients list (using existing read offload mechanism)
    • Post-negotiation handling:
      • Creates read/write events if needed for incomplete negotiations
      • Calls accept handler for completed negotiations
  2. I/O Thread:

    • Performs TLS negotiation
    • Updates connection flags based on negotiation result

Related issue:https://github.com/valkey-io/valkey/issues/761

codecov[bot] commented 4 days ago

Codecov Report

Attention: Patch coverage is 7.14286% with 26 lines in your changes missing coverage. Please review.

Project coverage is 70.74%. Comparing base (3d0c834) to head (dc5aa76). Report is 14 commits behind head on unstable.

Files with missing lines Patch % Lines
src/io_threads.c 0.00% 25 Missing :warning:
src/networking.c 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## unstable #1338 +/- ## ============================================ + Coverage 70.71% 70.74% +0.02% ============================================ Files 115 116 +1 Lines 63159 63307 +148 ============================================ + Hits 44666 44786 +120 - Misses 18493 18521 +28 ``` | [Files with missing lines](https://app.codecov.io/gh/valkey-io/valkey/pull/1338?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io) | Coverage Δ | | |---|---|---| | [src/connection.h](https://app.codecov.io/gh/valkey-io/valkey/pull/1338?src=pr&el=tree&filepath=src%2Fconnection.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL2Nvbm5lY3Rpb24uaA==) | `87.05% <ø> (-0.16%)` | :arrow_down: | | [src/server.c](https://app.codecov.io/gh/valkey-io/valkey/pull/1338?src=pr&el=tree&filepath=src%2Fserver.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL3NlcnZlci5j) | `87.65% <100.00%> (+0.02%)` | :arrow_up: | | [src/server.h](https://app.codecov.io/gh/valkey-io/valkey/pull/1338?src=pr&el=tree&filepath=src%2Fserver.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL3NlcnZlci5o) | `100.00% <ø> (ø)` | | | [src/tls.c](https://app.codecov.io/gh/valkey-io/valkey/pull/1338?src=pr&el=tree&filepath=src%2Ftls.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL3Rscy5j) | `100.00% <ø> (ø)` | | | [src/networking.c](https://app.codecov.io/gh/valkey-io/valkey/pull/1338?src=pr&el=tree&filepath=src%2Fnetworking.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL25ldHdvcmtpbmcuYw==) | `88.53% <50.00%> (-0.14%)` | :arrow_down: | | [src/io\_threads.c](https://app.codecov.io/gh/valkey-io/valkey/pull/1338?src=pr&el=tree&filepath=src%2Fio_threads.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL2lvX3RocmVhZHMuYw==) | `6.94% <0.00%> (-0.67%)` | :arrow_down: | ... and [17 files with indirect coverage changes](https://app.codecov.io/gh/valkey-io/valkey/pull/1338/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io)

🚨 Try these New Features:

uriyage commented 4 days ago

This implementation basically breaks the connection abstraction, since it has the TLS implementation calling functions related to IO threading (which is supposed to be agnostic to the connection type). I was sort of expecting we would offload the event handler.

We still don't check in any point for the connection type, since the TLS code calls the IO threads to offload the negotiation with a supplied callback, not the other way around. Maybe we can rename it to 'accept' instead of 'tls_negotiate' to be more abstract.

I was sort of expecting we would offload the event handler.

Not sure I get this, would you please elaborate.