| Message ID | 20110920101341.9861.51453.stgit@localhost6.localdomain6 |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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@parallels.com> > > --- > 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? > +} > + > +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 */ > + 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); > +} > + > /* > * Returns zero on success, otherwise a negative errno value > * is returned. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Schumaker, Bryan > Sent: Tuesday, September 20, 2011 9:05 AM > To: Stanislav Kinsbursky > Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; xemul@parallels.com; > neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > bfields@fieldses.org; davem@davemloft.net > 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@parallels.com> > > > > --- > > 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. > > +} > > + > > +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? > > + 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); > > +} > > + > > /* > > * Returns zero on success, otherwise a negative errno value > > * is returned. > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" > > in the body of a message to majordomo@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org; xemul@parallels.com; >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >> bfields@fieldses.org; davem@davemloft.net >> 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@parallels.com> >>> >>> --- >>> 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(). >>> + 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); >>> +} >>> + >>> /* >>> * Returns zero on success, otherwise a negative errno value >>> * is returned. >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" >>> in the body of a message to majordomo@vger.kernel.org More >> majordomo >>> info at http://vger.kernel.org/majordomo-info.html > > �{.n�+�������+%��lzwm��b�맲��r��zX��߲)���w* jg��� �����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a���� �G���h��j:+v���w�٥
> -----Original Message----- > From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com] > Sent: Tuesday, September 20, 2011 9:35 AM > To: Myklebust, Trond > Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov; > neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > bfields@fieldses.org; davem@davemloft.net > 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@vger.kernel.org; xemul@parallels.com; > >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > >> bfields@fieldses.org; davem@davemloft.net > >> 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@parallels.com> > >>> > >>> --- > >>> 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); > >>> +} > >>> +
20.09.2011 18:14, Myklebust, Trond пишет: >>> >>> 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? > Yes, you right. > 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? > We check rpcb_users under spinlock. Gain spinlock forces memory barrier, doesn't it?
> -----Original Message----- > From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com] > Sent: Tuesday, September 20, 2011 10:35 AM > To: Myklebust, Trond > Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov; > neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > bfields@fieldses.org; davem@davemloft.net > Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference > counted rpcbind clients > > 20.09.2011 18:14, Myklebust, Trond пишет: > > >>> > >>> 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? > > > > Yes, you right. > > > 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? > > > > We check rpcb_users under spinlock. Gain spinlock forces memory barrier, > doesn't it? Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order. Cheers Trond
20.09.2011 18:38, Myklebust, Trond пишет: >> -----Original Message----- >> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com] >> Sent: Tuesday, September 20, 2011 10:35 AM >> To: Myklebust, Trond >> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov; >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >> bfields@fieldses.org; davem@davemloft.net >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference >> counted rpcbind clients >> >> 20.09.2011 18:14, Myklebust, Trond пишет: >> >>>>> >>>>> 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? >>> >> >> Yes, you right. >> >>> 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? >>> >> >> We check rpcb_users under spinlock. Gain spinlock forces memory barrier, >> doesn't it? > > Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order. > Yep, now I understand what are you talking about. Will fix both places (set and put).
20.09.2011 18:38, Myklebust, Trond пишет: >> -----Original Message----- >> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com] >> Sent: Tuesday, September 20, 2011 10:35 AM >> To: Myklebust, Trond >> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov; >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >> bfields@fieldses.org; davem@davemloft.net >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference >> counted rpcbind clients >> >> 20.09.2011 18:14, Myklebust, Trond пишет: >> >>>>> >>>>> 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? >>> >> >> Yes, you right. >> >>> 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? >>> >> >> We check rpcb_users under spinlock. Gain spinlock forces memory barrier, >> doesn't it? > > Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order. > Trond, I've thought again and realized, that even if these writes (rpcb_users and rpbc_local_clnt/4) will be done in reversed order, then no barrier required here. Because if we have another process trying to create those clients (it can't use them since it's not started yet) on other CPU, than it's waiting on creation mutex. When it will gain the mutex, we will have required memory barrier for us. Or I missed something in your explanation?
> -----Original Message----- > From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com] > Sent: Tuesday, September 20, 2011 12:21 PM > To: Myklebust, Trond > Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov; > neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > bfields@fieldses.org; davem@davemloft.net > Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference > counted rpcbind clients > > 20.09.2011 18:38, Myklebust, Trond пишет: > >> -----Original Message----- > >> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com] > >> Sent: Tuesday, September 20, 2011 10:35 AM > >> To: Myklebust, Trond > >> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov; > >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > >> bfields@fieldses.org; davem@davemloft.net > >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference > >> counted rpcbind clients > >> > >> 20.09.2011 18:14, Myklebust, Trond пишет: > >> > >>>>> > >>>>> 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? > >>> > >> > >> Yes, you right. > >> > >>> 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? > >>> > >> > >> We check rpcb_users under spinlock. Gain spinlock forces memory > >> barrier, doesn't it? > > > > Yes, and so you don't need an smp_rmb() on the reader side. However, > you still need to ensure that the processor which _sets_ the rpcb_users and > rpcb_local_clnt/4 actually writes them in the correct order. > > > > Trond, I've thought again and realized, that even if these writes (rpcb_users > and rpbc_local_clnt/4) will be done in reversed order, then no barrier > required here. > Because if we have another process trying to create those clients (it can't use > them since it's not started yet) on other CPU, than it's waiting on creation > mutex. When it will gain the mutex, we will have required memory barrier > for us. > > Or I missed something in your explanation? You need to ensure that if someone calls rpcb_get_local() and gets a positive result, then they can rely on rpcb_local_clnt/4 being non-zero. Without the write barrier, that is not the case. Without that guarantee, you can't really ensure that rpcb_put_local() will work as expected either, since it will be possible to trigger the --rpcb_users == 0 case without shutting down rpcb_local_clnt/4 (because clnt/clnt4 == 0). Cheers Trond
20.09.2011 21:13, Myklebust, Trond пишет: >> -----Original Message----- >> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com] >> Sent: Tuesday, September 20, 2011 12:21 PM >> To: Myklebust, Trond >> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov; >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >> bfields@fieldses.org; davem@davemloft.net >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference >> counted rpcbind clients >> >> 20.09.2011 18:38, Myklebust, Trond пишет: >>>> -----Original Message----- >>>> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com] >>>> Sent: Tuesday, September 20, 2011 10:35 AM >>>> To: Myklebust, Trond >>>> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov; >>>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> bfields@fieldses.org; davem@davemloft.net >>>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference >>>> counted rpcbind clients >>>> >>>> 20.09.2011 18:14, Myklebust, Trond пишет: >>>> >>>>>>> >>>>>>> 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? >>>>> >>>> >>>> Yes, you right. >>>> >>>>> 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? >>>>> >>>> >>>> We check rpcb_users under spinlock. Gain spinlock forces memory >>>> barrier, doesn't it? >>> >>> Yes, and so you don't need an smp_rmb() on the reader side. However, >> you still need to ensure that the processor which _sets_ the rpcb_users and >> rpcb_local_clnt/4 actually writes them in the correct order. >>> >> >> Trond, I've thought again and realized, that even if these writes (rpcb_users >> and rpbc_local_clnt/4) will be done in reversed order, then no barrier >> required here. >> Because if we have another process trying to create those clients (it can't use >> them since it's not started yet) on other CPU, than it's waiting on creation >> mutex. When it will gain the mutex, we will have required memory barrier >> for us. >> >> Or I missed something in your explanation? > > You need to ensure that if someone calls rpcb_get_local() and gets a positive result, then they can rely on rpcb_local_clnt/4 being non-zero. Without the write barrier, that is not the case. > In current context we can be sure, that between rpcb_get_local() and first dereference of rpcb_local_clnt/4 we have at least one spinlock (svc_xprt_class_lock in svc_create_xprt). But I understand, that we can't relay on it since this code can be changed in future. So I'll add barrier there. > Without that guarantee, you can't really ensure that rpcb_put_local() will work as expected either, since it will be possible to trigger the --rpcb_users == 0 case without shutting down rpcb_local_clnt/4 (because clnt/clnt4 == 0). > Yes, you right. But it doesn't mean, that we require barrier here, because we don't need this garantee you are talking about. We can be sure, that we always see right rpcb_users value. It means that, if we set this value to zero, then no other running services left and no references to those clients can occur. And even if we have another process which is going to create new service right now on another CPU, then this process will see that no rpcbind users present and will create new clients and assign them to global variables prior to any use of this clients can occur. And this assign will be done with barrier as we agreed earlier. > Cheers > Trond >
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; +} + +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 */ + 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); +} + /* * Returns zero on success, otherwise a negative errno value * is returned.
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@parallels.com> --- net/sunrpc/rpcb_clnt.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 50 insertions(+), 0 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html