Skip to content

Offload TLS negotiation to I/O threads#1338

Merged
ranshid merged 7 commits into
valkey-io:unstablefrom
uriyage:tls_negotiation_offload
Dec 18, 2024
Merged

Offload TLS negotiation to I/O threads#1338
ranshid merged 7 commits into
valkey-io:unstablefrom
uriyage:tls_negotiation_offload

Conversation

@uriyage

@uriyage uriyage commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

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

  • Added infrastructure to offload TLS negotiations to I/O threads
  • Refactored SSL event handling to allow I/O threads modify conn flags.
  • Introduced new connection flag to identify client connections

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

  • With Offloading: Minimal impact (1050K → 990K ops/sec)
  • Without Offloading: Significant drop (1050K → 670K ops/sec)

New Connection Rate

  • With Offloading:
    • 1,757 conn/sec
  • Without Offloading:
    • 477 conn/sec

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:#761

Signed-off-by: Uri Yagelnik <uriy@amazon.com>
@codecov

codecov Bot commented Nov 21, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 12.50000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 70.78%. Comparing base (3d0c834) to head (cea9d4a).
Report is 81 commits behind head on unstable.

Files with missing lines Patch % Lines
src/io_threads.c 0.00% 21 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1338      +/-   ##
============================================
+ Coverage     70.71%   70.78%   +0.06%     
============================================
  Files           115      119       +4     
  Lines         63159    64715    +1556     
============================================
+ Hits          44666    45806    +1140     
- Misses        18493    18909     +416     
Files with missing lines Coverage Δ
src/connection.h 87.50% <ø> (+0.29%) ⬆️
src/networking.c 88.23% <100.00%> (-0.45%) ⬇️
src/server.c 87.46% <100.00%> (-0.17%) ⬇️
src/server.h 100.00% <ø> (ø)
src/tls.c 100.00% <ø> (ø)
src/io_threads.c 6.96% <0.00%> (-0.64%) ⬇️

... and 63 files with indirect coverage changes

@madolson madolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread src/io_threads.c Outdated
Comment thread src/connection.h Outdated
Comment thread src/io_threads.c
@madolson madolson requested a review from ranshid November 21, 2024 22:01
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
@uriyage

uriyage commented Nov 22, 2024

Copy link
Copy Markdown
Contributor Author

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.

@ranshid ranshid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uri. We discussed offline about several architectual changes to reduce the code changes. Also placed some comments I think we can improve.

Comment thread src/tls.c Outdated
Comment thread src/io_threads.c Outdated
Comment thread src/server.h Outdated
Comment thread src/networking.c Outdated
…abstraction

Signed-off-by: Uri Yagelnik <uriy@amazon.com>
@uriyage uriyage force-pushed the tls_negotiation_offload branch from cd86e38 to c0cf818 Compare November 27, 2024 15:22
Comment thread src/tls.c Outdated
Comment thread src/tls.c Outdated
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Comment thread src/tls.c
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
@ranshid

ranshid commented Nov 28, 2024

Copy link
Copy Markdown
Member

@uriyage it looks much better now IMO. I still want @madolson to also take a better look since these parts are very fragile and hard to miss some cases. Let's also wait for her to provide feedback

Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Comment thread src/server.c Outdated
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: ranshid <88133677+ranshid@users.noreply.github.com>
@ranshid ranshid merged commit 8060c86 into valkey-io:unstable Dec 18, 2024
kronwerk pushed a commit to kronwerk/valkey that referenced this pull request Jan 27, 2025
## 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
- Added infrastructure to offload TLS negotiations to I/O threads
- Refactored SSL event handling to allow I/O threads modify conn flags.
- Introduced new connection flag to identify client connections

### 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
- **With Offloading**: Minimal impact (1050K → 990K ops/sec)
- **Without Offloading**: Significant drop (1050K → 670K ops/sec)

#### New Connection Rate
- **With Offloading**: 
  - 1,757 conn/sec
- **Without Offloading**: 
  - 477 conn/sec

### 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:valkey-io#761

---------

Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Signed-off-by: ranshid <88133677+ranshid@users.noreply.github.com>
Co-authored-by: ranshid <88133677+ranshid@users.noreply.github.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.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.

3 participants