Re: [PATCH for-rc 4/6] RDMA/bnxt_re: Synchronize destroy_qp with poll_cq

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2896 bytes --]

On Fri, Feb 16, 2018 at 05:09:15PM +0530, Selvin Xavier wrote:
> On Fri, Feb 16, 2018 at 1:08 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Feb 15, 2018 at 09:20:11PM -0800, Selvin Xavier wrote:
> >> Avoid system crash when destroy_qp is invoked while
> >> the driver is processing the poll_cq. Synchronize these
> >> functions using the cq_lock.
> >>
> >> Signed-off-by: Selvin Xavier <selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> >> ---
> >>  drivers/infiniband/hw/bnxt_re/ib_verbs.c | 39 +++++++++++++++++++++++++++++---
> >>  drivers/infiniband/hw/bnxt_re/ib_verbs.h |  2 ++
> >>  drivers/infiniband/hw/bnxt_re/qplib_fp.c | 21 +++++------------
> >>  drivers/infiniband/hw/bnxt_re/qplib_fp.h |  4 +++-
> >>  4 files changed, 47 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> index b7eb104..9cb9928 100644
> >> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> @@ -786,20 +786,51 @@ int bnxt_re_query_ah(struct ib_ah *ib_ah, struct rdma_ah_attr *ah_attr)
> >>       return 0;
> >>  }
> >>
> >> +static unsigned long bnxt_re_lock_cqs(struct bnxt_re_qp *qp)
> >> +     __acquires(&qp->scq->cq_lock) __acquires(&qp->rcq->cq_lock)
> >> +{
> >> +     unsigned long flags;
> >> +
> >> +     spin_lock_irqsave(&qp->scq->cq_lock, flags);
> >> +     if (qp->rcq != qp->scq)
> >> +             spin_lock(&qp->rcq->cq_lock);
> >> +     else
> >> +             __acquire(&qp->rcq->cq_lock);
> >
> > Sorry for naive question, why do you need __acquire/__release? And why
> > can't spin_is_locked(&qp->rcq->cq_lock) be used for unlock code?
> >
> I used __acquire and __release for satisfying the sparse. Sparse used to
> give warnings  "context  imbalance - wrong count at exit" since the locking
> and unlocking happens from two different functions. seen similar implementation
> in other drivers too.

I see, actually I think that sparse complain was right and you went to
far by wrapping standard calls. I see that it is very popular in bnxt_re
code and the problem with that, that it makes very hard to refactor such
code in case of some global change/improvement.

>
> I feel spin_is_locked also can be used.. but it doesn't avoid the RCQ
> != SCQ check.
> Also, if feel the code symmetry is better in this way.
> >
> >> +
> >> +     return flags;
> >> +}
> >> +
> >> +static void bnxt_re_unlock_cqs(struct bnxt_re_qp *qp,
> >> +                            unsigned long flags)
> >> +     __releases(&qp->scq->cq_lock) __releases(&qp->rcq->cq_lock)
> >> +{
> >> +     if (qp->rcq != qp->scq)
> >> +             spin_unlock(&qp->rcq->cq_lock);
> >> +     else
> >> +             __release(&qp->rcq->cq_lock);
> >> +     spin_unlock_irqrestore(&qp->scq->cq_lock, flags);
> >> +}
> >> +
> >
> > Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]