Re: [PATCH net-next v2 2/6] net: Introduce read_sock_rectype proto_ops for control record delivery

2026-07-28, 22:51:30 -0400, Chuck Lever wrote:
> 
> 
> On Tue, Jul 28, 2026, at 10:30 PM, Jakub Kicinski wrote:
> > On Tue, 28 Jul 2026 21:57:21 -0400 Chuck Lever wrote:
> >> > To me this is an ugly one-off workaround that doesn't fit into 
> >> > the proto_ops (only TLS will use it). And you seem to net out
> >> > to the same LOC on SUNRPC side with and without this?  
> >> 
> >> It’s not about LOC. It’s about not cluttering the normal I/O
> >> path with a lot of exception processing to handle TLS Alert
> >> records. The CMSG API is very difficult to use and leaks the
> >> alert messages into I/O buffers (which for in-kernel consumers
> >> are page cache pages). It’s piss-poor API design.
> >
> > I'm not arguing that it's amazing. Doesn't mean we will YOLO
> > a special proto callback for every protocol stacking :/
> 
> No-one is asking you to roll over. Review means you get to steer
> us in the right direction, and I promise to do the leg work. Terse
> rejection doesn’t move the discussion forward. It stops it cold.
> 
> Complaining about slop also does not tell me where you need this
> to go. I use AI to go from blank page to RFC/v1. Where we go next
> is up to human taste, as always.

RFC/v1 wasn't sent to netdev.

[jumping to the end of your reply]
> I thought the RFC series cover letter made it clear that we are
> looking for input and direction, not to sell a completely formed idea.

Then this should have been tagged as "RFC v2". "PATCH v2" sounds more
like a fully formed idea.


I see in the RFC thread some doubts about whether read_sock is
actually helpful with TLS.

Ignoring the "does read_sock even help?" aspect, how much improvement
are you seeing by going from "read_sock with fallback to recvmsg+cmsg
in case we get a non-DATA record" to "read_sock_rectype"? (current
svcsock [before this series] doesn't use read_sock, it may be good to
compare those 3 variants and not just "old read_sock vs new
read_sock", but "read_sock vs read_sock++" is the important one to
justify an API change)

Non-DATA record should be fairly uncommon, I'm not that convinced "oh
well let's try again" once in a while causes a measurable degradation.

Even with recvmsg(), you have 2 choices:
 - pass a cmsg every time, and check the record type for every recv
 - don't pass a cmsg, and do a retry when you get -EIO

This proposal (call a different CB depending on record type) is... an
"interesting" approach.

> >> > There needs to be a very strong reason for us to add APIs for
> >> > in kernel consumers.  
> >> 
> >> This is not a helpful position. Your objection is the same
> >> every time, treating the in-kernel users as second-class
> >> citizens.
> >
> > No, it's not a second class citizen. But kernel consumers have a
> > tendency to break all abstractions and insert hacks all over the place
> > just because they are not forced to go via uAPI boundary which forces
> > people to think about the API design.
> 
> Granted that user space self-tests can’t reach kernel-only APIs.
> But that is what Kunit is for.
> 
> 
> > You just need to try a little harder to produce a better solution.
> > Rework or augment existing callbacks to let your achieve the behavior
> > you want.
> 
> My original approach was to add a new read_sock variant because I
> suspected you wouldn’t want read_sock itself to grow another argument.

Given that there's only 2 existing consumers of read_sock (strp and
nvme, and I'm not sure why strp/sockmap use it at all) [1], and 3
arguments to read_sock, adding an argument would be ok IMO. The
implementation (tls_sw_read_sock/tls_sw_read_sock_rectype) ends up
being a small wrapper around a function that does the actual work with
a NULL check, might as well propagate that to the callers.

For me the problem is more that this new argument is very specific to
TLS, and dropping something TLS-specific in a generic API (struct
proto_ops) is quite ugly. If we want to make this generic, we're back
to cmsg (or something cmsg-like). And then the benefit for users of
read_sock gets down to avoiding the "try read_sock, then fall back to
recvmsg" logic.


[1] well, there's also some users that call tcp_read_sock directly
[1], but I think they can be ignored other than "they'll need to pass
NULL since .read_sock = tcp_read_sock"
drivers/infiniband/sw/siw/siw_cm.c	tcp_read_sock(sk, &rd_desc, siw_tcp_rx_data);
drivers/infiniband/sw/siw/siw_qp.c	tcp_read_sock(sk, &rd_desc, siw_tcp_rx_data);
drivers/scsi/iscsi_tcp.c		tcp_read_sock(sk, &rd_desc, iscsi_sw_tcp_recv);
net/rds/tcp_recv.c			tcp_read_sock(sock->sk, &desc, rds_tcp_data_recv);

-- 
Sabrina