> -----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); > >>> +} > >>> + ��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
