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

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Danil Kipnis <danil.kipnis@cloud.ionos.com>
Cc: Jack Wang <jinpu.wang@cloud.ionos.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Sagi Grimberg <sagi@grimberg.me>,
	Bart Van Assche <bvanassche@acm.org>,
	Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Roman Penyaev <rpenyaev@suse.de>,
	Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Subject: Re: [PATCH v10 06/26] RDMA/rtrs: client: main functionality
Date: Fri, 13 Mar 2020 09:25:46 -0300	[thread overview]
Message-ID: <20200313122546.GC31668@ziepe.ca> (raw)
In-Reply-To: <CAHg0HuxmjWu2V6gN=OTsv3v6aYxDkQN=z4F4gMYAu5Wwvp1qGg@mail.gmail.com>

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?

Jason