From: Chuck Lever <chuck.lever@oracle.com>
Subject: Re: [RFC,
	PATCH 05/35] svc: Move sk_sendto and sk_recvfrom to svc_xprt_class
Date: Tue, 2 Oct 2007 12:57:49 -0400
Message-ID: <4A775179-9659-41B6-999F-8316BA181152@oracle.com>
References: <20071001191426.3250.15371.stgit@dell3.ogc.int>
	<20071001192740.3250.73564.stgit@dell3.ogc.int>
	<CBDFC3B7-FE67-4715-87CA-E828D76844BB@oracle.com>
	<1191342596.1565.11.camel@trinity.ogc.int>
Mime-Version: 1.0 (Apple Message framework v752.2)
Content-Type: text/plain; charset="us-ascii"
Cc: neilb@suse.de, bfields@fieldses.org, nfs@lists.sourceforge.net,
	gnb@sgi.com
To: tom@opengridcomputing.com
In-Reply-To: <1191342596.1565.11.camel@trinity.ogc.int>
Sender: nfs-bounces@lists.sourceforge.net
Errors-To: nfs-bounces@lists.sourceforge.net


On Oct 2, 2007, at 12:29 PM, Tom Tucker wrote:

> On Tue, 2007-10-02 at 11:04 -0400, Chuck Lever wrote:
>> On Oct 1, 2007, at 3:27 PM, Tom Tucker wrote:
>>>
>>> The sk_sendto and sk_recvfrom are function pointers that allow
>>> svc_sock
>>> to be used for both UDP and TCP. Move these function pointers to the
>>> svc_xprt_ops structure.
>>>
>>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>>> ---
>>>
>>>  include/linux/sunrpc/svc_xprt.h |    2 ++
>>>  include/linux/sunrpc/svcsock.h  |    3 ---
>>>  net/sunrpc/svcsock.c            |   12 ++++++------
>>>  3 files changed, 8 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/
>>> svc_xprt.h
>>> index 827f0fe..f0ba052 100644
>>> --- a/include/linux/sunrpc/svc_xprt.h
>>> +++ b/include/linux/sunrpc/svc_xprt.h
>>> @@ -10,6 +10,8 @@ #define SUNRPC_SVC_XPRT_H
>>>  #include <linux/sunrpc/svc.h>
>>>
>>>  struct svc_xprt_ops {
>>> +	int		(*xpo_recvfrom)(struct svc_rqst *);
>>> +	int		(*xpo_sendto)(struct svc_rqst *);
>>>  };
>>>
>>>  struct svc_xprt_class {
>>> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/
>>> svcsock.h
>>> index 1878cbe..08e78d0 100644
>>> --- a/include/linux/sunrpc/svcsock.h
>>> +++ b/include/linux/sunrpc/svcsock.h
>>> @@ -45,9 +45,6 @@ #define	SK_DETACHED	10			/* detached fro
>>>  						 * be revisted */
>>>  	struct mutex		sk_mutex;	/* to serialize sending data */
>>>
>>> -	int			(*sk_recvfrom)(struct svc_rqst *rqstp);
>>> -	int			(*sk_sendto)(struct svc_rqst *rqstp);
>>> -
>>>  	/* We keep the old state_change and data_ready CB's here */
>>>  	void			(*sk_ostate)(struct sock *);
>>>  	void			(*sk_odata)(struct sock *, int bytes);
>>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>>> index d84b5c8..150531f 100644
>>> --- a/net/sunrpc/svcsock.c
>>> +++ b/net/sunrpc/svcsock.c
>>> @@ -900,6 +900,8 @@ svc_udp_sendto(struct svc_rqst *rqstp)
>>>  }
>>>
>>>  static struct svc_xprt_ops svc_udp_ops = {
>>> +	.xpo_recvfrom = svc_udp_recvfrom,
>>> +	.xpo_sendto = svc_udp_sendto,
>>>  };
>>>
>>>  static struct svc_xprt_class svc_udp_class = {
>>> @@ -917,8 +919,6 @@ svc_udp_init(struct svc_sock *svsk)
>>>  	svc_xprt_init(&svc_udp_class, &svsk->sk_xprt);
>>>  	svsk->sk_sk->sk_data_ready = svc_udp_data_ready;
>>>  	svsk->sk_sk->sk_write_space = svc_write_space;
>>> -	svsk->sk_recvfrom = svc_udp_recvfrom;
>>> -	svsk->sk_sendto = svc_udp_sendto;
>>>
>>>  	/* initialise setting must have enough space to
>>>  	 * receive and respond to one request.
>>> @@ -1354,6 +1354,8 @@ svc_tcp_sendto(struct svc_rqst *rqstp)
>>>  }
>>>
>>>  static struct svc_xprt_ops svc_tcp_ops = {
>>> +	.xpo_recvfrom = svc_tcp_recvfrom,
>>> +	.xpo_sendto = svc_tcp_sendto,
>>>  };
>>>
>>>  static struct svc_xprt_class svc_tcp_class = {
>>> @@ -1381,8 +1383,6 @@ svc_tcp_init(struct svc_sock *svsk)
>>>  	struct tcp_sock *tp = tcp_sk(sk);
>>>
>>>  	svc_xprt_init(&svc_tcp_class, &svsk->sk_xprt);
>>> -	svsk->sk_recvfrom = svc_tcp_recvfrom;
>>> -	svsk->sk_sendto = svc_tcp_sendto;
>>>
>>>  	if (sk->sk_state == TCP_LISTEN) {
>>>  		dprintk("setting up TCP socket for listening\n");
>>> @@ -1530,7 +1530,7 @@ svc_recv(struct svc_rqst *rqstp, long ti
>>>
>>>  	dprintk("svc: server %p, pool %u, socket %p, inuse=%d\n",
>>>  		 rqstp, pool->sp_id, svsk, atomic_read(&svsk->sk_inuse));
>>> -	len = svsk->sk_recvfrom(rqstp);
>>> +	len = svsk->sk_xprt.xpt_ops.xpo_recvfrom(rqstp);
>>>  	dprintk("svc: got len=%d\n", len);
>>>
>>>  	/* No data, incomplete (TCP) read, or accept() */
>>> @@ -1590,7 +1590,7 @@ svc_send(struct svc_rqst *rqstp)
>>>  	if (test_bit(SK_DEAD, &svsk->sk_flags))
>>>  		len = -ENOTCONN;
>>>  	else
>>> -		len = svsk->sk_sendto(rqstp);
>>> +		len = svsk->sk_xprt.xpt_ops.xpo_sendto(rqstp);
>>>  	mutex_unlock(&svsk->sk_mutex);
>>>  	svc_sock_release(rqstp);
>>
>> Again, here you have copied a pointer from the class structure to the
>> instance structure -- the address of the transport ops structure
>> never changes during the lifetime of the xprt instance, does it?  You
>> could just as easily use the class's ops pointer instead.
>>
>> It looks like on the client side, I didn't put the ops vector or the
>> payload maximum in the class structure at all... 6 of one, half dozen
>> of the other.  Using the class's value of the ops and payload maximum
>> would save some space in the svc_xprt, though, come to think of it.
>>
>
> cache thing again. let's see how Greg weighs in.

The ops vector itself will be in some other CPU's memory most of the  
time on big systems.  I don't see how you can avoid a peek... but  
since it's a constant, caching should protect you most of the time, yes?

Chuck Lever
chuck.lever@oracle.com


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs