Message-ID: <bf8c357e-6a1d-4c42-e6f8-f259879b67c6@nvidia.com>
Date:   Thu, 2 Jun 2022 12:13:53 +0300
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>, dsahern@...il.com
Cc:     netdev@...r.kernel.org, stephen@...workplumber.org,
        tariqt@...dia.com
Subject: Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS
 zerocopy sendfile

Hi,

I would expect to get a comment on my patch, instead of submitting your 
own v2 and dropping my author and signed-off-by.

On 2022-06-02 02:42, Jakub Kicinski wrote:
> Commit 21c07b45688f ("ss: Show zerocopy sendfile status of TLS
> sockets") used "key: value" format for the sendfile read-only
> optimization. Move to a more appropriate "flag" display format.
> Rename the flag to something based on the assumption it allows
> the kernel to make.

The kernel feature is exposed to the userspace as "zerocopy sendfile", 
see the constants for setsockopt and sock_diag. ss should just print 
whatever is exposed via sock_diag as is. IMO, inventing new names for it 
would cause confusion. Calling the feature by the same name everywhere 
looks clearer to me.

> the term "zero-copy"
> is particularly confusing in TLS where we call decrypt/encrypt
> directly from user space a zero-copy as well.

I don't think "zerocopy_sendfile" is confusing. There is no second 
zerocopy sendfile, and the zero-copy you are talking about is neither 
related to sendfile nor exposed to the userspace, as far as I see.

What is confusing is calling a feature not by its name, but by one of 
its implications, and picking a name that doesn't have any references 
elsewhere.

I believe, we are going to have more and more zerocopy features in the 
kernel, and it's OK to distinguish them by "zerocopy TLS sendfile", 
"zerocopy AF_XDP", etc. This is why my feature isn't called just "zerocopy".

 > >> 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.

My intention was to emphasize some positive points and give the readers 
understanding why they may want to enable this feature. "Zero-copy 
behavior" sounds neutral to me, and the following paragraphs describe 
the limitations only, so I wanted to add some positive phrasing like 
"improved performance" or "reduced CPU cycles spent on extra copies". 
"Transmitting data directly to the NIC without making an in-kernel copy" 
implies these points, but it's not explicit. If you think it's obvious 
enough for the target audience, I'm fine with the current version.

 > Avoid "salesman speak", the term "zero-copy"

In the documentation you wrote, "true zero-copy behavior" was an 
acceptable term, and the "ad" was the performance numbers. However, in 
the context of this patch, you call "zerocopy" a "salesman speak". What 
is different in this context that "zerocopy" became an unwanted term?

> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>   misc/ss.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index c4434a20bcfe..ac678c296006 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -2988,7 +2988,8 @@ 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");
> +	if (attr)
> +		out(" sendfile_ro");
>   }
>   
>   static void mptcp_subflow_info(struct rtattr *tb[])

Powered by blists - more mailing lists