| Message ID | 4E7899E7.9090809@parallels.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 20 Sep 2011 17:49:27 +0400 Stanislav Kinsbursky <skinsbursky@parallels.com> wrote: > v5: fixed races with rpcb_users in rpcb_get_local() > > 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 53 insertions(+), 0 deletions(-) > > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c > index e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) > kfree(map); > } > +static int rpcb_get_local(void) > +{ > + int cnt; > + > + spin_lock(&rpcb_clnt_lock); > + if (rpcb_users) > + rpcb_users++; > + cnt = rpcb_users; > + spin_unlock(&rpcb_clnt_lock); > + > + return cnt; > +} > + > +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; > + } In the function below, you mention that the above pointers are protected by rpcb_create_local_mutex, but it looks like they get reset here without that being held? Might it be simpler to just protect rpcb_users with the rpcb_create_local_mutex and ensure that it's held whenever you call one of these routines? None of these are codepaths are particularly hot. > + 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
> -----Original Message----- > From: Jeff Layton [mailto:jlayton@redhat.com] > Sent: Tuesday, September 20, 2011 10:25 AM > To: Stanislav Kinsbursky > Cc: Myklebust, Trond; 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 v5 1/8] SUNRPC: introduce helpers for reference > counted rpcbind clients > > On Tue, 20 Sep 2011 17:49:27 +0400 > Stanislav Kinsbursky <skinsbursky@parallels.com> wrote: > > > v5: fixed races with rpcb_users in rpcb_get_local() > > > > 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 | 53 > ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 files changed, 53 insertions(+), 0 deletions(-) > > > > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index > > e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) > > kfree(map); > > } > > +static int rpcb_get_local(void) > > +{ > > + int cnt; > > + > > + spin_lock(&rpcb_clnt_lock); > > + if (rpcb_users) > > + rpcb_users++; > > + cnt = rpcb_users; > > + spin_unlock(&rpcb_clnt_lock); > > + > > + return cnt; > > +} > > + > > +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; > > + } > > In the function below, you mention that the above pointers are protected by > rpcb_create_local_mutex, but it looks like they get reset here without that > being held? > > Might it be simpler to just protect rpcb_users with the > rpcb_create_local_mutex and ensure that it's held whenever you call one of > these routines? None of these are codepaths are particularly hot. Alternatively, if you do if (rpcb_users == 1) { rpcb_local_clnt = NULL; rpcb_local_clnt4 = NULL; smp_wmb(); rpcb_users = 0; } else rpcb_users--; then the spinlock protection in rpbc_get_local() is still good enough to guarantee correctness. -- 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
20.09.2011 18:24, Jeff Layton пишет: > On Tue, 20 Sep 2011 17:49:27 +0400 > Stanislav Kinsbursky<skinsbursky@parallels.com> wrote: > >> v5: fixed races with rpcb_users in rpcb_get_local() >> >> 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 53 insertions(+), 0 deletions(-) >> >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c >> index e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) >> kfree(map); >> } >> +static int rpcb_get_local(void) >> +{ >> + int cnt; >> + >> + spin_lock(&rpcb_clnt_lock); >> + if (rpcb_users) >> + rpcb_users++; >> + cnt = rpcb_users; >> + spin_unlock(&rpcb_clnt_lock); >> + >> + return cnt; >> +} >> + >> +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; >> + } > > In the function below, you mention that the above pointers are > protected by rpcb_create_local_mutex, but it looks like they get reset > here without that being held? > Assigning of them is protected by rpcb_create_local_mutex. Dereferencing of them is protected by rpcb_clnt_lock. > Might it be simpler to just protect rpcb_users with the > rpcb_create_local_mutex and ensure that it's held whenever you call one > of these routines? None of these are codepaths are particularly hot. > I just inherited this lock-mutex logic. Actually, you right. This codepaths are used rarely. But are use sure, that we need to remove this "speed-up" logic if we take into account that it was here already? >> + 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 > >
On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote: > 20.09.2011 18:24, Jeff Layton пишет: >> On Tue, 20 Sep 2011 17:49:27 +0400 >> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote: >> >>> v5: fixed races with rpcb_users in rpcb_get_local() >>> >>> 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 53 insertions(+), 0 deletions(-) >>> >>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c >>> index e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) >>> kfree(map); >>> } >>> +static int rpcb_get_local(void) >>> +{ >>> + int cnt; >>> + >>> + spin_lock(&rpcb_clnt_lock); >>> + if (rpcb_users) >>> + rpcb_users++; >>> + cnt = rpcb_users; >>> + spin_unlock(&rpcb_clnt_lock); >>> + >>> + return cnt; >>> +} >>> + >>> +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; >>> + } >> >> In the function below, you mention that the above pointers are >> protected by rpcb_create_local_mutex, but it looks like they get reset >> here without that being held? >> > > Assigning of them is protected by rpcb_create_local_mutex. > Dereferencing of them is protected by rpcb_clnt_lock. Shouldn't you be using the same lock for assigning and dereferencing? Otherwise one thread could change these variables while another is using them. > >> Might it be simpler to just protect rpcb_users with the >> rpcb_create_local_mutex and ensure that it's held whenever you call one >> of these routines? None of these are codepaths are particularly hot. >> > > I just inherited this lock-mutex logic. > Actually, you right. This codepaths are used rarely. > But are use sure, that we need to remove this "speed-up" logic if we take into account that it was here already? > >>> + 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
On Tue, 20 Sep 2011 18:43:45 +0400 Stanislav Kinsbursky <skinsbursky@parallels.com> wrote: > 20.09.2011 18:24, Jeff Layton пишет: > > On Tue, 20 Sep 2011 17:49:27 +0400 > > Stanislav Kinsbursky<skinsbursky@parallels.com> wrote: > > > >> v5: fixed races with rpcb_users in rpcb_get_local() > >> > >> 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 files changed, 53 insertions(+), 0 deletions(-) > >> > >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c > >> index e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) > >> kfree(map); > >> } > >> +static int rpcb_get_local(void) > >> +{ > >> + int cnt; > >> + > >> + spin_lock(&rpcb_clnt_lock); > >> + if (rpcb_users) > >> + rpcb_users++; > >> + cnt = rpcb_users; > >> + spin_unlock(&rpcb_clnt_lock); > >> + > >> + return cnt; > >> +} > >> + > >> +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; > >> + } > > > > In the function below, you mention that the above pointers are > > protected by rpcb_create_local_mutex, but it looks like they get reset > > here without that being held? > > > > Assigning of them is protected by rpcb_create_local_mutex. > Dereferencing of them is protected by rpcb_clnt_lock. > That's probably a bug, but I haven't sat down to work through the logic. > > Might it be simpler to just protect rpcb_users with the > > rpcb_create_local_mutex and ensure that it's held whenever you call one > > of these routines? None of these are codepaths are particularly hot. > > > > I just inherited this lock-mutex logic. > Actually, you right. This codepaths are used rarely. > But are use sure, that we need to remove this "speed-up" logic if we take into > account that it was here already? > There are many ways to do this... In general, it's difficult to get locking right, especially when you start mixing multiple locks on related resources. Personally, I'd go with a simpler scheme here. There's not much value in protecting this counter with a spinlock when the other parts need to be protected by a mutex. If you do decide to do it with multiple locks, then please do document in comments how the locking is expected to work. An alternate scheme might be to consider doing this with krefs, but I haven't really considered whether that idiom makes sense here.
20.09.2011 18:58, Bryan Schumaker пишет: > On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote: >> 20.09.2011 18:24, Jeff Layton пишет: >>> On Tue, 20 Sep 2011 17:49:27 +0400 >>> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote: >>> >>>> v5: fixed races with rpcb_users in rpcb_get_local() >>>> >>>> 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 files changed, 53 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c >>>> index e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) >>>> kfree(map); >>>> } >>>> +static int rpcb_get_local(void) >>>> +{ >>>> + int cnt; >>>> + >>>> + spin_lock(&rpcb_clnt_lock); >>>> + if (rpcb_users) >>>> + rpcb_users++; >>>> + cnt = rpcb_users; >>>> + spin_unlock(&rpcb_clnt_lock); >>>> + >>>> + return cnt; >>>> +} >>>> + >>>> +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; >>>> + } >>> >>> In the function below, you mention that the above pointers are >>> protected by rpcb_create_local_mutex, but it looks like they get reset >>> here without that being held? >>> >> >> Assigning of them is protected by rpcb_create_local_mutex. >> Dereferencing of them is protected by rpcb_clnt_lock. > > Shouldn't you be using the same lock for assigning and dereferencing? Otherwise one thread could change these variables while another is using them. Probably I wasn't clear with my previous explanation. Actually, we use only spinlock to make sure, that the pointers are valid when we dereferencing them. Synchronization point here is rpcb_users counter. IOW, we use these pointers only from svc code and only after service already started. And with this patch-set we can be sure, that this pointers has been created already to the point, when this service starts. But when this counter is zero, we can experience races with assigning those pointers. It takes a lot of time, so we use local mutex here instead of spinlock. Have I answered your question?
20.09.2011 18:41, Myklebust, Trond пишет: >> -----Original Message----- >> From: Jeff Layton [mailto:jlayton@redhat.com] >> Sent: Tuesday, September 20, 2011 10:25 AM >> To: Stanislav Kinsbursky >> Cc: Myklebust, Trond; 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 v5 1/8] SUNRPC: introduce helpers for reference >> counted rpcbind clients >> >> On Tue, 20 Sep 2011 17:49:27 +0400 >> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote: >> >>> v5: fixed races with rpcb_users in rpcb_get_local() >>> >>> 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 | 53 >> ++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 53 insertions(+), 0 deletions(-) >>> >>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index >>> e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) >>> kfree(map); >>> } >>> +static int rpcb_get_local(void) >>> +{ >>> + int cnt; >>> + >>> + spin_lock(&rpcb_clnt_lock); >>> + if (rpcb_users) >>> + rpcb_users++; >>> + cnt = rpcb_users; >>> + spin_unlock(&rpcb_clnt_lock); >>> + >>> + return cnt; >>> +} >>> + >>> +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; >>> + } >> >> In the function below, you mention that the above pointers are > protected by >> rpcb_create_local_mutex, but it looks like they get reset here without > that >> being held? >> >> Might it be simpler to just protect rpcb_users with the >> rpcb_create_local_mutex and ensure that it's held whenever you call > one of >> these routines? None of these are codepaths are particularly hot. > > Alternatively, if you do > > if (rpcb_users == 1) { > rpcb_local_clnt = NULL; > rpcb_local_clnt4 = NULL; > smp_wmb(); > rpcb_users = 0; > } else > rpcb_users--; > > then the spinlock protection in rpbc_get_local() is still good enough to > guarantee correctness. I don't understand the idea of this code. It guarantees, that if rpcb_users == 0, then rpcb_local_clnt == NULL and rpcb_local_clnt4 == NULL. But we don't need such guarantee from my pow. I.e. if rpcb_users == 0, then it means, that no services running right now. For example, processes, destroying those clients, is running on CPU#0. On CPU#1, for example, we have another process trying to get those clients and waiting on spinlock. When this process will gain the spinlock, it will see 0 users, gain mutex and then try to create new clients. We still have no users on this clients yet. And this process will just reassign whose rpcbind clients pointers (and here we need memmory barrier for sure).
On 09/20/2011 11:38 AM, Stanislav Kinsbursky wrote: > 20.09.2011 18:58, Bryan Schumaker пишет: >> On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote: >>> 20.09.2011 18:24, Jeff Layton пишет: >>>> On Tue, 20 Sep 2011 17:49:27 +0400 >>>> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote: >>>> >>>>> v5: fixed races with rpcb_users in rpcb_get_local() >>>>> >>>>> 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 files changed, 53 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c >>>>> index e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) >>>>> kfree(map); >>>>> } >>>>> +static int rpcb_get_local(void) >>>>> +{ >>>>> + int cnt; >>>>> + >>>>> + spin_lock(&rpcb_clnt_lock); >>>>> + if (rpcb_users) >>>>> + rpcb_users++; >>>>> + cnt = rpcb_users; >>>>> + spin_unlock(&rpcb_clnt_lock); >>>>> + >>>>> + return cnt; >>>>> +} >>>>> + >>>>> +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; >>>>> + } >>>> >>>> In the function below, you mention that the above pointers are >>>> protected by rpcb_create_local_mutex, but it looks like they get reset >>>> here without that being held? >>>> >>> >>> Assigning of them is protected by rpcb_create_local_mutex. >>> Dereferencing of them is protected by rpcb_clnt_lock. >> >> Shouldn't you be using the same lock for assigning and dereferencing? Otherwise one thread could change these variables while another is using them. > > Probably I wasn't clear with my previous explanation. > Actually, we use only spinlock to make sure, that the pointers are valid when we dereferencing them. Synchronization point here is rpcb_users counter. > IOW, we use these pointers only from svc code and only after service already started. And with this patch-set we can be sure, that this pointers has been created already to the point, when this service starts. > > But when this counter is zero, we can experience races with assigning those pointers. It takes a lot of time, so we use local mutex here instead of spinlock. > > Have I answered your question? I think I understand now. Thanks! > -- 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
20.09.2011 19:11, Jeff Layton пишет: > > In general, it's difficult to get locking right, especially when you > start mixing multiple locks on related resources. Personally, I'd go > with a simpler scheme here. There's not much value in protecting this > counter with a spinlock when the other parts need to be protected by a > mutex. If you do decide to do it with multiple locks, then please do > document in comments how the locking is expected to work. > > An alternate scheme might be to consider doing this with krefs, but I > haven't really considered whether that idiom makes sense here. > Jeff, please, have a look at my answer to Bryan Schumaker. Does it allay your apprehensions?
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index e45d2fb..5f4a406 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,56 @@ static void rpcb_map_release(void *data) kfree(map);
v5: fixed races with rpcb_users in rpcb_get_local() 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 53 insertions(+), 0 deletions(-) } +static int rpcb_get_local(void) +{ + int cnt; + + spin_lock(&rpcb_clnt_lock); + if (rpcb_users) + rpcb_users++; + cnt = rpcb_users; + spin_unlock(&rpcb_clnt_lock); + + return cnt; +} + +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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html