svcrdma: Remove redunant call to xpo_release_rqst method.
Subsystems (2)
Quoted 10 lines · Tom Tucker
The
xpo_release_rqstmethod is called fromsvc_xprt_releaseand therefore this direct call of the provider method is redundant. There is no bug here since the provider protects against multiple calls.Originally, this code was here because the
xpo_release_rqstfunction was being used by the RDMA transport to return credits to the client, i.e. posting a receive buffer. This had to be done before the send in order to avoid a race wherein the client responds immediately with a new request but the buffer has not yet been posted. This is a poor design point and the recv buffer posting has been modified in the rdma transport.
The comment there refers to the socket code, not the rdma code, and there was a svc_release_skb() there before the rdma code came along. I'd like to understand why.
--b.
Quoted 21 lines
Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/svc_xprt.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-)net/sunrpc/svc_xprt.c +0
-3@@ -751,9 +751,6 @@ int svc_send(struct svc_rqst *rqstp)if (!xprt)return -EFAULT ; -/* release the receive skb before sending the reply */ - rqstp->rq_xprt->xpt_ops->xpo_release_rqst (rqstp); -/* calculate over-all length */ xb = &rqstp->rq_res; xb->len = xb->head[0].iov_len +
Re: [PATCH 2/3] svcrdma: Remove extra check for XPT_DEAD bit in svc_xprt_enqueue
In reply to a message not in the archiveQuoted 2 lines · Tom Tucker
Remove a redundant check for the
XPT_DEADbit in thesvc_xprt_enqueuefunction. This same bit is checked below while holding the pool lock
Yeah, seems pointless. I wonder how it got there.
--b.
Quoted 22 lines
and prints a debug message if found to be dead.
Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/svc_xprt.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)net/sunrpc/svc_xprt.c +0
-2@@ -296,8 +296,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)if (!(xprt->xpt_flags & ((1 <<XPT_CONN )|(1 <<XPT_DATA )|(1 <<XPT_CLOSE )|(1 <<XPT_DEFERRED ))))return ; -if (test_bit (XPT_DEAD , &xprt->xpt_flags)) -return ; cpu =get_cpu (); pool =svc_pool_for_cpu (xprt->xpt_server, cpu);
Quoted 5 lines · J. Bruce Fields
Quoted 4 lines · Tom Tucker
The
xpo_release_rqstmethod is called fromsvc_xprt_releaseand therefore this direct call of the provider method is redundant. There is no bug here since the provider protects against multiple calls.
Quoted 8 lines
Quoted 6 lines
Originally, this code was here because the
xpo_release_rqstfunction was being used by the RDMA transport to return credits to the client, i.e. posting a receive buffer. This had to be done before the send in order to avoid a race wherein the client responds immediately with a new request but the buffer has not yet been posted. This is a poor design point and the recv buffer posting has been modified in the rdma transport.The comment there refers to the socket code, not the rdma code, and
Yes, you are correct.
Quoted 2 lines
there was a
svc_release_skb()there before the rdma code came along. I'd like to understand why.
I believe it's because the socket code has buffer quotas and the skb you're holding consumes credits in the recv sock buf. So it's conceivable that you could send a message, another client message arrives prior to releasing the skb your holding, and this new message gets dropped unnecessarily. I don't believe it affects the sending at all because the message you're holding came from sock_recv_datagram and only consumes recv side credits.
Since this patch is only for "clean up" purposes, I'm inclined to just ditch this patch and let us noodle on it a little more. Agreed?
Quoted 28 lines
--b.
Quoted 12 lines
Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/svc_xprt.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-)net/sunrpc/svc_xprt.c +0
-0net/sunrpc/svc_xprt.c int svc_send(struct svc_rqst *rqstp) · line 756
Quoted 9 lines
net/sunrpc/svc_xprt.c +0
-3@@ -751,9 +751,6 @@ int svc_send(struct svc_rqst *rqstp)if (!xprt)return -EFAULT ; -/* release the receive skb before sending the reply */ - rqstp->rq_xprt->xpt_ops->xpo_release_rqst (rqstp); -/* calculate over-all length */ xb = &rqstp->rq_res; xb->len = xb->head[0].iov_len +--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 29 lines · Tom Tucker
Quoted 5 lines · J. Bruce Fields
Quoted 4 lines · Tom Tucker
The
xpo_release_rqstmethod is called fromsvc_xprt_releaseand therefore this direct call of the provider method is redundant. There is no bug here since the provider protects against multiple calls.Quoted 8 lines
Quoted 6 lines
Originally, this code was here because the
xpo_release_rqstfunction was being used by the RDMA transport to return credits to the client, i.e. posting a receive buffer. This had to be done before the send in order to avoid a race wherein the client responds immediately with a new request but the buffer has not yet been posted. This is a poor design point and the recv buffer posting has been modified in the rdma transport.The comment there refers to the socket code, not the rdma code, and
Yes, you are correct.
Quoted 2 lines
there was a
svc_release_skb()there before the rdma code came along. I'd like to understand why.I believe it's because the socket code has buffer quotas and the skb you're holding consumes credits in the recv sock buf. So it's conceivable that you could send a message, another client message arrives prior to releasing the skb your holding, and this new message gets dropped unnecessarily. I don't believe it affects the sending at all because the message you're holding came from
sock_recv_datagramand only consumes recv side credits.
Quoted 2 lines
Since this patch is only for "clean up" purposes, I'm inclined to just ditch this patch and let us noodle on it a little more. Agreed?
Yep, sounds prudent.
--b.