RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Myklebust, Trond]

> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@xxxxxxxxxxxxx]
> Sent: Tuesday, September 20, 2011 9:35 AM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; linux-nfs@xxxxxxxxxxxxxxx; Pavel Emelianov;
> neilb@xxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> bfields@xxxxxxxxxxxx; davem@xxxxxxxxxxxxx
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
> 
> 20.09.2011 17:15, Myklebust, Trond ÐÐÑÐÑ:
> >> -----Original Message-----
> >> From: Schumaker, Bryan
> >> Sent: Tuesday, September 20, 2011 9:05 AM
> >> To: Stanislav Kinsbursky
> >> Cc: Myklebust, Trond; linux-nfs@xxxxxxxxxxxxxxx; xemul@xxxxxxxxxxxxx;
> >> neilb@xxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> bfields@xxxxxxxxxxxx; davem@xxxxxxxxxxxxx
> >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> >> counted rpcbind clients
> >>
> >> On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:
> >>> This helpers will be used for dynamical creation and destruction of
> >>> rpcbind clients.
> >>> Variable rpcb_users is actually a counter of lauched RPC services.
> >>> If rpcbind clients has been created already, then we just increase
> rpcb_users.
> >>>
> >>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@xxxxxxxxxxxxx>
> >>>
> >>> ---
> >>>   net/sunrpc/rpcb_clnt.c |   50
> >> ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>   1 files changed, 50 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
> >>> e45d2fb..8724780 100644
> >>> --- a/net/sunrpc/rpcb_clnt.c
> >>> +++ b/net/sunrpc/rpcb_clnt.c
> >>> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
> >>>   static struct rpc_clnt *	rpcb_local_clnt;
> >>>   static struct rpc_clnt *	rpcb_local_clnt4;
> >>>
> >>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
> >>> +unsigned int			rpcb_users;
> >>> +
> >>>   struct rpcbind_args {
> >>>   	struct rpc_xprt *	r_xprt;
> >>>
> >>> @@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
> >>>   	kfree(map);
> >>>   }
> >>>
> >>> +static int rpcb_get_local(void)
> >>> +{
> >>> +	spin_lock(&rpcb_clnt_lock);
> >>> +	if (rpcb_users)
> >>> +		rpcb_users++;
> >>> +	spin_unlock(&rpcb_clnt_lock);
> >>> +
> >>> +	return rpcb_users;
> >>          ^^^^^^^^^^^^^^^^^^
> >> Is it safe to use this variable outside of the rpcb_clnt_lock?
> >>
> > Nope. If rpcb_users was zero in the protected section above, nothing
> guarantees that it will still be zero here, and so the caller may get the (wrong)
> impression that the counter was incremented.
> >
> 
> Yep, you right. Will fix this races.
> 
> >>> +}
> >>> +
> >>> +void rpcb_put_local(void)
> >>> +{
> >>> +	struct rpc_clnt *clnt = rpcb_local_clnt;
> >>> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> >>> +	int shutdown;
> >>> +
> >>> +	spin_lock(&rpcb_clnt_lock);
> >>> +	if (--rpcb_users == 0) {
> >>> +		rpcb_local_clnt = NULL;
> >>> +		rpcb_local_clnt4 = NULL;
> >>> +	}
> >>> +	shutdown = !rpcb_users;
> >>> +	spin_unlock(&rpcb_clnt_lock);
> >>> +
> >>> +	if (shutdown) {
> >>> +		/*
> >>> +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> >>> +		 */
> >>> +		if (clnt4)
> >>> +			rpc_shutdown_client(clnt4);
> >>> +		if (clnt)
> >>> +			rpc_shutdown_client(clnt);
> >>> +	}
> >>> +	return;
> >>> +}
> >>> +
> >>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt
> >>> +*clnt4) {
> >>> +	/* Protected by rpcb_create_local_mutex */
> >
> > Doesn't it  need to be protected by rpcb_clnt_lock too?
> >
> 
> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no one
> will change rpcb_users since it's zero. If it's non zero - we willn't get to
> rpcb_set_local().

OK, so you are saying that the rpcb_users++ below could be replaced by rpcb_users=1?

In that case, don't you need a smp_wmb() between the setting of rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?

> >>> +	rpcb_local_clnt = clnt;
> >>> +	rpcb_local_clnt4 = clnt4;
> >>> +	rpcb_users++;
> > ^^^^^^^^^^^^^^^^^^^
> >
> >>> +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
> >>> +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> >>> +			rpcb_local_clnt4);
> >>> +}
> >>> +

¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_