Date:   Mon, 6 Jun 2022 14:29:02 +0300
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     dsahern@...il.com, netdev@...r.kernel.org,
        stephen@...workplumber.org, tariqt@...dia.com
Subject: Re: [PATCH iproute2-next v2] ss: Shorter display format for TLS
 zerocopy sendfile

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 :(

Powered by blists - more mailing lists