Message-ID: <20220603085140.26f29d80@kernel.org>
Date:   Fri, 3 Jun 2022 08:51:40 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Maxim Mikityanskiy <maximmi@...dia.com>
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 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 :(

Powered by blists - more mailing lists