FW: [PATCH] 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

Mon May 6 06:02:10 UTC 2019

-----Original Message-----
From: Ben Ben Ishay <benishay at mellanox.com> 
Sent: Monday, April 29, 2019 12:08 PM
To: Maxim Dounin <mdounin at mdounin.ru>; Boris Pismenny <borisp at mellanox.com>
Subject: Re: [PATCH] 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

Hello!
thank you for the review.

we fixed the code style issues. before i send another patch i want to be sure that you agree with our attitude.

On 4/18/2019 4:07 PM, Maxim Dounin wrote:
> Hello!
> 
> On Thu, Apr 18, 2019 at 10:32:56AM +0300, ben ben ishay wrote:
> 
>> # HG changeset patch
>> # User ben ben ishay <benishay at mellanox.com>
>> # Date 1555572726 -10800
>> #      Thu Apr 18 10:32:06 2019 +0300
>> # Node ID bb4c564a9f1c5c721c192e6188967c19aabbc0b9
>> # Parent  a6e23e343081b79eb924da985a414909310aa7a3
>> 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fnetdevconf.org%2F1.2%2Fpapers%2Fktls.pdf&data=02%7C01%7Cbenishay%40mellanox.com%7C2203ce7481ec4c3e562308d6c3fed9f8%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C636911896691245657&sdata=4%2F7ofqINYiEr9m%2B53E%2BA2hxrzb0P9xGeJsFrE4N6a6U%3D&reserved=0 ).
>> 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(this is the source https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapigee%2Fapib)%2Cone&data=02%7C01%7Cbenishay%40mellanox.com%7C2203ce7481ec4c3e562308d6c3fed9f8%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C636911896691255667&sdata=ZzsqevUA%2FOgpiZrIkos1MKoxbGkAEh0m1APYZbyEhrU%3D&reserved=0 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.
> 
> In no particular order:
> 
> - Please read https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fnginx.org%2Fen%2Fdocs%2Fcontributing_changes.html&data=02%7C01%7Cbenishay%40mellanox.com%7C2203ce7481ec4c3e562308d6c3fed9f8%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C636911896691255667&sdata=E%2BJmG06kBUWZfzWna%2FmH4C46u1ZHD%2F03R%2B50evoEOEk%3D&reserved=0 for
>    basic tips on how to submit patches.  In particular, please make
>    sure to follow nginx coding style.
> 
> - Note well that at least one bug in your code is directly related
>    to incorrect indentation and failure to properly use curly
>    brackets as explicitly required by the coding style.  The style
>    is there to prevent such accidental bugs.
> 
> - When you update your patches based on the feedback given,
>    consider replying to the thread in question instead of
>    posting a new thread.  If for some reason you think a new thread
>    will be better, describe these reasons and post a reference to
>    the old thread.  Also, make sure to describe changes you've
>    made based on the feedback.
> 
> - The SSL_sendfile() call you are using in this version does not
>    seem to exists in any published version of OpenSSL, including
>    github repo.  This is not going to work.
> 
we attach a documentation link about this 
function(https://github.com/Mellanox/openssl/blob/tls_sendfile/doc/man3/SSL_write.pod), 
we have a pull request at advanced stage for adding this function.

> - The approach you are using - that is, introducing changes into
>    ngx_linux_sendfile_chain.c - is not portable, and is not going
>    to work on other platforms if/when appropriate kernel level and
>    OpenSSL level support will be available.  As suggested in the
>    previous thread, this should be something handled at the
>    ngx_event_openssl.c level.
> 
we think that this is the best solution because there is a unique 
function for every OS  and there is a difference between them for 
example ngx_linux_sendfile_chain use TCP_CORK option while 
ngx_freebsd_sendfile_chain dosent(in addition in solaris function 
ngx_solaris_sendfilev_chain there is a call to sendfilev that dosent 
exists in linux) , we can create a function that is constructed from the 
diffrenet flow for OS's but we think this option include copying code 
and thus the best option is to change the sendfile call in every 
ngx_sendfile function when the OS and OPENSSL will support SSL_Sendfile.

> [...]
> 