[PATCH 0/3] Minor fixes for svcrdma

[PATCH 0/3] Minor fixes for svcrdma

[Chuck Lever]

Rediscovered these while cleaning out old development branches.
All minor fixes.

Chuck Lever (3):
  svcrdma: Grant credits from the clamped sc_max_requests
  svcrdma: Clear XPT_DATA when the last receive context is consumed
  SUNRPC: Skip xpt_reserved accounting for non-UDP transports

 include/linux/sunrpc/svc_xprt.h          |  5 +++-
 net/sunrpc/svc_xprt.c                    | 30 +++++++++++++++---------
 net/sunrpc/svcsock.c                     |  1 +
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  | 18 ++++++++++----
 net/sunrpc/xprtrdma/svc_rdma_transport.c |  3 ++-
 5 files changed, 40 insertions(+), 17 deletions(-)

-- 
2.54.0

[PATCH 1/3] svcrdma: Grant credits from the clamped sc_max_requests

[Chuck Lever]

svc_rdma_accept() computes sc_fc_credits from sc_max_requests before
the Receive Queue depth is checked against the device's max_qp_wr.
When that check lowers sc_max_requests, the credit grant keeps the
original value, so the server advertises more credits than it has
Receives posted. A client that uses the full grant overruns the
Receive Queue, and the connection is lost with an RNR error.

Set sc_fc_credits after the clamp so the grant matches the number
of Receives the server posts.

Fixes: fc2e69db82c1 ("svcrdma: Clean up comment in svc_rdma_accept()")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/sunrpc/xprtrdma/svc_rdma_transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 927269598ac2..f949601b2144 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -471,7 +471,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 	newxprt->sc_max_requests = svcrdma_max_requests;
 	newxprt->sc_max_bc_requests = svcrdma_max_bc_requests;
 	newxprt->sc_recv_batch = RPCRDMA_MAX_RECV_BATCH;
