Re: [PATCH v10 06/26] RDMA/rtrs: client: main functionality

[Jinpu Wang <jinpu.wang@cloud.ionos.com>]

snip
> > > > > > +static void rtrs_clt_add_path_to_arr(struct rtrs_clt_sess *sess,
> > > > > > +                                   struct rtrs_addr *addr)
> > > > > > +{
> > > > > > +     struct rtrs_clt *clt = sess->clt;
> > > > > > +
> > > > > > +     mutex_lock(&clt->paths_mutex);
> > > > > > +     clt->paths_num++;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * Firstly increase paths_num, wait for GP and then
> > > > > > +      * add path to the list.  Why?  Since we add path with
> > > > > > +      * !CONNECTED state explanation is similar to what has
> > > > > > +      * been written in rtrs_clt_remove_path_from_arr().
> > > > > > +      */
> > > > > > +     synchronize_rcu();
> > > > >
> > > > > This makes no sense to me. RCU readers cannot observe the element in
> > > > > the list without also observing paths_num++
> > > > Paths_num is only used to make sure a reader doesn't look for a
> > > > CONNECTED path in the list for ever - instead he makes at most
> > > > paths_num attempts. The reader can in fact observe paths_num++ without
> > > > observing new element in the paths_list, but this is OK. When adding a
> > > > new path we first increase the paths_num and them add the element to
> > > > the list to make sure the reader will also iterate over it. When
> > > > removing the path - the logic is opposite: we first remove element
> > > > from the list and only then decrement the paths_num.
> > >
> > > I don't understand how this explains why synchronize_rcu would be need
> > > here.
> > It is needed here so that readers who read the old (smaller) value of
> > paths_num and are iterating over the list of paths will have a chance
> > to reach the new path we are about to insert. Basically it is here to
> > be symmetrical with the removal procedure: remove path,
> > syncronize_rcu, path_num--.
>
> How do readers see the paths_num before it is inserted into the list?
>
> Jason
We checked again, the synchronize_rcu indeed bogus, list_add_tail_rcu
has SMP barier,
so change will be visible to all CPU. We will remove it.

Thanks!