Re: [PATCH 2/2] SUNRPC: Splice a request's pages into the TCP socket

[Mike Snitzer <snitzer@kernel.org>]

On Fri, Nov 13, 2026 at 04:26:06PM -0500, Anna Schumaker wrote:
> Hi Mike,
> 
> On Thu, Nov 12, 2026, at 7:16 PM, Mike Snitzer wrote:
> > xprt_sock_sendmsg() sends a request's page array to the socket as a
> > bvec, and tcp_sendmsg_locked() copies every byte of it into the
> > socket's skbs under the socket lock.  For an NFS WRITE that is the
> > whole payload, copied by rpciod one request at a time a transport: at
> > 1 MiB a WRITE it comes to about 3 GB/s a connection, and on a client
> > with 24 connections to a server whose storage takes more, it is the
> > ceiling of the rate, with 18% of the client's 48 CPUs in memcpy under
> > tcp_sendmsg_locked().
> >
> > Until commit 0472e4766049 ("SUNRPC: Convert socket page send code to
> > use iov_iter()") the client sent the pages with kernel_sendpage(), so
> > the socket held them and copied nothing.  Send them with
> > MSG_SPLICE_PAGES, as the server's svc_tcp_sendmsg() has since
> > commit 5df5dd03a8f7 ("sunrpc: Use sendmsg(MSG_SPLICE_PAGES) rather then
> > sendpage"), and the socket holds them again.  Only the page array is
> > spliced: the head and the tail are kvecs over the request's buffer,
> > memory the socket may not hold a reference to, and are copied as
> > before.
> >
> > A request's pages are the caller's until the reply, and TCP delivers
> > them before the server can reply, so a first transmission is safe to
> > splice.  A retransmission is not: the reply to the first transmission
> > may complete the call and release the pages to be rewritten while the
> > socket still holds them for the second, the corruption that
> > commit a6b31d18b02f ("SUNRPC: Fix a data corruption issue when
> > retransmitting RPC calls") found with kernel_sendpage().  So a
> > retransmission is copied, as it was then, and only the TCP transport
> > asks for the splice: UDP and AF_LOCAL copy as before.
> >
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  net/sunrpc/socklib.c  | 26 ++++++++++++++++++++++----
> >  net/sunrpc/xprtsock.c |  9 +++++++++
> >  2 files changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/sunrpc/socklib.c b/net/sunrpc/socklib.c
> > index d8d8842c7de5c..ba18387f8ed6d 100644
> > --- a/net/sunrpc/socklib.c
> > +++ b/net/sunrpc/socklib.c
> > @@ -173,11 +173,17 @@ static int xprt_send_kvec(struct socket *sock, 
> > struct msghdr *msg,
> >  }
> > 
> >  static int xprt_send_pagedata(struct socket *sock, struct msghdr *msg,
> > -			      struct xdr_buf *xdr, size_t base)
> > +			      struct xdr_buf *xdr, size_t base,
> > +			      unsigned int splice)
> >  {
> > +	int ret;
> > +
> >  	iov_iter_bvec(&msg->msg_iter, ITER_SOURCE, xdr->bvec, xdr_buf_pagecount(xdr),
> >  		      xdr->page_len + xdr->page_base);
> > -	return xprt_sendmsg(sock, msg, base + xdr->page_base);
> > +	msg->msg_flags |= splice;
> > +	ret = xprt_sendmsg(sock, msg, base + xdr->page_base);
> > +	msg->msg_flags &= ~MSG_SPLICE_PAGES;
> > +	return ret;
> >  }
> > 
> >  /* Common case:
> > @@ -207,6 +213,12 @@ static int xprt_send_rm_and_kvec(struct socket 
> > *sock, struct msghdr *msg,
> >   * @sock: open socket to send on
> >   * @msg: socket message metadata
> >   * @xdr: xdr_buf containing this request
> > + *
> > + * With %MSG_SPLICE_PAGES set in @msg->msg_flags, the pages of @xdr are
> > + * spliced into the socket rather than copied, and the caller must 
> > leave
> > + * them unchanged until the socket has sent them.  The head and the 
> > tail
> > + * are always copied.
> > + *
> 
> What an odd place to put this comment. I honestly don't think it's needed,
> however. 
> 
> >   * @base: starting position in the buffer
> >   * @marker: stream record marker field
> >   * @sent_p: return the total number of bytes successfully queued for 
> > sending
> > @@ -221,6 +233,7 @@ int xprt_sock_sendmsg(struct socket *sock, struct 
> > msghdr *msg,
> >  {
> >  	unsigned int rmsize = marker ? sizeof(marker) : 0;
> >  	unsigned int remainder = rmsize + xdr->len - base;
> > +	unsigned int splice = msg->msg_flags & MSG_SPLICE_PAGES;
> >  	unsigned int want;
> >  	int err = 0;
> > 
> > @@ -229,6 +242,10 @@ int xprt_sock_sendmsg(struct socket *sock, struct 
> > msghdr *msg,
> >  	if (unlikely(!sock))
> >  		return -ENOTSOCK;
> > 
> > +	/* The head and tail are kvecs over memory the socket may not
> > +	 * hold a reference to, so only the page array is spliced.
> > +	 */
> 
> Same here. Code should be self-documenting enough that comments should
> be saved to explain tricky decisions and not describe basic behavior.
> 
> > +	msg->msg_flags &= ~MSG_SPLICE_PAGES;
> >  	msg->msg_flags |= MSG_MORE;
> >  	want = xdr->head[0].iov_len + rmsize;
> >  	if (base < want) {
> > @@ -256,7 +273,7 @@ int xprt_sock_sendmsg(struct socket *sock, struct 
> > msghdr *msg,
> >  		remainder -= len;
> >  		if (remainder == 0)
> >  			msg->msg_flags &= ~MSG_MORE;
> > -		err = xprt_send_pagedata(sock, msg, xdr, base);
> > +		err = xprt_send_pagedata(sock, msg, xdr, base, splice);
> >  		if (remainder == 0 || err != len)
> >  			goto out;
> >  		*sent_p += err;
> > @@ -266,10 +283,11 @@ int xprt_sock_sendmsg(struct socket *sock, struct 
> > msghdr *msg,
> >  	}
> > 
> >  	if (base >= xdr->tail[0].iov_len)
> > -		return 0;
> > +		goto out;
> >  	msg->msg_flags &= ~MSG_MORE;
> >  	err = xprt_send_kvec(sock, msg, &xdr->tail[0], base);
> >  out:
> > +	msg->msg_flags |= splice;
> >  	if (err > 0) {
> >  		*sent_p += err;
> >  		err = 0;
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index 173e3b0fd7a3b..2a077cddcc8ad 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -1136,6 +1136,15 @@ static int xs_tcp_send_request(struct rpc_rqst *req)
> > 
> >  	xs_set_srcport(transport, transport->sock);
> > 
> > +	/* Splice the pages of a first transmission into the socket: TCP
> > +	 * delivers them before the server can reply, and the reply is
> 
> Does "TCP delivers them before the server can reply" really need to be stated?
> 
> Anyway, what do you think is the right path forward for this? Chuck's patches
> seem to replace your 1/2, so should I just put this one on top of those?

Chuck's replacement for my 1/2 is basically identical, so yes that's
fine.

Sorry for the excessive (and weirdly placed) comments.  Feel free to
nuke them, or I can clean them up and send v2.

Mike