Re: [Patch net] rds: mark bound socket with SOCK_RCU_FREE

[Santosh Shilimkar <santosh.shilimkar@oracle.com>]

On 9/10/2018 5:16 PM, Cong Wang wrote:
> On Mon, Sep 10, 2018 at 5:04 PM Sowmini Varadhan
> <sowmini.varadhan@oracle.com> wrote:
>>
>> On (09/10/18 16:51), Cong Wang wrote:
>>>
>>>          __rds_create_bind_key(key, addr, port, scope_id);
>>> -       rs = rhashtable_lookup_fast(&bind_hash_table, key, ht_parms);
>>> +       rcu_read_lock();
>>> +       rs = rhashtable_lookup(&bind_hash_table, key, ht_parms);
>>>          if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
>>>                  rds_sock_addref(rs);
>>>          else
>>>                  rs = NULL;
>>> +       rcu_read_unlock();
>>
>> aiui, the rcu_read lock/unlock is only useful if the write
>> side doing destructive operations  does something to make sure readers
>> are done before doing the destructive opertion. AFAIK, that does
>> not exist for rds socket management today
> 
> That is exactly why we need it here, right?
> 
> As you mentioned previously, the sock could be freed after
> rhashtable_lookup_fast() but before rds_sock_addref(), extending
> the RCU read section after rds_sock_addref() exactly solves
> the problem here.
>
Thats the case really.

> Or is there any other destructive problem you didn't mention?
> Mind to be specific?
> 
> 
>>
>>
>>> Although sock release path is not a very hot path, but blocking
>>> it isn't a good idea either, especially when you can use call_rcu(),
>>> which has the same effect.
>>>
>>> I don't see any reason we should prefer synchronize_rcu() here.
>>
>> Usually correctness (making sure all readers are done, before nuking a
>> data structure) is a little bit more important than perforamance, aka
>> "safety before speed" is what I've always been taught.
>>
> 
> Hmm, so you are saying synchronize_rcu() is kinda more correct
> than call_rcu()?? I never hear this before, would like to know why.
> 
> To my best knowledge, the only difference between them is the context,
> one is blocking, the other is non-blocking. Their correctness must be
> equivalent.
> 
We have burn our hands with blocking synchronize_rcu() and that was
actually the main reason we moved to rw locks from rcu to localise
the cost. call_rcu()should be just fine as long as it plugs the hole.
I don't want to add blocking in this path since this slows
down the connection shutdown path and at times lead to soft dumps.

Would you mind posting an updated patch please with call_rcu and
above extended RCU grace period with rcu_read_lock. Thanks !!

Regards,
Santosh

Regards,
Santosh