Re: [RFC,PATCH 21/38] svc: Change svc_sock_received to svc_xprt_received and export it

[Tom Tucker <tom@opengridcomputing.com>]

On Fri, 2007-11-30 at 16:33 -0500, Chuck Lever wrote:
> On Nov 29, 2007, at 5:40 PM, Tom Tucker wrote:
> > All fields touched by svc_sock_received are now transport independent.
> > Change it to use svc_xprt directly. This function is called from
> > transport dependent code, so export it.
> >
> > Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> > ---
> >
> >  include/linux/sunrpc/svc_xprt.h |    2 +-
> >  net/sunrpc/svcsock.c            |   37 +++++++++++++++++ 
> > +-------------------
> >  2 files changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/ 
> > svc_xprt.h
> > index d5ef902..c416d05 100644
> > --- a/include/linux/sunrpc/svc_xprt.h
> > +++ b/include/linux/sunrpc/svc_xprt.h
> > @@ -62,8 +62,8 @@ int	svc_unreg_xprt_class(struct svc_xprt_class *);
> >  void	svc_xprt_init(struct svc_xprt_class *, struct svc_xprt *,
> >  		      struct svc_serv *);
> >  int	svc_create_xprt(struct svc_serv *, char *, unsigned short, int);
> > +void	svc_xprt_received(struct svc_xprt *);
> >  void	svc_xprt_put(struct svc_xprt *xprt);
> > -
> >  static inline void svc_xprt_get(struct svc_xprt *xprt)
> >  {
> >  	kref_get(&xprt->xpt_ref);
> > diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> > index 5666541..0015839 100644
> > --- a/net/sunrpc/svcsock.c
> > +++ b/net/sunrpc/svcsock.c
> > @@ -347,14 +347,14 @@ svc_sock_dequeue(struct svc_pool *pool)
> >   * Note: XPT_DATA only gets cleared when a read-attempt finds
> >   * no (or insufficient) data.
> >   */
> > -static inline void
> > -svc_sock_received(struct svc_sock *svsk)
> > +void
> > +svc_xprt_received(struct svc_xprt *xprt)
> 
> Style police again.  I notice several of these patches add new  
> functions with the return value split onto a separate line.

The policy I used was if I didn't change the function signature, I left
it like it was. If I copied it to svc_xprt, I fixed the formatting of
the signature to conform. I didn't do that for this one. I'll check for
others.

> 
> >  {
> > -	svsk->sk_xprt.xpt_pool = NULL;
> > -	clear_bit(XPT_BUSY, &svsk->sk_xprt.xpt_flags);
> > -	svc_xprt_enqueue(&svsk->sk_xprt);
> > +	xprt->xpt_pool = NULL;
> > +	clear_bit(XPT_BUSY, &xprt->xpt_flags);
> > +	svc_xprt_enqueue(xprt);
> >  }
> > -
> > +EXPORT_SYMBOL_GPL(svc_xprt_received);
> 
> When I submitted the RPC client-side transport switch, Trond  
> suggested we add the EXPORTs later when it was clear why they are  
> needed.  This may be a personal preference of the server maintainer,  
> but I just thought I'd mention the possibility; it seems to make  
> sense here too.

Sure, but we already have a server side provider that helps accelerate
the proving process. The svcrdma module won't build without this being
exported.

> 
> >  /**
> >   * svc_reserve - change the space reserved for the reply to a  
> > request.
> > @@ -783,7 +783,7 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
> >  				(serv->sv_nrthreads+3) * serv->sv_max_mesg);
> >
> >  	if ((rqstp->rq_deferred = svc_deferred_dequeue(svsk))) {
> > -		svc_sock_received(svsk);
> > +		svc_xprt_received(&svsk->sk_xprt);
> >  		return svc_deferred_recv(rqstp);
> >  	}
> >
> > @@ -800,7 +800,7 @@ 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_sock_received(svsk);
> > +		svc_xprt_received(&svsk->sk_xprt);
> >  		return -EAGAIN;
> >  	}
> >  	rqstp->rq_addrlen = sizeof(rqstp->rq_addr);
> > @@ -815,7 +815,7 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
> >  	/*
> >  	 * Maybe more packets - kick another thread ASAP.
> >  	 */
> > -	svc_sock_received(svsk);
> > +	svc_xprt_received(&svsk->sk_xprt);
> >
> >  	len  = skb->len - sizeof(struct udphdr);
> >  	rqstp->rq_arg.len = len;
> > @@ -1123,8 +1123,6 @@ static struct svc_xprt *svc_tcp_accept(struct  
> > svc_xprt *xprt)
> >  	}
> >  	memcpy(&newsvsk->sk_local, sin, slen);
> >
> > -	svc_sock_received(newsvsk);
> > -
> 
> I assume it's OK to remove svc_sock_received() here (rather than  
> replacing it with svc_xprt_received()) because you are adding a call  
> to xvs_xprt_received() below in svc_recv().
> 
> I think this is a non-trivial change amongst a whole bunch of trivial  
> ones in this patch.  Thus it would be nicer if we did this in a  
> separate patch so you can document your rationale for this change.   
> (Yeah, I think we went over this in e-mail some time ago, but still...)
> 

