[PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[Maxim Mikityanskiy]

Print the activation status of zerocopy sendfile on TLS sockets.
Zerocopy sendfile was recently added to Linux and exposed via sock_diag.

Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
 include/uapi/linux/tls.h | 2 ++
 misc/ss.c                | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/uapi/linux/tls.h b/include/uapi/linux/tls.h
index 3ad54af2..83a3cea4 100644
--- a/include/uapi/linux/tls.h
+++ b/include/uapi/linux/tls.h
@@ -39,6 +39,7 @@
 /* TLS socket options */
 #define TLS_TX			1	/* Set transmit parameters */
 #define TLS_RX			2	/* Set receive parameters */
+#define TLS_TX_ZEROCOPY_SENDFILE	3	/* transmit zerocopy sendfile */
 
 /* Supported versions */
 #define TLS_VERSION_MINOR(ver)	((ver) & 0xFF)
@@ -160,6 +161,7 @@ enum {
 	TLS_INFO_CIPHER,
 	TLS_INFO_TXCONF,
 	TLS_INFO_RXCONF,
+	TLS_INFO_ZC_SENDFILE,
 	__TLS_INFO_MAX,
 };
 #define TLS_INFO_MAX (__TLS_INFO_MAX - 1)
diff --git a/misc/ss.c b/misc/ss.c
index 4b3ca9c4..57677cf2 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2952,6 +2952,11 @@ static void tcp_tls_conf(const char *name, struct rtattr *attr)
 	}
 }
 
