Re: The recent kref_put warning (was: [PATCH] sunrpc: remove unnecessary svc_xprt_put)

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Mar 01, 2010 at 04:51:14PM +1100, Neil Brown wrote:
> On Sun, 28 Feb 2010 22:46:47 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > I also agree with the cleanup that moves the svc_xprt_received to one
> > place, I'm just hoping you won't mind regenerating it against this.
> 
> See below.
> There is still room to tidy up svc_recv, including getting the xpo_recvfrom
> routines to report -EAGAIN when that is what they mean, rather than '0',
> but I'm not really happy with what I have so-far so I won't post it yet.

I've applied this for 2.6.35; apologies for the delay.

Did you get another chance to look the further cleanup you were
considering?

--b.

> 
> NeilBrown
> 
> 
> From 1e75b9d1232957cd44e0d8ea704c9af431cc85be Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.de>
> Date: Mon, 1 Mar 2010 15:49:11 +1100
> Subject: [PATCH] sunrpc: centralise most calls to svc_xprt_received
> 
> svc_xprt_received must be called when ->xpo_recvfrom has finished
> receiving a message, so that the XPT_BUSY flag will be cleared and
> if necessary, requeued for further work.
> 
> This call is currently made in each ->xpo_recvfrom function, often
> from multiple different point, in each case it is the earliest point
> on a particular path where it is known that the protection provided by
> XPT_BUSY is no longer needed.
> 
> However there are (still) some error paths which do not call
> svc_xprt_received, and requiring each ->xpo_recvfrom to make the call
> does not encourage robustness.
> 
> So: move the svc_xprt_received call to be made just after the
> call to ->xpo_recvfrom(), and move it of the various ->xpo_recvfrom
> methods.
> 
> This means that it may not be called at the earliest possible instant,
> but this is unlikely to be a measurable performance issue.
> 
> Note that there are still other calls to svc_xprt_received as it is
> also needed when an xprt is newly created.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 6bd41a9..70b74be 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -736,8 +736,10 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
>  		if (rqstp->rq_deferred) {
>  			svc_xprt_received(xprt);
>  			len = svc_deferred_recv(rqstp);
> -		} else
> +		} else {
>  			len = xprt->xpt_ops->xpo_recvfrom(rqstp);
> +			svc_xprt_received(xprt);
> +		}
>  		dprintk("svc: got len=%d\n", len);
>  	}
>  
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 528efef..7425029 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -547,7 +547,6 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>  			dprintk("svc: recvfrom returned error %d\n", -err);
>  			set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
>  		}
> -		svc_xprt_received(&svsk->sk_xprt);
>  		return -EAGAIN;
>  	}
>  	len = svc_addr_len(svc_addr(rqstp));
> @@ -562,11 +561,6 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>  	svsk->sk_sk->sk_stamp = skb->tstamp;
>  	set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags); /* there may be more data... */
>  
> -	/*
> -	 * Maybe more packets - kick another thread ASAP.
> -	 */
> -	svc_xprt_received(&svsk->sk_xprt);
> -
>  	len  = skb->len - sizeof(struct udphdr);
>  	rqstp->rq_arg.len = len;
>  
> @@ -917,7 +911,6 @@ static int svc_tcp_recv_record(struct svc_sock *svsk, struct svc_rqst *rqstp)
>  		if (len < want) {
>  			dprintk("svc: short recvfrom while reading record "
>  				"length (%d of %d)\n", len, want);
> -			svc_xprt_received(&svsk->sk_xprt);
>  			goto err_again; /* record header not complete */
>  		}
>  
> @@ -953,7 +946,6 @@ static int svc_tcp_recv_record(struct svc_sock *svsk, struct svc_rqst *rqstp)
>  	if (len < svsk->sk_reclen) {
>  		dprintk("svc: incomplete TCP record (%d of %d)\n",
>  			len, svsk->sk_reclen);
> -		svc_xprt_received(&svsk->sk_xprt);
>  		goto err_again;	/* record not complete */
>  	}
>  	len = svsk->sk_reclen;
> @@ -961,14 +953,11 @@ static int svc_tcp_recv_record(struct svc_sock *svsk, struct svc_rqst *rqstp)
>  
>  	return len;
>   error:
> -	if (len == -EAGAIN) {
> +	if (len == -EAGAIN)
>  		dprintk("RPC: TCP recv_record got EAGAIN\n");
> -		svc_xprt_received(&svsk->sk_xprt);
> -	}
>  	return len;
>   err_delete:
>  	set_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags);
> -	svc_xprt_received(&svsk->sk_xprt);
>   err_again:
>  	return -EAGAIN;
>  }
> @@ -1110,7 +1099,6 @@ out:
>  	svsk->sk_tcplen = 0;
>  
>  	svc_xprt_copy_addrs(rqstp, &svsk->sk_xprt);
> -	svc_xprt_received(&svsk->sk_xprt);
>  	if (serv->sv_stats)
>  		serv->sv_stats->nettcpcnt++;
>  
> @@ -1119,7 +1107,6 @@ out:
>  err_again:
>  	if (len == -EAGAIN) {
>  		dprintk("RPC: TCP recvfrom got EAGAIN\n");
> -		svc_xprt_received(&svsk->sk_xprt);
>  		return len;
>  	}
>  error:
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index f92e37e..0194de8 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -566,7 +566,6 @@ static int rdma_read_complete(struct svc_rqst *rqstp,
>  		ret, rqstp->rq_arg.len,	rqstp->rq_arg.head[0].iov_base,
>  		rqstp->rq_arg.head[0].iov_len);
>  
> -	svc_xprt_received(rqstp->rq_xprt);
>  	return ret;
>  }
>  
> @@ -665,7 +664,6 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>  		rqstp->rq_arg.head[0].iov_len);
>  	rqstp->rq_prot = IPPROTO_MAX;
>  	svc_xprt_copy_addrs(rqstp, xprt);
> -	svc_xprt_received(xprt);
>  	return ret;
>  
>   close_out:
> @@ -678,6 +676,5 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>  	 */
>  	set_bit(XPT_CLOSE, &xprt->xpt_flags);
>  defer:
> -	svc_xprt_received(xprt);
>  	return 0;
>  }