Re: [PATCH 14/38] svc: Change sk_inuse to a kref

[Tom Tucker <tom@opengridcomputing.com>]

On Fri, 2007-12-14 at 15:52 -0500, J. Bruce Fields wrote:
> On Tue, Dec 11, 2007 at 05:32:25PM -0600, Tom Tucker wrote:
> > 
> > Change the atomic_t reference count to a kref and move it to the 
> > transport indepenent svc_xprt structure. Change the reference count
> > wrapper names to be generic.
> > 
> > Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> > ---
> > 
> >  include/linux/sunrpc/svc_xprt.h |    8 ++++++
> >  include/linux/sunrpc/svcsock.h  |    1 -
> >  net/sunrpc/svc_xprt.c           |   17 ++++++++++++
> >  net/sunrpc/svcsock.c            |   54 +++++++++++++++------------------------
> >  4 files changed, 46 insertions(+), 34 deletions(-)
> > 
> > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> > index 3f4a1df..eb801ad 100644
> > --- a/include/linux/sunrpc/svc_xprt.h
> > +++ b/include/linux/sunrpc/svc_xprt.h
> > @@ -8,6 +8,7 @@
> >  #define SUNRPC_SVC_XPRT_H
> >  
> >  #include <linux/sunrpc/svc.h>
> > +#include <linux/module.h>
> >  
> >  struct svc_xprt_ops {
> >  	struct svc_xprt	*(*xpo_create)(struct svc_serv *,
> > @@ -34,11 +35,18 @@ struct svc_xprt_class {
> >  struct svc_xprt {
> >  	struct svc_xprt_class	*xpt_class;
> >  	struct svc_xprt_ops	*xpt_ops;
> > +	struct kref		xpt_ref;
> >  };
> >  
> >  int	svc_reg_xprt_class(struct svc_xprt_class *);
> >  int	svc_unreg_xprt_class(struct svc_xprt_class *);
> >  void	svc_xprt_init(struct svc_xprt_class *, struct svc_xprt *);
> >  int	svc_create_xprt(struct svc_serv *, char *, unsigned short, int);
> > +void	svc_xprt_put(struct svc_xprt *xprt);
> > +
> > +static inline void svc_xprt_get(struct svc_xprt *xprt)
> > +{
> > +	kref_get(&xprt->xpt_ref);
> > +}
> >  
> >  #endif /* SUNRPC_SVC_XPRT_H */
> > diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> > index 3181d9d..ba07d50 100644
> > --- a/include/linux/sunrpc/svcsock.h
> > +++ b/include/linux/sunrpc/svcsock.h
> > @@ -24,7 +24,6 @@ struct svc_sock {
> >  
> >  	struct svc_pool *	sk_pool;	/* current pool iff queued */
> >  	struct svc_serv *	sk_server;	/* service for this socket */
> > -	atomic_t		sk_inuse;	/* use count */
> >  	unsigned long		sk_flags;
> >  #define	SK_BUSY		0			/* enqueued/receiving */
> >  #define	SK_CONN		1			/* conn pending */
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index 9136da4..43418cf 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -82,6 +82,22 @@ int svc_unreg_xprt_class(struct svc_xprt_class *xcl)
> >  }
> >  EXPORT_SYMBOL_GPL(svc_unreg_xprt_class);
> >  
> > +static void svc_xprt_free(struct kref *kref)
> > +{
> > +	struct svc_xprt *xprt =
> > +		container_of(kref, struct svc_xprt, xpt_ref);
> > +	struct module *owner = xprt->xpt_class->xcl_owner;
> > +	BUG_ON(atomic_read(&kref->refcount));
> 
> All this BUG_ON() is doing is checking for what looks like a bizarrely
> unlikely bug in kref_put().  The BUG_ON from the original code is:
> 
> 	BUG_ON(!test_bit(SK_DEAD, &svsk->sk_flags));
> 
> But I don't have any strong feelings as to whether that's necessary.
> 

It's not. 

> --b.