On Fri, Mar 13, 2020 at 1:25 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> On Fri, Mar 13, 2020 at 01:18:23PM +0100, Danil Kipnis wrote:
> > > > > calling rcu list iteration without holding rcu_lock is wrong
> > > > This function (add_path) along with the corresponding
> > > > remove_path_from_arr() are the only functions modifying the
> > > > paths_list. In both functions paths_mutex is taken so that they are
> > > > serialized. Since the modification of the paths_list is protected by
> > > > the mutex, the rcu_read_lock is superfluous here.
> > >
> > > Then don't use the _rcu functions.
> > We need to traverse rcu list in the update-side of the code. According
> > to the whatisRCU.rst "if list_for_each_entry_rcu() instance might be
> > used by update-side code...then an additional lockdep expression can
> > be added to its list of arguments..." The would be our case since we
> > always hold a lock when doing this, but I don't see a corresponding
> > API. We can just surround the statement with
> > rcu_readlock/rcu_readunlock to avoid the warning.
>
> The only case where you can avoid RCU is if the code is already
> holding a lock preventing writes to the list, in which case you use
> the normal list iterator.
>
> > > > > > + /*
> > > > > > + * @pcpu paths can still point to the path which is going to be
> > > > > > + * removed, so change the pointer manually.
> > > > > > + */
> > > > > > + for_each_possible_cpu(cpu) {
> > > > > > + struct rtrs_clt_sess __rcu **ppcpu_path;
> > > > > > +
> > > > > > + ppcpu_path = per_cpu_ptr(clt->pcpu_path, cpu);
> > > > > > + if (rcu_dereference(*ppcpu_path) != sess)
> > > > >
> > > > > calling rcu_dereference without holding rcu_lock is wrong.
> > > > We only need a READ_ONCE semantic here. ppcpu_path is pointing to the
> > > > last path used for an IO and is used for the round robin multipath
> > > > policy. I guess the call can be changed to rcu_dereference_raw to
> > > > avoid rcu_lockdep warning. The round-robin algorithm has been reviewed
> > > > by Paul E. McKenney, he wrote a litmus test for it:
> > > > https://lkml.org/lkml/2018/5/28/2080.
> > >
> > > You can't call rcu expecting functions without holding the rcu lock -
> > > use READ_ONCE/etc if that is what is really going on
>
> > Look's people are using rcu_dereference_protected when dereferencing
> > rcu pointer in update-side and have an explicit lock to protect it, as
> > we do. Will dig into it next week.
>
> Yes, that is right too
>
> > > > > > +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?
We increase paths_num and insert element into the list while holding
paths_mutex. The readers only do rcu_readlock and rcu_readunlock when
iterating over the list, they do not take the paths_mutex, so there is
a window where they can see the increased paths_num, but the element
isn't yet in the list...
>
> Jason
