Hello! On Wed, Apr 10, 2019 at 02:45:52PM +0300, ben ben ishay wrote:
> # HG changeset patch > # User ben ben ishay <benis...@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 > benchmark(https://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: https://lists.freebsd.org/pipermail/freebsd-transport/2018-February/000196.html 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: https://github.com/openssl/openssl/commit/6ba76c4f23e4b4ddc27b9e7234c8b9c3bcff5eff#diff-869032903e697780f95495f7e44410b1R127 As such, the patch doesn't look correct to me (or at least OpenSSL's interface needs further clarification). [...] > @@ -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. [...] -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
