Hello!
Thank you for the review.
we create another patch that take care of your
notes.(https://forum.nginx.org/read.php?29,283833)


On 4/10/2019 6:04 PM, Maxim Dounin wrote:
> Hello!
>
> On Wed, Apr 10, 2019 at 02:45:52PM +0300, ben ben ishay wrote:
>
>> # HG changeset patch
>> # User ben ben ishay <benishay@mellanox.com>
>> # Date 1554896607 -10800
>> # Wed Apr 10 14:43:27 2019 +0300
>> # Node ID 87938decdb98bf4a06ed18002a15156a5e8fbd67
>> # Parent 65074e13f1716e09c28d730586babad7930b7a98
>> Add support for using sendfile when openssl support ktls
>>
>> when we need to transfer data between file and socket we prefer to use sendfile instead of write because we save the copy to a buffer.
>> the use of sendfile is possible in openssl only if it support ktls(the master of openssl support ktls) otherwise there is a copy of the data to userspace for encryption in any case (this paper explain this https://netdevconf.org/1.2/papers/ktls.pdf ).
>> the patch change the flow when the request is to send data over ssl and also the nginx use openssl that support ktls, the new flow using the sendfile function that tcp use for send data (ngx_linux_sendfile_chain).
>> the performence with this patch applied was check with apib benchmarkhttps://github.com/apigee/apib), one machine run nginx and the other machine that connect back to back to the first one run apib with this comand: ./apib -c <num of connection> -d 30 https://<ip address>/<file name to send>.
>> the file size was 100K.
>>
>> the result display in this table , each value represnt average throughput in GBps of 10 runs.
>>
>> num of connection | regular nginx | new nginx
>> 1 5 5.2
>> 2 7.5 8.5
>> 3 7.7 9
>>
>> this result prove that this patch increase nginx performance and thus is useful.
>
> Thank you for your patch. We've helped to develop similar
> functionality by Netflix for in-kernel TLS on FreeBSD (an earlier
> paper is referenced by the ktls.pdf you've linked). See, for
> example, this post for a high-level description:
>
>[...]
>
> The most obvious difference one can observe is that the
> application-level code instead uses SSL_sendfile() call as
> provided by the SSL library, and it is library responsibility to
> make sure keys are properly synced with the kernel when
> kernel-level functions are called.
>
> In contrast, in your patch you assume that as long as
> BIO_get_ktls_send() returns true it is safe to use native kernel
> functions. This looks unsafe, at least without a documentation
> which explicitly states otherwise, as various control messages
> might interfere with direct calls on the socket. Moreover, quick
> look at the code seems to suggest that this is indeed might be
> unsafe - before writing anything to the socket OpenSSL checks if
> there are any pending control messages, and using sendfile()
> directly won't allow this to happen:
>
> [...]
>
> As such, the patch doesn't look correct to me (or at least
> OpenSSL's interface needs further clarification).
>
> [...]
we have a patch for openssl that introduces SSL_sendfile
(https://github.com/openssl/openssl/pull/8727), this function handled
the control message and we use it instead of the regular sendfile.
>
>> @@ -140,3 +140,12 @@
>> fi
>>
>> fi
>> +ngx_feature="OpenSSL library with KTLS"
>> +ngx_feature_name="NGX_OPENSSL_KTLS"
>> +ngx_feature_run=no
>> +ngx_feature_incs="#include \"openssl/bio.h\" "
>> +ngx_feature_path=
>> +ngx_feature_libs="-lssl -lcrypto $NGX_LIBDL $NGX_LIBPTHREAD"
>> +ngx_feature_test="BIO_get_ktls_send(NULL)"
>> +. auto/feature
>> +
>
> Note that we don't really use configure-time feature tests for
> OpenSSL. Instead, consider checking appropriate #define, such as
> #ifdef BIO_get_ktls_send.
>
>> diff -r 65074e13f171 -r 87938decdb98 src/event/ngx_event_openssl.c
>> --- a/src/event/ngx_event_openssl.c Tue Mar 26 09:33:57 2019 +0300
>> +++ b/src/event/ngx_event_openssl.c Wed Apr 10 14:43:27 2019 +0300
>> @@ -1528,6 +1528,9 @@
>> #endif
>>
>> sc->connection = SSL_new(ssl->ctx);
>> +#if (NGX_OPENSSL_KTLS)
>> + sc->ktls = 0;
>> +#endif
>>
>> if (sc->connection == NULL) {
>> ngx_ssl_error(NGX_LOG_ALERT, c->log, 0, "SSL_new() failed");
>> @@ -1639,6 +1642,12 @@
>> c->recv_chain = ngx_ssl_recv_chain;
>> c->send_chain = ngx_ssl_send_chain;
>>
>> +#if (NGX_OPENSSL_KTLS)
>> + if(BIO_get_ktls_send(SSL_get_wbio(c->ssl->connection))){
>> + c->ssl->ktls = 1;
>> + c->send_chain = ngx_linux_sendfile_chain;
>> + }
>> +#endif
>
> Note that compiling this will fail on anything but Linux as long
> as BIO_get_ktls_send() is present in the OpenSSL library, as
> ngx_linux_sendfile_chain() is only available on Linux.
>
> [...]
>
In these days the BIO_get_ktls_send supported only in linux, we add to
the new patch a #if (NGX_LINUX) that ensure this code will compile only
if the nginx runs on linux, thus we can use the generic fucntion
ngx_send_chain instead.
we want to hear your opinion about this, we didnt change the no-linux
sendfile function to support SSL_sendfile cause we cant check the
correctness of this on our system, and the openssl dont support it as
well. if you think we need to implement this although we cant verify it
Please let us know what you think.

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel