[PATCH iproute2-next] ss: Shorter display format for TLS zerocopy sendfile

[PATCH iproute2-next] ss: Shorter display format for TLS zerocopy sendfile

[Maxim Mikityanskiy]

Commit 21c07b45688f ("ss: Show zerocopy sendfile status of TLS
sockets") started displaying the activation status of zerocopy sendfile
on TLS sockets, exposed via sock_diag. This commit makes the format more
compact: the flag's name is shorter and is printed only when the feature
is active, similar to other flag options.

Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
---
 misc/ss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index c4434a20..2bbb3b4d 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(" zc_sendfile");
 }
 
 static void mptcp_subflow_info(struct rtattr *tb[])
-- 
2.25.1

[PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Jakub Kicinski]

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. Avoid "salesman speak", the term "zero-copy"
is particularly confusing in TLS where we call decrypt/encrypt
directly from user space a zero-copy as well.

Signed-off-by: Jakub Kicinski <kuba@kernel.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[])
-- 
2.36.1

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Maxim Mikityanskiy]

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@kernel.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[])

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Jakub Kicinski]

On Thu, 2 Jun 2022 12:13:53 +0300 Maxim Mikityanskiy wrote:
> I would expect to get a comment on my patch, instead of submitting your 
> own v2

I replied to v1 that the name is not okay. And you didn't go with my
suggestion. This is a trivial matter, faster for me to send my own
patch.

> and dropping my author and signed-off-by.

Sorry, you can add it now tho.

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

Sure, there discrepancy is a little annoying. Do you want to send 
the kernel rename patch, or should I?

> > 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 your thinking based on? I spent the last 8 months in meetings
about TLS and I had to explain over and over that TLS zero-copy is not
zero-copy. Granted that's the SW path that's to blame as it moves data
from one place to another and still calls that zero-copy. But the term
zero-copy is tainted for all of kernel TLS at this point.

Unless we report a matrix with the number of copies per syscall I'd
prefer to avoid calling random ones zero-copy again.

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

The sockopt is a promise from the user space to the kernel that it will
not modify the data in the file. So I'd prefer to call it sendfile_ro. 
I have a similar (in spirit) optimization I'll send out for the Rx path
which saves the SW path from making a copy when the user knows that
there will be no TLS 1.3 padding. I want to call it expect_nopad or
such, not tls13_zc or tls13_onefewercopy or IDK what.

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

Everyone wants zero-copy, if that's what was being declared here
we should just default it to enabled and not bother.

Developers need to read the fine print first.

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

I don't understand why you care about the numbers. They will be
meaningless for other platforms (AMD), other versions of your NICs, and
under real application loads. It seems like a declaration of a
performance boost which can't be accurately delivered on. Do you really
want the users to call you and say "your numbers show 40% improvement
but we only see 20%"? I'm not dead-set on excluding the numbers, I just
can't recall ever seeing numbers for a particular NIC included in the
documentation for a feature or an API.

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

I put that sentence in there because I thought you'd appreciate it.
I can remove it if it makes my opinion look inconsistent.
Trying to be nice always backfires for me, eh.

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Maxim Mikityanskiy]

On 2022-06-02 19:44, Jakub Kicinski wrote:
> On Thu, 2 Jun 2022 12:13:53 +0300 Maxim Mikityanskiy wrote:
>> I would expect to get a comment on my patch, instead of submitting your
>> own v2
> 
> I replied to v1 that the name is not okay. And you didn't go with my
> suggestion.

Well, you didn't go with it either :)

I got two suggestions: "unsafe_sendfile" from you and "zc_sendfile" from 
a maintainer of iproute2. I had to choose one. I chose zc_sendfile, 
because it matches the name of the feature and comes from the iproute2 
maintainer. Sorry, but "unsafe_sendfile" doesn't make any sense to me 
and looks like a false ad and an attempt to put the feature in a bad light.

> This is a trivial matter, faster for me to send my own
> patch.
> 
>> and dropping my author and signed-off-by.
> 
> Sorry, you can add it now tho.
> 
>> 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.
> 
> Sure, there discrepancy is a little annoying. Do you want to send
> the kernel rename patch, or should I?

You reviewed the kernel patch and were fine with the naming. Could you 
tell me what happened after merging the patch, what changed your mind 
and made you unhappy about it?

>>> 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 your thinking based on?

You quoted the answer, I don't have much to add.

> I spent the last 8 months in meetings
> about TLS and I had to explain over and over that TLS zero-copy is not
> zero-copy. Granted that's the SW path that's to blame as it moves data
> from one place to another and still calls that zero-copy. But the term
> zero-copy is tainted for all of kernel TLS at this point.

That sounds like a good reason to rename the "zero-copy which is not 
actually zero-copy" feature. On the other hand, zerocopy sendfile is 
truly zerocopy, it doesn't have this issue.

> Unless we report a matrix with the number of copies per syscall I'd
> prefer to avoid calling random ones zero-copy again.
> 
>> 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.
> 
> The sockopt is a promise from the user space to the kernel that it will
> not modify the data in the file. So I'd prefer to call it sendfile_ro.

That's another way of thinking about it. So, one way is to request 
specific effects and deal with the limitations. Another way is to 
declare the limitations and let the supported optimizations kick in 
automatically. Both approaches look valid, but I have to think about it. 
It's hard to figure out which is better when we have only one 
optimization and one limitation.

> I have a similar (in spirit) optimization I'll send out for the Rx path
> which saves the SW path from making a copy when the user knows that
> there will be no TLS 1.3 padding. I want to call it expect_nopad or
> such, not tls13_zc or tls13_onefewercopy or IDK what.

The fact that zerocopy is a bad name for your feature doesn't mean that 
it's a bad name for mine.

I don't have all the details about your feature, but I suppose it's not 
truly zerocopy, because a copy is made when the userspace calls recv(). 
On the other hand, zerocopy sendfile is zerocopy all the way through 
from the file cache to the send WQE.

>> 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".
>>
>>   > > 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.
> 
> Everyone wants zero-copy, if that's what was being declared here
> we should just default it to enabled and not bother.
> 
> Developers need to read the fine print first.
> 
>>   > 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.
> 
> I don't understand why you care about the numbers. They will be
> meaningless for other platforms (AMD), other versions of your NICs, and
> under real application loads. It seems like a declaration of a
> performance boost which can't be accurately delivered on. Do you really
> want the users to call you and say "your numbers show 40% improvement
> but we only see 20%"? I'm not dead-set on excluding the numbers, I just
> can't recall ever seeing numbers for a particular NIC included in the
> documentation for a feature or an API.

As I said in the previous email, it's not about having the numbers in 
the documentation. I'm not pushing for that, and I totally agree the 
numbers will be different for everyone. I simply suggested adding a note 
that zerocopy means performance increase, and I said that if it's 
obvious for the target audience, I'm fine with the current version.

>> 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?
> 
> I put that sentence in there because I thought you'd appreciate it.
> I can remove it if it makes my opinion look inconsistent.
> Trying to be nice always backfires for me, eh.

I'm sorry if I didn't read your intention right, but I felt the opposite 
of nice when I started receiving derogatory nicknames for my feature in 
a passive-aggressive manner.

We could have prevented all the miscommunication if you had sent me a 
note at the point when you felt we need to rename the whole feature. 
Instead, I was under impression that you suddenly started hating my 
feature, and I couldn't really get why.

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Jakub Kicinski]

On Fri, 3 Jun 2022 16:47:43 +0300 Maxim Mikityanskiy wrote:
> >> 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.  
> > 
> > Sure, there discrepancy is a little annoying. Do you want to send
> > the kernel rename patch, or should I?  
> 
> You reviewed the kernel patch and were fine with the naming. Could you 
> tell me what happened after merging the patch, what changed your mind 
> and made you unhappy about it?

Ah, I had the explanation but I cut it to keep the email shorter :S

The difference is that the person writing the code (who will interact
with kernel defines) is likely to have a deeper understanding of the
technology and have read the doc. My concern is that an ss user will
have much more superficial understanding of the internals so we need 
to be more careful to present the information in the most meaningful
way.

E.g. see the patch for changing dev->operstate to UP from UNKNOWN
because users are "confused". If you just call the thing "zc is enabled"
I'm afraid users will start reporting that the "go fast mode" is not
engaged as a bug, without appreciation for the possible side effects.

> > I spent the last 8 months in meetings
> > about TLS and I had to explain over and over that TLS zero-copy is not
> > zero-copy. Granted that's the SW path that's to blame as it moves data
> > from one place to another and still calls that zero-copy. But the term
> > zero-copy is tainted for all of kernel TLS at this point.  
> 
> That sounds like a good reason to rename the "zero-copy which is not 
> actually zero-copy" feature. On the other hand, zerocopy sendfile is 
> truly zerocopy, it doesn't have this issue.

Well, maybe, or maybe the SW path does not make a copy either just
*crypts to a different buffer. IDK if that's a copy.

> > Unless we report a matrix with the number of copies per syscall I'd
> > prefer to avoid calling random ones zero-copy again.

This was a serious suggestion BTW. More legwork, but I believe it'd be
quite useful. If we could express the "number of data movements" in a
more comprehensive manner it'd be helpful for all the cases, and you'd
get the "0" for the sendfile.

Hopefully such a matrix would be complicated enough to make people look
at the docs for an explanation of the details.

> >> 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.  
> > 
> > The sockopt is a promise from the user space to the kernel that it will
> > not modify the data in the file. So I'd prefer to call it sendfile_ro.  
> 
> That's another way of thinking about it. So, one way is to request 
> specific effects and deal with the limitations. Another way is to 
> declare the limitations and let the supported optimizations kick in 
> automatically. Both approaches look valid, but I have to think about it. 
> It's hard to figure out which is better when we have only one 
> optimization and one limitation.

Dunno if it's useful but FWIW I pushed my WIP branch out:

https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=tls-wip&id=d923f1049a1ae1c2bdc1d8f0081fd9f3a35d4155
https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=tls-wip&id=b814ee782eef62d6e2602ab3ba7b31ca03cfe44c

> >> 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?  
> > 
> > I put that sentence in there because I thought you'd appreciate it.
> > I can remove it if it makes my opinion look inconsistent.
> > Trying to be nice always backfires for me, eh.  
> 
> I'm sorry if I didn't read your intention right, but I felt the opposite 
> of nice when I started receiving derogatory nicknames for my feature in 
> a passive-aggressive manner.
> 
> We could have prevented all the miscommunication if you had sent me a 
> note at the point when you felt we need to rename the whole feature. 
> Instead, I was under impression that you suddenly started hating my 
> feature, and I couldn't really get why.

Not at all, sorry. In fact I hope you / someone implements a similar
thing for sendmsg. At which point I may be involved in people using
it. Therefore I started to care about user reports / complaints coming
in and me having to explain the context over and over :(

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Maxim Mikityanskiy]

On 2022-06-03 18:51, Jakub Kicinski wrote:
> On Fri, 3 Jun 2022 16:47:43 +0300 Maxim Mikityanskiy wrote:
>>>> 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.
>>>
>>> Sure, there discrepancy is a little annoying. Do you want to send
>>> the kernel rename patch, or should I?
>>
>> You reviewed the kernel patch and were fine with the naming. Could you
>> tell me what happened after merging the patch, what changed your mind
>> and made you unhappy about it?
> 
> Ah, I had the explanation but I cut it to keep the email shorter :S
> 
> The difference is that the person writing the code (who will interact
> with kernel defines) is likely to have a deeper understanding of the
> technology and have read the doc. My concern is that an ss user will
> have much more superficial understanding of the internals so we need
> to be more careful to present the information in the most meaningful
> way.
> 
> E.g. see the patch for changing dev->operstate to UP from UNKNOWN
> because users are "confused". If you just call the thing "zc is enabled"
> I'm afraid users will start reporting that the "go fast mode" is not
> engaged as a bug, without appreciation for the possible side effects.

That makes some sense to me. What about calling the ss flag 
"zc_sendfile_ro" or "zc_ro_sendfile"? It will still be clear it's 
zerocopy, but with some nuance.

>>> I spent the last 8 months in meetings
>>> about TLS and I had to explain over and over that TLS zero-copy is not
>>> zero-copy. Granted that's the SW path that's to blame as it moves data
>>> from one place to another and still calls that zero-copy. But the term
>>> zero-copy is tainted for all of kernel TLS at this point.
>>
>> That sounds like a good reason to rename the "zero-copy which is not
>> actually zero-copy" feature. On the other hand, zerocopy sendfile is
>> truly zerocopy, it doesn't have this issue.
> 
> Well, maybe, or maybe the SW path does not make a copy either just
> *crypts to a different buffer. IDK if that's a copy.