Yeah, this is probably a good idea. I toyed with removing
svc_xprt_received from the provider all-together and putting it in the
common logic which would remove 10s of calls to svc_xprt_received and
avoid potential races caused by calling it without the BUSY bit held.
The reason I didn't do it was because it means that the xpo_receive
function must complete before queuing the transport back for more I/O,
and this is suboptimal from an MP perspective since some transports can
release the transport early (e.g. RDMA) while it parses the transport
header and does other business.

> >  	if (serv->sv_stats)
> >  		serv->sv_stats->nettcpconn++;
> >
> > @@ -1153,7 +1151,7 @@ svc_tcp_recvfrom(struct svc_rqst *rqstp)
> >  		test_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags));
> >
> >  	if ((rqstp->rq_deferred = svc_deferred_dequeue(svsk))) {
> > -		svc_sock_received(svsk);
> > +		svc_xprt_received(&svsk->sk_xprt);
> >  		return svc_deferred_recv(rqstp);
> >  	}
> >
> > @@ -1193,7 +1191,7 @@ svc_tcp_recvfrom(struct svc_rqst *rqstp)
> >  		if (len < want) {
> >  			dprintk("svc: short recvfrom while reading record length (%d of  
> > %lu)\n",
> >  				len, want);
> > -			svc_sock_received(svsk);
> > +			svc_xprt_received(&svsk->sk_xprt);
> >  			return -EAGAIN; /* record header not complete */
> >  		}
> >
> > @@ -1229,7 +1227,7 @@ svc_tcp_recvfrom(struct svc_rqst *rqstp)
> >  	if (len < svsk->sk_reclen) {
> >  		dprintk("svc: incomplete TCP record (%d of %d)\n",
> >  			len, svsk->sk_reclen);
> > -		svc_sock_received(svsk);
> > +		svc_xprt_received(&svsk->sk_xprt);
> >  		return -EAGAIN;	/* record not complete */
> >  	}
> >  	len = svsk->sk_reclen;
> > @@ -1269,7 +1267,7 @@ svc_tcp_recvfrom(struct svc_rqst *rqstp)
> >  	svsk->sk_reclen = 0;
> >  	svsk->sk_tcplen = 0;
> >
> > -	svc_sock_received(svsk);
> > +	svc_xprt_received(&svsk->sk_xprt);
> >  	if (serv->sv_stats)
> >  		serv->sv_stats->nettcpcnt++;
> >
> > @@ -1282,7 +1280,7 @@ svc_tcp_recvfrom(struct svc_rqst *rqstp)
> >   error:
> >  	if (len == -EAGAIN) {
> >  		dprintk("RPC: TCP recvfrom got EAGAIN\n");
> > -		svc_sock_received(svsk);
> > +		svc_xprt_received(&svsk->sk_xprt);
> >  	} else {
> >  		printk(KERN_NOTICE "%s: recvfrom returned errno %d\n",
> >  		       svsk->sk_xprt.xpt_server->sv_name, -len);
> > @@ -1607,8 +1605,9 @@ svc_recv(struct svc_rqst *rqstp, long timeout)
> >  			 */
> >  			__module_get(newxpt->xpt_class->xcl_owner);
> >  			svc_check_conn_limits(svsk->sk_xprt.xpt_server);
> > +			svc_xprt_received(newxpt);
> >  		}
> > -		svc_sock_received(svsk);
> > +		svc_xprt_received(&svsk->sk_xprt);
> >  	} else {
> >  		dprintk("svc: server %p, pool %u, socket %p, inuse=%d\n",
> >  			rqstp, pool->sp_id, svsk,
> > @@ -1827,7 +1826,7 @@ int svc_addsock(struct svc_serv *serv,
> >  	else {
> >  		svsk = svc_setup_socket(serv, so, &err, SVC_SOCK_DEFAULTS);
> >  		if (svsk) {
> > -			svc_sock_received(svsk);
> > +			svc_xprt_received(&svsk->sk_xprt);
> >  			err = 0;
> >  		}
> >  	}
> > @@ -1882,7 +1881,7 @@ svc_create_socket(struct svc_serv *serv, int  
> > protocol,
> >  	}
> >
> >  	if ((svsk = svc_setup_socket(serv, sock, &error, flags)) != NULL) {
> > -		svc_sock_received(svsk);
> > +		svc_xprt_received(&svsk->sk_xprt);
> >  		return (struct svc_xprt *)svsk;
> >  	}
> >
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> -
> 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