From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Tucker Subject: Re: [RFC,PATCH 21/38] svc: Change svc_sock_received to svc_xprt_received and export it Date: Fri, 30 Nov 2007 17:17:14 -0600 Message-ID: <1196464634.5432.68.camel@trinity.ogc.int> References: <20071129223917.14563.77633.stgit@dell3.ogc.int> <20071129224037.14563.69171.stgit@dell3.ogc.int> <7E88BE3B-F35C-44B1-AE84-E5DE62E4EFA5@oracle.com> Mime-Version: 1.0 Content-Type: text/plain Cc: "J. Bruce Fields" , linux-nfs@vger.kernel.org To: Chuck Lever Return-path: Received: from 209-198-142-2-host.prismnet.net ([209.198.142.2]:60659 "EHLO smtp.opengridcomputing.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754489AbXK3XNJ (ORCPT ); Fri, 30 Nov 2007 18:13:09 -0500 In-Reply-To: <7E88BE3B-F35C-44B1-AE84-E5DE62E4EFA5@oracle.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: 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 > > --- > > > > 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