+static void tcp_tls_zc_sendfile(struct rtattr *attr)
+{
+	out(" zerocopy_sendfile: %s", attr ? "active" : "inactive");
+}
+
 static void mptcp_subflow_info(struct rtattr *tb[])
 {
 	u_int32_t flags = 0;
@@ -3182,6 +3187,7 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
 			tcp_tls_cipher(tlsinfo[TLS_INFO_CIPHER]);
 			tcp_tls_conf("rxconf", tlsinfo[TLS_INFO_RXCONF]);
 			tcp_tls_conf("txconf", tlsinfo[TLS_INFO_TXCONF]);
+			tcp_tls_zc_sendfile(tlsinfo[TLS_INFO_ZC_SENDFILE]);
 		}
 		if (ulpinfo[INET_ULP_INFO_MPTCP]) {
 			struct rtattr *sfinfo[MPTCP_SUBFLOW_ATTR_MAX + 1] =
-- 
2.25.1

Re: [PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[Stephen Hemminger]

On Mon, 30 May 2022 17:14:38 +0300
Maxim Mikityanskiy <maximmi@nvidia.com> wrote:

> +static void tcp_tls_zc_sendfile(struct rtattr *attr)
> +{
> +	out(" zerocopy_sendfile: %s", attr ? "active" : "inactive");
> +}

I would prefer a shorter output just adding "zc_sendfile" if present and nothing
if not present. That is how other optns like ecn, ecnseen, etc work.

At some point ss needs conversion to json but that will take days of work.

Re: [PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[Maxim Mikityanskiy]

On 2022-05-30 19:24, Stephen Hemminger wrote:
> On Mon, 30 May 2022 17:14:38 +0300
> Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
> 
>> +static void tcp_tls_zc_sendfile(struct rtattr *attr)
>> +{
>> +	out(" zerocopy_sendfile: %s", attr ? "active" : "inactive");
>> +}
> 
> I would prefer a shorter output just adding "zc_sendfile" if present and nothing
> if not present. That is how other optns like ecn, ecnseen, etc work.

I see David merged the patch as is to net-next, despite the comments. 
Should I still make the requested change? If yes, should I submit it as 
a v2 or as a next patch on top of this one?

> At some point ss needs conversion to json but that will take days of work.

Re: [PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[David Ahern]

On 5/31/22 1:00 AM, Maxim Mikityanskiy wrote:
> On 2022-05-30 19:24, Stephen Hemminger wrote:
>> On Mon, 30 May 2022 17:14:38 +0300
>> Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>>
>>> +static void tcp_tls_zc_sendfile(struct rtattr *attr)
>>> +{
>>> +    out(" zerocopy_sendfile: %s", attr ? "active" : "inactive");
>>> +}
>>
>> I would prefer a shorter output just adding "zc_sendfile" if present
>> and nothing
>> if not present. That is how other optns like ecn, ecnseen, etc work.
> 
> I see David merged the patch as is to net-next, despite the comments.
> Should I still make the requested change? If yes, should I submit it as
> a v2 or as a next patch on top of this one?
> 

The patch was merged before the comments. Given that you should be
sending a patch against -next branch that addresses the comments.

Re: [PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[Jakub Kicinski]

On Mon, 30 May 2022 17:14:38 +0300 Maxim Mikityanskiy wrote:
> +	out(" zerocopy_sendfile: %s", attr ? "active" : "inactive");

I'd call it "unsafe_sendfile".

Also please send a patch to Documentation/, I forgot about that.

Re: [PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[Jakub Kicinski]

On Mon, 30 May 2022 11:17:45 -0700 Jakub Kicinski wrote:
> Also please send a patch to Documentation/, I forgot about that.

Actually let me take care of that on my own, I have some optimizations
to add, saves me a rebase ;)

Does this sound good?


Optional optimizations
----------------------

There are certain condition-specific optimizations the TLS ULP can make,
if requested. Those optimizations are either not universally beneficial
or may impact correctness, hence they require an opt-in.
All options are set per-socket using setsockopt(), and their
state can be checked using getsockopt() and via socket diag (``ss``).

TLS_INFO_ZC_SENDFILE
~~~~~~~~~~~~~~~~~~~~

For device offload only. Allow sendfile() data to be transmitted directly
to the NIC without making an in-kernel copy. This allows true zero-copy
behavior when device offload is enabled.

The application must make sure that the data is not modified between being
submitted and transmission completing. In other words this is mostly
applicable if the data sent on a socket via sendfile() is read-only.

Modifying the data may result in different versions of the data being used
for the original TCP transmission and TCP retransmissions. To the receiver
this will look like TLS records had been tampered with and will result
in record authentication failures.

Re: [PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[Jakub Kicinski]

On Tue, 31 May 2022 12:18:29 -0700 Jakub Kicinski wrote:
> TLS_INFO_ZC_SENDFILE

I copy/pasted the diag name, this should be TLS_TX_ZEROCOPY_SENDFILE.

Re: [PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[Maxim Mikityanskiy]

On 2022-05-31 22:18, Jakub Kicinski wrote:
> On Mon, 30 May 2022 11:17:45 -0700 Jakub Kicinski wrote:
>> Also please send a patch to Documentation/, I forgot about that.
> 
> Actually let me take care of that on my own, I have some optimizations
> to add, saves me a rebase ;)
> 
> Does this sound good?
> 
> 
> Optional optimizations
> ----------------------
> 
> There are certain condition-specific optimizations the TLS ULP can make,
> if requested. Those optimizations are either not universally beneficial
> or may impact correctness, hence they require an opt-in.
> All options are set per-socket using setsockopt(), and their
> state can be checked using getsockopt() and via socket diag (``ss``).
> 
> TLS_INFO_ZC_SENDFILE
> ~~~~~~~~~~~~~~~~~~~~
> 
> For device offload only. Allow sendfile() data to be transmitted directly
> to the NIC without making an in-kernel copy. This allows true zero-copy
> behavior when device offload is enabled.

I suggest mentioning the purpose of this optimization: a huge 
performance boost of up to 2.4 times compared to non-zerocopy device 
offload. See the performance numbers from my commit message:

 > Performance numbers in a single-core test with 24 HTTPS streams on
 > nginx, under 100% CPU load:
 >
 > * non-zerocopy: 33.6 Gbit/s
 > * zerocopy: 79.92 Gbit/s
 >
 > CPU: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz

The rest of the text looks good to me and accurately describes the 
limitations, intended use case and possible consequences. Thanks for 
taking care of the documentation!

> The application must make sure that the data is not modified between being
> submitted and transmission completing. In other words this is mostly
> applicable if the data sent on a socket via sendfile() is read-only.
> 
> Modifying the data may result in different versions of the data being used
> for the original TCP transmission and TCP retransmissions. To the receiver
> this will look like TLS records had been tampered with and will result
> in record authentication failures.

Re: [PATCH iproute2-next] ss: Show zerocopy sendfile status of TLS sockets

[Jakub Kicinski]

On Wed, 1 Jun 2022 11:58:22 +0300 Maxim Mikityanskiy wrote:
> > For device offload only. Allow sendfile() data to be transmitted directly
> > to the NIC without making an in-kernel copy. This allows true zero-copy
> > behavior when device offload is enabled.  
> 
> I suggest mentioning the purpose of this optimization: a huge 
> performance boost of up to 2.4 times compared to non-zerocopy device 
> offload. See the performance numbers from my commit message:

That reads like and ad to me.