I consider that a copy, but I understand why someone could have another 
vision on it.

>>> Unless we report a matrix with the number of copies per syscall I'd
>>> prefer to avoid calling random ones zero-copy again.
> 
> This was a serious suggestion BTW. More legwork, but I believe it'd be
> quite useful. If we could express the "number of data movements" in a
> more comprehensive manner it'd be helpful for all the cases, and you'd
> get the "0" for the sendfile.

Sounds like a good idea for a future plan, as long as this matrix is 
maintained properly when new optimizations are added.

> Hopefully such a matrix would be complicated enough to make people look
> at the docs for an explanation of the details.
> 
>>>> 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.
>>>
>>> The sockopt is a promise from the user space to the kernel that it will
>>> not modify the data in the file. So I'd prefer to call it sendfile_ro.
>>
>> That's another way of thinking about it. So, one way is to request
>> specific effects and deal with the limitations. Another way is to
>> declare the limitations and let the supported optimizations kick in
>> automatically. Both approaches look valid, but I have to think about it.
>> It's hard to figure out which is better when we have only one
>> optimization and one limitation.
> 
> Dunno if it's useful but FWIW I pushed my WIP branch out:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=tls-wip&id=d923f1049a1ae1c2bdc1d8f0081fd9f3a35d4155
> https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=tls-wip&id=b814ee782eef62d6e2602ab3ba7b31ca03cfe44c

I took a glance, and I agree zerocopy isn't the best name for your 
feature. If I wanted to indicate it saves one copy, I would call it 
"direct decrypt". "Expect no pad" also works from the point of view of 
declaring limitations.

Another topic to consider is whether TLS 1.3 should be part of the name, 
and should "TlsDecryptRetry" be more specific (if a future feature also 
retries decryption as a fallback, do we want to count these retries in 
the same counter or in a new counter?)

>>>> 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?
>>>
>>> I put that sentence in there because I thought you'd appreciate it.
>>> I can remove it if it makes my opinion look inconsistent.
>>> Trying to be nice always backfires for me, eh.
>>
>> I'm sorry if I didn't read your intention right, but I felt the opposite
>> of nice when I started receiving derogatory nicknames for my feature in
>> a passive-aggressive manner.
>>
>> We could have prevented all the miscommunication if you had sent me a
>> note at the point when you felt we need to rename the whole feature.
>> Instead, I was under impression that you suddenly started hating my
>> feature, and I couldn't really get why.
> 
> Not at all, sorry. In fact I hope you / someone implements a similar
> thing for sendmsg. At which point I may be involved in people using
> it. Therefore I started to care about user reports / complaints coming
> in and me having to explain the context over and over :(

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Stephen Hemminger]

On Mon, 6 Jun 2022 14:29:02 +0300
Maxim Mikityanskiy <maximmi@nvidia.com> wrote:

> That makes some sense to me. What about calling the ss flag 
> "zc_sendfile_ro" or "zc_ro_sendfile"? It will still be clear it's 
> zerocopy, but with some nuance.

-ENAMETOOLONG

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Jakub Kicinski]

On Mon, 6 Jun 2022 14:29:02 +0300 Maxim Mikityanskiy wrote:
> > The difference is that the person writing the code (who will interact
> > with kernel defines) is likely to have a deeper understanding of the
> > technology and have read the doc. My concern is that an ss user will
> > have much more superficial understanding of the internals so we need
> > to be more careful to present the information in the most meaningful
> > way.
> > 
> > E.g. see the patch for changing dev->operstate to UP from UNKNOWN
> > because users are "confused". If you just call the thing "zc is enabled"
> > I'm afraid users will start reporting that the "go fast mode" is not
> > engaged as a bug, without appreciation for the possible side effects.  
> 
> That makes some sense to me. What about calling the ss flag 
> "zc_sendfile_ro" or "zc_ro_sendfile"? It will still be clear it's 
> zerocopy, but with some nuance.

That'd be an acceptable compromise. Hopefully sufficiently forewarned
users will mentally remove the zc_ part and still have a meaningful
amount of info about what the flag does.

