Re: [PATCH for-next 12/22] RDMA/rtrs-clt: Check state of the rtrs_clt_sess before reading its stats

From: Jinpu Wang <jinpu.wang@ionos.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Gioh Kim <gi-oh.kim@ionos.com>,
	linux-rdma@vger.kernel.org, Bart Van Assche <bvanassche@acm.org>,
	Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Haris Iqbal <haris.iqbal@ionos.com>,
	Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
Subject: Re: [PATCH for-next 12/22] RDMA/rtrs-clt: Check state of the rtrs_clt_sess before reading its stats
Date: Thu, 8 Apr 2021 15:45:45 +0200	[thread overview]
Message-ID: <CAMGffE=pu7uhmsBaYBuZB2w+YOogrK+W5yEKRPZxTanx5+f0Gg@mail.gmail.com> (raw)
In-Reply-To: <20210408120418.GQ7405@nvidia.com>

On Thu, Apr 8, 2021 at 2:04 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Apr 06, 2021 at 10:55:59AM +0200, Gioh Kim wrote:
> > On Thu, Apr 1, 2021 at 8:44 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Thu, Mar 25, 2021 at 04:32:58PM +0100, Gioh Kim wrote:
> > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > index 42f49208b8f7..1519191d7154 100644
> > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > @@ -808,6 +808,9 @@ static struct rtrs_clt_sess *get_next_path_min_inflight(struct path_it *it)
> > > >       int inflight;
> > > >
> > > >       list_for_each_entry_rcu(sess, &clt->paths_list, s.entry) {
> > > > +             if (unlikely(READ_ONCE(sess->state) != RTRS_CLT_CONNECTED))
> > > > +                     continue;
> > >
> > > There is no way this could be right, a READ_ONCE can't guarentee that
> > > a following load is not going to happen without races.
> > >
> > > You need locking.
> >
> > Hi Jason,
> >
> > rtrs_clt_request() calls rcu_read_lock() before calling
> > get_next_path_min_inflight().
> > And rtrs_clt_change_state_from_to(), that changes the sess->state,
> > calls spin_lock_irq() before changing it.
> > I think that is enough, isn't it?
>
> Why would that be enough?
>
> Under RCU this check is racy and effetively does nothing.
>
> This is an OK usage of RCU:
>
>         list_del_rcu(&sess->s.entry);
>
>         /* Make sure everybody observes path removal. */
>         synchronize_rcu();
>
> And you could say that observing the sess in the list is required, but
> checking state is pointless.
>
> Jason
Hi Jason

Sending IO via disconnected session is not a problem, it will just get
an error. It's only about if in the meantime user delete the path
while IO running, and session is freed.

There is session state machine it will go CONNECTED to CLOSED will be
a few steps, disconnect, drain QP/etc,  the cpu will get the latest
state sooner or later.
We don't really care if there's a few cycles sess->state is no up to date.

Regards!