Re: [PATCH 1/3] svcrdma: Remove redunant call to xpo_release_rqst method.

Re: [PATCH 1/3] svcrdma: Remove redunant call to xpo_release_rqst method.

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:18:04AM -0500, Tom Tucker wrote:
> The xpo_release_rqst method is called from svc_xprt_release and 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_rqst function 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.

> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> 
> ---
>  net/sunrpc/svc_xprt.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index d8e8d79..74e52d4 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -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 1/3] svcrdma: Remove redunant call to xpo_release_rqst method.

[Tom Tucker]

On Tue, 2008-05-06 at 17:59 -0400, J. Bruce Fields wrote:
> On Fri, May 02, 2008 at 11:18:04AM -0500, Tom Tucker wrote:
> > The xpo_release_rqst method is called from svc_xprt_release and 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_rqst function 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. 

> 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?

> 
> --b.
> 
> > 
> > Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> > 
> > ---
> >  net/sunrpc/svc_xprt.c |    3 ---
> >  1 files changed, 0 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index d8e8d79..74e52d4 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] svcrdma: Remove redunant call to xpo_release_rqst method.

[J. Bruce Fields]

On Wed, May 07, 2008 at 10:08:06AM -0500, Tom Tucker wrote:
> 
> On Tue, 2008-05-06 at 17:59 -0400, J. Bruce Fields wrote:
> > On Fri, May 02, 2008 at 11:18:04AM -0500, Tom Tucker wrote:
> > > The xpo_release_rqst method is called from svc_xprt_release and 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_rqst function 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. 
> 
> > 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?

Yep, sounds prudent.

--b.

Re: [PATCH 2/3] svcrdma: Remove extra check for XPT_DEAD bit in svc_xprt_enqueue

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:18:05AM -0500, Tom Tucker wrote:
> Remove a redundant check for the XPT_DEAD bit in the svc_xprt_enqueue
> function. This same bit is checked below while holding the pool lock

Yeah, seems pointless.  I wonder how it got there.

--b.

> and prints a debug message if found to be dead.

> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> 
> ---
>  net/sunrpc/svc_xprt.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 74e52d4..fb8a785 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -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);