diff for duplicates of <4E78B9FD.70509@netapp.com> diff --git a/a/1.txt b/N1/1.txt index cb9a344..40f59f1 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,25 +1,21 @@ On 09/20/2011 11:38 AM, Stanislav Kinsbursky wrote: -> 20.09.2011 18:58, Bryan Schumaker =D0=BF=D0=B8=D1=88=D0=B5=D1=82: +> 20.09.2011 18:58, Bryan Schumaker пишет: >> On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote: ->>> 20.09.2011 18:24, Jeff Layton =D0=BF=D0=B8=D1=88=D0=B5=D1=82: +>>> 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 +>>>>> This helpers will be used for dynamical creation and destruction of rpcbind >>>>> clients. ->>>>> Variable rpcb_users is actually a counter of lauched RPC services= -=2E If rpcbind ->>>>> clients has been created already, then we just increase rpcb_user= -s. +>>>>> 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 +++++++++++++++++++++++++++++++= -+++++++++++++++++ +>>>>> 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 @@ -44,7 +40,7 @@ s. >>>>> + spin_lock(&rpcb_clnt_lock); >>>>> + if (rpcb_users) >>>>> + rpcb_users++; ->>>>> + cnt =3D rpcb_users; +>>>>> + cnt = rpcb_users; >>>>> + spin_unlock(&rpcb_clnt_lock); >>>>> + >>>>> + return cnt; @@ -52,42 +48,33 @@ s. >>>>> + >>>>> +void rpcb_put_local(void) >>>>> +{ ->>>>> + struct rpc_clnt *clnt =3D rpcb_local_clnt; ->>>>> + struct rpc_clnt *clnt4 =3D rpcb_local_clnt4; +>>>>> + struct rpc_clnt *clnt = rpcb_local_clnt; +>>>>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4; >>>>> + int shutdown; >>>>> + >>>>> + spin_lock(&rpcb_clnt_lock); ->>>>> + if (--rpcb_users =3D=3D 0) { ->>>>> + rpcb_local_clnt =3D NULL; ->>>>> + rpcb_local_clnt4 =3D NULL; +>>>>> + 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 r= -eset +>>>> 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 u= -sing them. ->=20 +>> 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 va= -lid when we dereferencing them. Synchronization point here is rpcb_user= -s 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 poin= -ters has been created already to the point, when this service starts. ->=20 -> 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 ins= -tead of spinlock. ->=20 +> 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! ->=20 +> diff --git a/a/content_digest b/N1/content_digest index dcbe174..d669119 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -21,27 +21,23 @@ "\00:1\0" "b\0" "On 09/20/2011 11:38 AM, Stanislav Kinsbursky wrote:\n" - "> 20.09.2011 18:58, Bryan Schumaker =D0=BF=D0=B8=D1=88=D0=B5=D1=82:\n" + "> 20.09.2011 18:58, Bryan Schumaker \320\277\320\270\321\210\320\265\321\202:\n" ">> On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:\n" - ">>> 20.09.2011 18:24, Jeff Layton =D0=BF=D0=B8=D1=88=D0=B5=D1=82:\n" + ">>> 20.09.2011 18:24, Jeff Layton \320\277\320\270\321\210\320\265\321\202:\n" ">>>> On Tue, 20 Sep 2011 17:49:27 +0400\n" ">>>> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote:\n" ">>>>\n" ">>>>> v5: fixed races with rpcb_users in rpcb_get_local()\n" ">>>>>\n" - ">>>>> This helpers will be used for dynamical creation and destruction =\n" - "of rpcbind\n" + ">>>>> This helpers will be used for dynamical creation and destruction of rpcbind\n" ">>>>> clients.\n" - ">>>>> Variable rpcb_users is actually a counter of lauched RPC services=\n" - "=2E If rpcbind\n" - ">>>>> clients has been created already, then we just increase rpcb_user=\n" - "s.\n" + ">>>>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind\n" + ">>>>> clients has been created already, then we just increase rpcb_users.\n" ">>>>>\n" ">>>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>\n" ">>>>>\n" ">>>>> ---\n" - ">>>>> net/sunrpc/rpcb_clnt.c | 53 +++++++++++++++++++++++++++++++=\n" - "+++++++++++++++++\n" + ">>>>> net/sunrpc/rpcb_clnt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++\n" ">>>>> 1 files changed, 53 insertions(+), 0 deletions(-)\n" ">>>>>\n" ">>>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c\n" @@ -66,7 +62,7 @@ ">>>>> + spin_lock(&rpcb_clnt_lock);\n" ">>>>> + if (rpcb_users)\n" ">>>>> + rpcb_users++;\n" - ">>>>> + cnt =3D rpcb_users;\n" + ">>>>> + cnt = rpcb_users;\n" ">>>>> + spin_unlock(&rpcb_clnt_lock);\n" ">>>>> +\n" ">>>>> + return cnt;\n" @@ -74,44 +70,35 @@ ">>>>> +\n" ">>>>> +void rpcb_put_local(void)\n" ">>>>> +{\n" - ">>>>> + struct rpc_clnt *clnt =3D rpcb_local_clnt;\n" - ">>>>> + struct rpc_clnt *clnt4 =3D rpcb_local_clnt4;\n" + ">>>>> + struct rpc_clnt *clnt = rpcb_local_clnt;\n" + ">>>>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;\n" ">>>>> + int shutdown;\n" ">>>>> +\n" ">>>>> + spin_lock(&rpcb_clnt_lock);\n" - ">>>>> + if (--rpcb_users =3D=3D 0) {\n" - ">>>>> + rpcb_local_clnt =3D NULL;\n" - ">>>>> + rpcb_local_clnt4 =3D NULL;\n" + ">>>>> + if (--rpcb_users == 0) {\n" + ">>>>> + rpcb_local_clnt = NULL;\n" + ">>>>> + rpcb_local_clnt4 = NULL;\n" ">>>>> + }\n" ">>>>\n" ">>>> In the function below, you mention that the above pointers are\n" - ">>>> protected by rpcb_create_local_mutex, but it looks like they get r=\n" - "eset\n" + ">>>> protected by rpcb_create_local_mutex, but it looks like they get reset\n" ">>>> here without that being held?\n" ">>>>\n" ">>>\n" ">>> Assigning of them is protected by rpcb_create_local_mutex.\n" ">>> Dereferencing of them is protected by rpcb_clnt_lock.\n" ">>\n" - ">> Shouldn't you be using the same lock for assigning and dereferencing=\n" - "? Otherwise one thread could change these variables while another is u=\n" - "sing them.\n" - ">=20\n" + ">> Shouldn't you be using the same lock for assigning and dereferencing? Otherwise one thread could change these variables while another is using them.\n" + "> \n" "> Probably I wasn't clear with my previous explanation.\n" - "> Actually, we use only spinlock to make sure, that the pointers are va=\n" - "lid when we dereferencing them. Synchronization point here is rpcb_user=\n" - "s counter.\n" - "> IOW, we use these pointers only from svc code and only after service =\n" - "already started. And with this patch-set we can be sure, that this poin=\n" - "ters has been created already to the point, when this service starts.\n" - ">=20\n" - "> But when this counter is zero, we can experience races with assigning=\n" - " those pointers. It takes a lot of time, so we use local mutex here ins=\n" - "tead of spinlock.\n" - ">=20\n" + "> Actually, we use only spinlock to make sure, that the pointers are valid when we dereferencing them. Synchronization point here is rpcb_users counter.\n" + "> 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.\n" + "> \n" + "> 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.\n" + "> \n" "> Have I answered your question?\n" "\n" "I think I understand now. Thanks!\n" - >=20 + > -477f63280dd5e93ff20aca8b764b2db8032b60e1ef9367f150523a42f952e512 +ad3b7c689c698eac42d80191aa24784f9bf05997414edee5fdc3839195d1c418
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.