Any reason why we wouldn't reuse the same knob for zc sendmsg()? If we
plan to reuse it we can s/sendfile/send/ to shorten the name, perhaps.

> > Dunno if it's useful but FWIW I pushed my WIP branch out:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=tls-wip&id=d923f1049a1ae1c2bdc1d8f0081fd9f3a35d4155
> > https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=tls-wip&id=b814ee782eef62d6e2602ab3ba7b31ca03cfe44c  
> 
> I took a glance, and I agree zerocopy isn't the best name for your 
> feature. If I wanted to indicate it saves one copy, I would call it 
> "direct decrypt". "Expect no pad" also works from the point of view of 
> declaring limitations.
> 
> Another topic to consider is whether TLS 1.3 should be part of the name, 
> and should "TlsDecryptRetry" be more specific (if a future feature also 
> retries decryption as a fallback, do we want to count these retries in 
> the same counter or in a new counter?)

I wanted to avoid the versions because TLS 1.4 may need the same
optimization.

You have a point about the more specific counter, let me add a counter
for NoPad being violated (tail == 0) as well as the overall "decryption
happened twice" counter.

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Maxim Mikityanskiy]

On 2022-06-06 20:59, Jakub Kicinski wrote:
> On Mon, 6 Jun 2022 14:29:02 +0300 Maxim Mikityanskiy wrote:
>>> The difference is that the person writing the code (who will interact
>>> with kernel defines) is likely to have a deeper understanding of the
>>> technology and have read the doc. My concern is that an ss user will
>>> have much more superficial understanding of the internals so we need
>>> to be more careful to present the information in the most meaningful
>>> way.
>>>
>>> E.g. see the patch for changing dev->operstate to UP from UNKNOWN
>>> because users are "confused". If you just call the thing "zc is enabled"
>>> I'm afraid users will start reporting that the "go fast mode" is not
>>> engaged as a bug, without appreciation for the possible side effects.
>>
>> That makes some sense to me. What about calling the ss flag
>> "zc_sendfile_ro" or "zc_ro_sendfile"? It will still be clear it's
>> zerocopy, but with some nuance.
> 
> That'd be an acceptable compromise. Hopefully sufficiently forewarned
> users will mentally remove the zc_ part and still have a meaningful
> amount of info about what the flag does.
> 
> Any reason why we wouldn't reuse the same knob for zc sendmsg()? If we
> plan to reuse it we can s/sendfile/send/ to shorten the name, perhaps.

We can even make it as short as zc_ro_tx in that case.

Regarding sendmsg, I can't anticipate what knob will be used. There is 
MSG_ZEROCOPY which is also a candidate.

Note that the constant in the header file has "SENDFILE" in its name, so 
if you want to reuse it for the future sendmsg zerocopy, we should think 
about renaming it in advance, before anyone starts using it. 
Alternatively, an alias for this constant can be added in the future.

>>> Dunno if it's useful but FWIW I pushed my WIP branch out:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=tls-wip&id=d923f1049a1ae1c2bdc1d8f0081fd9f3a35d4155
>>> https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=tls-wip&id=b814ee782eef62d6e2602ab3ba7b31ca03cfe44c
>>
>> I took a glance, and I agree zerocopy isn't the best name for your
>> feature. If I wanted to indicate it saves one copy, I would call it
>> "direct decrypt". "Expect no pad" also works from the point of view of
>> declaring limitations.
>>
>> Another topic to consider is whether TLS 1.3 should be part of the name,
>> and should "TlsDecryptRetry" be more specific (if a future feature also
>> retries decryption as a fallback, do we want to count these retries in
>> the same counter or in a new counter?)
> 
> I wanted to avoid the versions because TLS 1.4 may need the same
> optimization.
> 
> You have a point about the more specific counter, let me add a counter
> for NoPad being violated (tail == 0) as well as the overall "decryption
> happened twice" counter.

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Jakub Kicinski]

On Tue, 7 Jun 2022 13:35:19 +0300 Maxim Mikityanskiy wrote:
> > That'd be an acceptable compromise. Hopefully sufficiently forewarned
> > users will mentally remove the zc_ part and still have a meaningful
> > amount of info about what the flag does.
> > 
> > Any reason why we wouldn't reuse the same knob for zc sendmsg()? If we
> > plan to reuse it we can s/sendfile/send/ to shorten the name, perhaps.  
> 
> We can even make it as short as zc_ro_tx in that case.