-	newxprt->sc_fc_credits = cpu_to_be32(newxprt->sc_max_requests);
 
 	/* Qualify the transport's resource defaults with the
 	 * capabilities of this particular device.
@@ -492,6 +491,8 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 		newxprt->sc_max_bc_requests = 2;
 	}
 
+	newxprt->sc_fc_credits = cpu_to_be32(newxprt->sc_max_requests);
+
 	/* Estimate the needed number of rdma_rw contexts. The maximum
 	 * Read and Write chunks have one segment each. Each request
 	 * can involve one Read chunk and either a Write chunk or Reply
-- 
2.54.0

[PATCH 2/3] svcrdma: Clear XPT_DATA when the last receive context is consumed

[Chuck Lever]

svc_rdma_wc_receive() and svc_rdma_wc_read_done() set XPT_DATA after
adding a completed context to sc_rq_dto_q or sc_read_complete_q.
svc_rdma_recvfrom() dequeues one context and leaves XPT_DATA set, so
the svc_xprt_received() that follows re-enqueues the transport and
svc_xprt_enqueue() dispatches a second thread. That thread finds both
queues empty and returns zero.

Recheck the receive queues after each dequeue and clear XPT_DATA when
the last context is taken, rather than only when a dequeue finds
nothing. svc_xprt_received()'s kernel-doc no longer describes every
transport, so relax its note about when XPT_DATA is cleared.

Measured on one NFSv4.2 connection over 100GbE RoCE. A 4KB random read
at queue depth 1 falls from 2.997 transport dequeues per RPC to 1.998,
and from 96,629 to 82,398 server cycles per RPC. A 256KB random write
falls from 3.270 dequeues to 2.004, and from 259,936 to 246,059 cycles.
Each dispatch removed is worth about 10,000 cycles.

The gain shrinks as the receive queues fill, since a leftover XPT_DATA
then dispatches a thread that finds real work. An 8KB random write at
queue depth 512 already runs at the two dequeues an RPC with a Read
chunk requires, and shows no change. Throughput moves only where the
server has no idle CPU to absorb the saving, so only the queue depth 1
read gains, by 1.8%.

One dispatch per RPC remains. svc_rdma_send_ctxt_put() sets XPT_DATA to
schedule a drain of sc_send_release_list, and svc_rdma_recvfrom() does
not service that list.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/sunrpc/svc_xprt.c                   |  5 +++--
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 18 ++++++++++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 40040af588fb..c0e6772c6683 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -218,8 +218,9 @@ EXPORT_SYMBOL_GPL(svc_xprt_init);
  * The caller must hold the XPT_BUSY bit and must
  * not thereafter touch transport data.
  *
- * Note: XPT_DATA only gets cleared when a read-attempt finds no (or
- * insufficient) data.
+ * Note: xpo_recvfrom decides when to clear XPT_DATA. A transport may
+ * leave the bit set until a read attempt finds no (or insufficient)
+ * data, or clear it as soon as it consumes the last queued receive.
  */
 void svc_xprt_received(struct svc_xprt *xprt)
 {
diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
index fdfed1be97da..d029bcb7a5c0 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
@@ -925,8 +925,9 @@ static noinline void svc_rdma_read_complete(struct svc_rqst *rqstp,
  *	%-ENOTCONN if posting failed (connection is lost),
  *	%-EIO if rdma_rw initialization failed (DMA mapping, etc).
  *
- * Called in a loop when XPT_DATA is set. XPT_DATA is cleared only
- * when there are no remaining ctxt's to process.
+ * Called in a loop when XPT_DATA is set. XPT_DATA is cleared as
+ * soon as both receive queues are empty, so a consumed ctxt does
+ * not leave a stale bit behind.
  *
  * The next ctxt is removed from the "receive" lists.
  *
@@ -960,6 +961,15 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 	ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_read_complete_q);
 	if (ctxt) {
 		list_del(&ctxt->rc_list);
+		/* Producers add to these queues and set XPT_DATA under
+		 * this lock, so the clear cannot race one. The clear can
+		 * drop the XPT_DATA that svc_rdma_send_ctxt_put() sets
+		 * for sc_send_release_list. svc_xprt_release() drains
+		 * that list before this thread looks for more work.
+		 */
+		if (list_empty(&rdma_xprt->sc_read_complete_q) &&
+		    list_empty(&rdma_xprt->sc_rq_dto_q))
+			clear_bit(XPT_DATA, &xprt->xpt_flags);
 		spin_unlock(&rdma_xprt->sc_rq_dto_lock);
 		svc_xprt_received(xprt);
 		svc_rdma_read_complete(rqstp, ctxt);
@@ -968,8 +978,8 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 	ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_rq_dto_q);
 	if (ctxt)
 		list_del(&ctxt->rc_list);
-	else
-		/* No new incoming requests, terminate the loop */
+	/* sc_read_complete_q was empty above, under this same lock. */
+	if (list_empty(&rdma_xprt->sc_rq_dto_q))
 		clear_bit(XPT_DATA, &xprt->xpt_flags);
 	spin_unlock(&rdma_xprt->sc_rq_dto_lock);
 
-- 
2.54.0

[PATCH 3/3] SUNRPC: Skip xpt_reserved accounting for non-UDP transports

[Chuck Lever]

The xpt_reserved counter exists for UDP socket-buffer back-pressure.
svc_udp_has_wspace() is the only has_wspace implementation that
consults it, so on TCP and RDMA the counter is maintained and never
read. svc_handle_xprt() adds to it once per RPC. svc_reserve()
shrinks it again on each call from svc_process_common(), from
svc_xprt_release(), and from each proc function that calls
svc_reserve_auth(). Every shrinking call also runs
svc_xprt_resource_released(), which issues an smp_mb() and can
enqueue the transport.

Add an xcl_flags field to svc_xprt_class and set
SVC_XPRT_FLAG_WSPACE_RESERVE on the UDP class. Gate the xpt_reserved
accounting on that flag.

After the change, svc_reserve() no longer calls
svc_xprt_resource_released() on TCP and RDMA. Two paths still cover
that enqueue. svc_xprt_release() reaches the helper through
svc_xprt_release_slot(), and svc_xprt_received() enqueues a transport
whose XPT_DATA remains set.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 include/linux/sunrpc/svc_xprt.h |  5 ++++-
 net/sunrpc/svc_xprt.c           | 25 ++++++++++++++++---------
 net/sunrpc/svcsock.c            |  1 +
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index da2a2531e110..2af222f3ea2c 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -37,6 +37,9 @@ struct svc_xprt_class {
 	struct list_head	xcl_list;
 	u32			xcl_max_payload;
 	int			xcl_ident;
+	u32			xcl_flags;
+/* Set only on classes whose xpo_has_wspace() reads xpt_reserved */
+#define SVC_XPRT_FLAG_WSPACE_RESERVE	BIT(0)
 };
 
 /*
@@ -59,7 +62,7 @@ struct svc_xprt {
 	unsigned long		xpt_flags;
 
 	struct svc_serv		*xpt_server;	/* service for transport */
-	atomic_t    	    	xpt_reserved;	/* space on outq that is rsvd */
+	atomic_t		xpt_reserved;	/* outq space rsvd, UDP only */
 	atomic_t		xpt_nr_rqsts;	/* Number of requests */
 	struct mutex		xpt_mutex;	/* to serialize sending data */
 	spinlock_t		xpt_lock;	/* protects sk_deferred
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index c0e6772c6683..6e96e9b93071 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -476,11 +476,11 @@ static bool svc_xprt_ready(struct svc_xprt *xprt)
 
 	/*
 	 * If another cpu has recently updated xpt_flags,
-	 * sk_sock->flags, xpt_reserved, or xpt_nr_rqsts, we need to
-	 * know about it; otherwise it's possible that both that cpu and
-	 * this one could call svc_xprt_enqueue() without either
-	 * svc_xprt_enqueue() recognizing that the conditions below
-	 * are satisfied, and we could stall indefinitely:
+	 * sk_sock->flags, xpt_reserved (UDP only), or xpt_nr_rqsts,
+	 * we need to know about it; otherwise it's possible that both
+	 * that cpu and this one could call svc_xprt_enqueue() without
+	 * either svc_xprt_enqueue() recognizing that the conditions
+	 * below are satisfied, and we could stall indefinitely:
 	 */
 	smp_rmb();
 	xpt_flags = READ_ONCE(xprt->xpt_flags);
@@ -552,6 +552,10 @@ static struct svc_xprt *svc_xprt_dequeue(struct svc_pool *pool)
  * to make sure the reply fits.  This function reduces that reserved
  * space to be the amount of space used already, plus @space.
  *
+ * The transport's reservation is tracked only on classes that set
+ * SVC_XPRT_FLAG_WSPACE_RESERVE.  On the others, only @rqstp's
+ * reservation is updated.
+ *
  */
 void svc_reserve(struct svc_rqst *rqstp, int space)
 {
@@ -560,10 +564,12 @@ void svc_reserve(struct svc_rqst *rqstp, int space)
 	space += rqstp->rq_res.head[0].iov_len;
 
 	if (xprt && space < rqstp->rq_reserved) {
-		atomic_sub((rqstp->rq_reserved - space),
-			   &xprt->xpt_reserved);
+		if (xprt->xpt_class->xcl_flags & SVC_XPRT_FLAG_WSPACE_RESERVE) {
+			atomic_sub((rqstp->rq_reserved - space),
+				   &xprt->xpt_reserved);
+			svc_xprt_resource_released(xprt);
+		}
 		rqstp->rq_reserved = space;
-		svc_xprt_resource_released(xprt);
 	}
 }
 EXPORT_SYMBOL_GPL(svc_reserve);
@@ -870,7 +876,8 @@ static void svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)
 		else
 			len = xprt->xpt_ops->xpo_recvfrom(rqstp);
 		rqstp->rq_reserved = serv->sv_max_mesg;
-		atomic_add(rqstp->rq_reserved, &xprt->xpt_reserved);
+		if (xprt->xpt_class->xcl_flags & SVC_XPRT_FLAG_WSPACE_RESERVE)
+			atomic_add(rqstp->rq_reserved, &xprt->xpt_reserved);
 		if (len <= 0)
 			goto out;
 
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index ef7ac080fcd3..e5459d504b6a 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -800,6 +800,7 @@ static struct svc_xprt_class svc_udp_class = {
 	.xcl_ops = &svc_udp_ops,
 	.xcl_max_payload = RPCSVC_MAXPAYLOAD_UDP,
 	.xcl_ident = XPRT_TRANSPORT_UDP,
+	.xcl_flags = SVC_XPRT_FLAG_WSPACE_RESERVE,
 };
 
 static void svc_udp_init(struct svc_sock *svsk, struct svc_serv *serv)
-- 
2.54.0