Re: [RFC,PATCH 10/20] svc: Add generic refcount services

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 5272 bytes --]

Tom Tucker wrote:
> Add inline svc_sock_get() so that service transport code will not
> need to manipulate sk_inuse directly.  Also, make svc_sock_put()
> available so that transport code outside svcsock.c can use it.

If svc_sock_get/put is meant to be generic, then you should probably 
rename them, move them to a generic source file, and have them take 
something other than "svc_sock *".

Would it make sense to add a kref to these objects instead of 
manipulating a naked atomic_t reference count?

> Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> ---
> 
>  include/linux/sunrpc/svcsock.h |   15 +++++++++++++++
>  net/sunrpc/svcsock.c           |   29 ++++++++++++++---------------
>  2 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> index ea8b62b..9f37f30 100644
> --- a/include/linux/sunrpc/svcsock.h
> +++ b/include/linux/sunrpc/svcsock.h
> @@ -115,6 +115,7 @@ int		svc_addsock(struct svc_serv *serv,
>  			    int *proto);
>  void		svc_sock_enqueue(struct svc_sock *svsk);
>  void		svc_sock_received(struct svc_sock *svsk);
> +void	    	__svc_sock_put(struct svc_sock *svsk);
>  
>  /*
>   * svc_makesock socket characteristics
> @@ -123,4 +124,18 @@ #define SVC_SOCK_DEFAULTS	(0U)
>  #define SVC_SOCK_ANONYMOUS	(1U << 0)	/* don't register with pmap */
>  #define SVC_SOCK_TEMPORARY	(1U << 1)	/* flag socket as temporary */
>  
> +/*
> + * Take and drop a temporary reference count on the svc_sock.
> + */
> +static inline void svc_sock_get(struct svc_sock *svsk)
> +{
> +	atomic_inc(&svsk->sk_inuse);
> +}
> +
> +static inline void svc_sock_put(struct svc_sock *svsk)
> +{
> +	if (atomic_dec_and_test(&svsk->sk_inuse))
> +	    __svc_sock_put(svsk);
> +}
> +
>  #endif /* SUNRPC_SVCSOCK_H */
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index dcb5c7a..02f682a 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -273,7 +273,7 @@ svc_sock_enqueue(struct svc_sock *svsk)
>  				"svc_sock_enqueue: server %p, rq_sock=%p!\n",
>  				rqstp, rqstp->rq_sock);
>  		rqstp->rq_sock = svsk;
> -		atomic_inc(&svsk->sk_inuse);
> +		svc_sock_get(svsk);
>  		rqstp->rq_reserved = serv->sv_max_mesg;
>  		atomic_add(rqstp->rq_reserved, &svsk->sk_reserved);
>  		BUG_ON(svsk->sk_pool != pool);
> @@ -351,17 +351,16 @@ void svc_reserve(struct svc_rqst *rqstp,
>  /*
>   * Release a socket after use.
>   */
> -static inline void
> -svc_sock_put(struct svc_sock *svsk)
> +void
> +__svc_sock_put(struct svc_sock *svsk)
>  {
> -	if (atomic_dec_and_test(&svsk->sk_inuse)) {
> -		BUG_ON(! test_bit(SK_DEAD, &svsk->sk_flags));
> +	BUG_ON(! test_bit(SK_DEAD, &svsk->sk_flags));
>  
> -		if (svsk->sk_info_authunix != NULL)
> -			svcauth_unix_info_release(svsk->sk_info_authunix);
> -    	    	svsk->sk_xprt->xpt_free(svsk);
> -	}
> +	if (svsk->sk_info_authunix != NULL)
> +		svcauth_unix_info_release(svsk->sk_info_authunix);
> +	svsk->sk_xprt->xpt_free(svsk);
>  }
> +EXPORT_SYMBOL_GPL(__svc_sock_put);

I see here that you are integrating the detach and free methods into 
"svc_sock_put".  It might make sense to reorder your patches so that 
this comes earlier in the series (or is integrated somehow with the 
patch that introduces detach and free).

>  static void
>  svc_sock_release(struct svc_rqst *rqstp)
> @@ -1109,7 +1108,7 @@ svc_tcp_accept(struct svc_sock *svsk)
>  					  struct svc_sock,
>  					  sk_list);
>  			set_bit(SK_CLOSE, &svsk->sk_flags);
> -			atomic_inc(&svsk->sk_inuse);
> +			svc_sock_get(svsk);
>  		}
>  		spin_unlock_bh(&serv->sv_lock);
>  
> @@ -1481,7 +1480,7 @@ svc_recv(struct svc_rqst *rqstp, long ti
>  	spin_lock_bh(&pool->sp_lock);
>  	if ((svsk = svc_sock_dequeue(pool)) != NULL) {
>  		rqstp->rq_sock = svsk;
> -		atomic_inc(&svsk->sk_inuse);
> +		svc_sock_get(svsk);
>  		rqstp->rq_reserved = serv->sv_max_mesg;
>  		atomic_add(rqstp->rq_reserved, &svsk->sk_reserved);
>  	} else {
> @@ -1620,7 +1619,7 @@ svc_age_temp_sockets(unsigned long closu
>  			continue;
>  		if (atomic_read(&svsk->sk_inuse) || test_bit(SK_BUSY, &svsk->sk_flags))
>  			continue;
> -		atomic_inc(&svsk->sk_inuse);
> +		svc_sock_get(svsk);
>  		list_move(le, &to_be_aged);
>  		set_bit(SK_CLOSE, &svsk->sk_flags);
>  		set_bit(SK_DETACHED, &svsk->sk_flags);
> @@ -1868,7 +1867,7 @@ svc_delete_socket(struct svc_sock *svsk)
>  	 */
>  	if (!test_and_set_bit(SK_DEAD, &svsk->sk_flags)) {
>  		BUG_ON(atomic_read(&svsk->sk_inuse)<2);
> -		atomic_dec(&svsk->sk_inuse);
> +		svc_sock_put(svsk);
>  		if (test_bit(SK_TEMP, &svsk->sk_flags))
>  			serv->sv_tmpcnt--;
>  	}
> @@ -1883,7 +1882,7 @@ static void svc_close_socket(struct svc_
>  		/* someone else will have to effect the close */
>  		return;
>  
> -	atomic_inc(&svsk->sk_inuse);
> +	svc_sock_get(svsk);
>  	svc_delete_socket(svsk);
>  	clear_bit(SK_BUSY, &svsk->sk_flags);
>  	svc_sock_put(svsk);
> @@ -1976,7 +1975,7 @@ svc_defer(struct cache_req *req)
>  		dr->argslen = rqstp->rq_arg.len >> 2;
>  		memcpy(dr->args, rqstp->rq_arg.head[0].iov_base-skip, dr->argslen<<2);
>  	}
> -	atomic_inc(&rqstp->rq_sock->sk_inuse);
> +	svc_sock_get(rqstp->rq_sock);
>  	dr->svsk = rqstp->rq_sock;
>  
>  	dr->handle.revisit = svc_revisit;

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 315 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
email;internet:chuck dot lever at nospam oracle dot com
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs