Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <161721926778.515226.9805598788670386587.stgit@manet.1015granger.net>
 <161721937122.515226.14731175629421422152.stgit@manet.1015granger.net>
 <4004f56f-3603-f56c-aea9-651230b3181e@talpey.com> <CAFMMQGvortADqgmAzskZKcnyHDzsTEW0FtR501wpP+deUM57FA@mail.gmail.com>
 <b5286b3f-439a-ef7c-cd7c-6cebfbccb02d@talpey.com>
In-Reply-To: <b5286b3f-439a-ef7c-cd7c-6cebfbccb02d@talpey.com>
From:   Chuck Lever <chucklever@gmail.com>
Date:   Thu, 1 Apr 2021 12:56:17 -0400
Message-ID: <CAFMMQGuo6=Leymd++YBtAwQzptzp+rcynM6EXosu20vG67jrMw@mail.gmail.com>
Subject: Re: [PATCH v1 2/8] xprtrdma: Do not post Receives after disconnect
To:     Tom Talpey <tom@talpey.com>
Cc:     linux-rdma <linux-rdma@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Mar 31, 2021 at 5:22 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 3/31/2021 4:31 PM, Chuck Lever wrote:
> > On Wed, Mar 31, 2021 at 4:01 PM Tom Talpey <tom@talpey.com> wrote:
> >>
> >> On 3/31/2021 3:36 PM, Chuck Lever wrote:
> >>> Currently the Receive completion handler refreshes the Receive Queue
> >>> whenever a successful Receive completion occurs.
> >>>
> >>> On disconnect, xprtrdma drains the Receive Queue. The first few
> >>> Receive completions after a disconnect are typically successful,
> >>> until the first flushed Receive.
> snip
> >> Is it not possible to mark the WRs as
> >> being part of a batch, and allowing them to flush? You could borrow a
> >> bit in the completion cookie, and check it when the CQE pops out. Maybe.
> >
> > It's not an issue with batching, it's an issue with posting Receives from the
> > Receive completion handler. I'd think that any of the ULPs that post Receives
> > in their completion handler would have the same issue.
> >
> > The purpose of the QP drain in rpcrdma_xprt_disconnect() is to ensure there
> > are no more WRs in flight so that the hardware resources can be safely
> > destroyed. If the Receive completion handler continues to post Receive WRs
> > after the drain sentinel has been posted, leaks and crashes become possible.

> Well, why not do an atomic_set() of a flag just before posting the
> sentinel, and check it with atomic_get() before any other RQ post?

After a couple of private exchanges, Tom and I agree that doing an
ib_query_qp() in rpcrdma_post_recvs() has some drawbacks and
does not fully close the race window.

There is nothing that prevents rpcrdma_xprt_disconnect() from starting
the RQ drain while rpcrdma_post_recvs is still running. There needs to
be serialization between ib_drain_rq() and rpcrdma_post_recvs() so
that the drain sentinel is guaranteed to be the final Receive WR posted
on the RQ.

It would be great if the core ib_drain_rq() API itself could handle the
exclusion.


-- 
"We cannot take our next breath without the exhale."
 -- Ellen Scott Grable