Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky <skinsbursky@parallels.com>]

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?

-- 
Best regards,
Stanislav Kinsbursky