On Mon, Sep 10, 2018 at 5:04 PM Sowmini Varadhan <sowmini.varad...@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. 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.
