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

[Cong Wang <xiyou.wangcong@gmail.com>]

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.

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.