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:

quoted

On Tue, 28 Jul 2026 21:57:21 -0400 Chuck Lever wrote:

quoted

quoted

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.

quoted

quoted

quoted

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.

quoted

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