SG

> Regarding sendmsg, I can't anticipate what knob will be used. There is 
> MSG_ZEROCOPY which is also a candidate.

Right, that's what I'm wondering. MSG_ZEROCOPY already has some
restrictions on user not touching the data but technically a pure 
TCP connection will not be broken if the data is modified. I'd lean
towards requiring the user setting zc_ro_tx, but admittedly I don't
have a very strong reason.

> Note that the constant in the header file has "SENDFILE" in its name, so 
> if you want to reuse it for the future sendmsg zerocopy, we should think 
> about renaming it in advance, before anyone starts using it. 
> Alternatively, an alias for this constant can be added in the future.

Would be good to rename it to whatever we settle for on the iproute2
side. Are we going with zc_ro_tx, then?

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Stephen Hemminger]

On Tue, 7 Jun 2022 10:30:28 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 7 Jun 2022 13:35:19 +0300 Maxim Mikityanskiy wrote:
> > > That'd be an acceptable compromise. Hopefully sufficiently forewarned
> > > users will mentally remove the zc_ part and still have a meaningful
> > > amount of info about what the flag does.
> > > 
> > > Any reason why we wouldn't reuse the same knob for zc sendmsg()? If we
> > > plan to reuse it we can s/sendfile/send/ to shorten the name, perhaps.    
> > 
> > We can even make it as short as zc_ro_tx in that case.  
> 
> SG
> 
> > Regarding sendmsg, I can't anticipate what knob will be used. There is 
> > MSG_ZEROCOPY which is also a candidate.  
> 
> Right, that's what I'm wondering. MSG_ZEROCOPY already has some
> restrictions on user not touching the data but technically a pure 
> TCP connection will not be broken if the data is modified. I'd lean
> towards requiring the user setting zc_ro_tx, but admittedly I don't
> have a very strong reason.
> 
> > Note that the constant in the header file has "SENDFILE" in its name, so 
> > if you want to reuse it for the future sendmsg zerocopy, we should think 
> > about renaming it in advance, before anyone starts using it. 
> > Alternatively, an alias for this constant can be added in the future.  
> 
> Would be good to rename it to whatever we settle for on the iproute2
> side. Are we going with zc_ro_tx, then?

Works for me

Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS zerocopy sendfile

[Maxim Mikityanskiy]

On 2022-06-07 20:30, Jakub Kicinski wrote:
> On Tue, 7 Jun 2022 13:35:19 +0300 Maxim Mikityanskiy wrote:
>>> That'd be an acceptable compromise. Hopefully sufficiently forewarned
>>> users will mentally remove the zc_ part and still have a meaningful
>>> amount of info about what the flag does.
>>>
>>> Any reason why we wouldn't reuse the same knob for zc sendmsg()? If we
>>> plan to reuse it we can s/sendfile/send/ to shorten the name, perhaps.
>>
>> We can even make it as short as zc_ro_tx in that case.
> 
> SG
> 
>> Regarding sendmsg, I can't anticipate what knob will be used. There is
>> MSG_ZEROCOPY which is also a candidate.
> 
> Right, that's what I'm wondering. MSG_ZEROCOPY already has some
> restrictions on user not touching the data but technically a pure
> TCP connection will not be broken if the data is modified.

Sounds similar to sendfile. With bare TCP, the user shouldn't modify the 
sendfile data, but the connection isn't broken in any case. With TLS, 
the connection may be broken, so we require an explicit opt-in. So, I 
think, a similar rule for MSG_ZEROCOPY will make sense: MSG_ZEROCOPY 
works out of the box with bare TCP, because the connection can't be 
severed by modifying data, but it will require an opt-in for TLS.

> I'd lean
> towards requiring the user setting zc_ro_tx, but admittedly I don't
> have a very strong reason.
> 
>> Note that the constant in the header file has "SENDFILE" in its name, so
>> if you want to reuse it for the future sendmsg zerocopy, we should think
>> about renaming it in advance, before anyone starts using it.
>> Alternatively, an alias for this constant can be added in the future.
> 
> Would be good to rename it to whatever we settle for on the iproute2
> side. Are we going with zc_ro_tx, then?

Yes, I'll submit the patches.