ktls / af_ktls

Linux Kernel TLS/DTLS Module
GNU General Public License v2.0
159 stars 25 forks source link

Remove rx async work #69

Closed ilyalesokhin closed 8 years ago

ilyalesokhin commented 8 years ago

Can you please explain why the rx async work is needed? Why can't we read the data and decrypt it inside tls_recvmsg from process context?

I'm guessing the concern is that we will deadlock if there is not enough room for 1 record in the TCP socket. But can't we just enforce SO_RCVBUF > 16KB?

fridex commented 8 years ago

The original idea was to decrypt asynchronously to user space and provide data with cost of a data copy (not to wait for Crypto API decryption).

ilyalesokhin commented 8 years ago

I guess the optimal solution depends on the use case. But I think we should consider removing the rx work on the following ground:

  1. The CPU usage of the decryption is not attributed to the correct process.
  2. If the user application is waiting in tls_recv message this is clearly a loss as it adds more data shuffling and context switches. In normal TCP proccessing, the kernel doesn't linearize incoming SKB asynchronously to speed up later tcp_recvmsg calls.
fridex commented 8 years ago

I guess the optimal solution depends on the use case.

Yes, exactly. In the worst case, if user space hits worker, there will be additional copy involved.

But I think we should consider removing the rx work on the following ground:

  1. The CPU usage of the decryption is not attributed to the correct process.
  2. If the user application is waiting in tls_recv message this is clearly a loss as it adds more data shuffling and context switches. In normal TCP proccessing, the kernel doesn't linearize incoming SKB asynchronously to speed up later tcp_recvmsg calls.

Sounds reasonable, I'm not a kernel dev though. The implementation would be simplified as well.

lancerchao commented 8 years ago

I think OP makes a great point. If we really only cared about throughput then the async worker is a good way to make use of idle cores, but this won't scale to large number of connections.

lancerchao commented 8 years ago

Patch for this #73. Dave will discuss this issue at netdev

djwatson commented 8 years ago

Let's pull this in for now