Re: [PATCH for-next 1/7] RDMA/bnxt_re: Refactor queue pair creation code

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jan 27, 2020 at 01:46:13PM +0530, Devesh Sharma wrote:
> On Mon, Jan 27, 2020 at 1:43 PM Devesh Sharma
> <devesh.sharma@broadcom.com> wrote:
> >
> > On Sat, Jan 25, 2020 at 11:16 PM Jason Gunthorpe <jgg@mellanox.com> wrote:
> > >
> > > On Fri, Jan 24, 2020 at 12:52:39AM -0500, Devesh Sharma wrote:
> > > > +static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
> > > > +{
> > > > +     struct bnxt_re_qp *gsi_sqp;
> > > > +     struct bnxt_re_ah *gsi_sah;
> > > > +     struct bnxt_re_dev *rdev;
> > > > +     int rc = 0;
> > > > +
> > > > +     rdev = qp->rdev;
> > > > +     gsi_sqp = rdev->gsi_ctx.gsi_sqp;
> > > > +     gsi_sah = rdev->gsi_ctx.gsi_sah;
> > > > +
> > > > +     /* remove from active qp list */
> > > > +     mutex_lock(&rdev->qp_lock);
> > > > +     list_del(&gsi_sqp->list);
> > > > +     atomic_dec(&rdev->qp_count);
> > > > +     mutex_unlock(&rdev->qp_lock);
> > > > +
> > > > +     dev_dbg(rdev_to_dev(rdev), "Destroy the shadow AH\n");
> > > > +     bnxt_qplib_destroy_ah(&rdev->qplib_res,
> > > > +                           &gsi_sah->qplib_ah,
> > > > +                           true);
> > > > +     bnxt_qplib_clean_qp(&qp->qplib_qp);
> > > > +
> > > > +     dev_dbg(rdev_to_dev(rdev), "Destroy the shadow QP\n");
> > > > +     rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &gsi_sqp->qplib_qp);
> > > > +     if (rc) {
> > > > +             dev_err(rdev_to_dev(rdev), "Destroy Shadow QP failed");
> > > > +             goto fail;
> > > > +     }
> > > > +     bnxt_qplib_free_qp_res(&rdev->qplib_res, &gsi_sqp->qplib_qp);
> > > > +
> > > > +     kfree(rdev->gsi_ctx.sqp_tbl);
> > > > +     kfree(gsi_sah);
> > > > +     kfree(gsi_sqp);
> > > > +     rdev->gsi_ctx.gsi_sqp = NULL;
> > > > +     rdev->gsi_ctx.gsi_sah = NULL;
> > > > +     rdev->gsi_ctx.sqp_tbl = NULL;
> > > > +
> > > > +     return 0;
> > > > +fail:
> > > > +     mutex_lock(&rdev->qp_lock);
> > > > +     list_add_tail(&gsi_sqp->list, &rdev->qp_list);
> > > > +     atomic_inc(&rdev->qp_count);
> > > > +     mutex_unlock(&rdev->qp_lock);
> > > > +     return rc;
> > >
> > > This error unwind approach looks racy. destroy is not allowed to
> > > fail, so why all this mess?
> > True, the unwind is not required, even if the driver wants to keep it
> > for debugging purpose, the zombie resource would give rise to
> > confusion.
> > >
> > > >  /* Queue Pairs */
> > > >  int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
> > > >  {
> > > > @@ -750,10 +797,18 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
> > > >       unsigned int flags;
> > > >       int rc;
> > > >
> > > > +     mutex_lock(&rdev->qp_lock);
> > > > +     list_del(&qp->list);
> > > > +     atomic_dec(&rdev->qp_count);
> > > > +     mutex_unlock(&rdev->qp_lock);
> > > >       bnxt_qplib_flush_cqn_wq(&qp->qplib_qp);
> > > >       rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp);
> > > >       if (rc) {
> > > >               dev_err(rdev_to_dev(rdev), "Failed to destroy HW QP");
> > > > +             mutex_lock(&rdev->qp_lock);
> > > > +             list_add_tail(&qp->list, &rdev->qp_list);
> > > > +             atomic_inc(&rdev->qp_count);
> > > > +             mutex_unlock(&rdev->qp_lock);
> > > >               return rc;
> > > >       }
> > >
> > > More..
> > Let me see if I can remove it in this series, else future series would
> > remove it.
> > >
> > > Jason
>
> At the top level, if provider driver is so keen on returning success
> in any case, should we change the return type to void of
> ib_destroy_xx() hooks?

We are doing it but in extremely slow way. Patches are welcomed.

Thanks