On Wed, Jun 27, 2018 at 05:06:43PM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 20, 2018 at 05:11:39PM +0300, Leon Romanovsky wrote:
> > diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> > index 0df0ac9c1de3..6497263d13c8 100644
> > +++ b/drivers/infiniband/core/uverbs_std_types.c
> > @@ -72,17 +72,16 @@ static int uverbs_free_qp(struct ib_uobject *uobject,
> > struct ib_qp *qp = uobject->object;
> > struct ib_uqp_object *uqp =
> > container_of(uobject, struct ib_uqp_object, uevent.uobject);
> > - int ret;
> > + int ret = list_empty(&uqp->mcast_list) ? 0 : -EBUSY;
> > +
> > + if (ib_is_destroy_retryable(ret, why, uobject))
> > + return ret;
> >
> > - if (why == RDMA_REMOVE_DESTROY) {
> > - if (!list_empty(&uqp->mcast_list))
> > - return -EBUSY;
> > - } else if (qp == qp->real_qp) {
> > + if (qp == qp->real_qp)
> > ib_uverbs_detach_umcast(qp, uqp);
> > - }
>
> This is super tricky, but I don't think this change is right.
>
> If and only if we are doing a user triggered destroy (eg
> RDMA_REMOVE_DESTROY) should we check the mcast list.
>
> The items on the mcast list are not uobjects, so they will not be
> destroyed via the cleanup loop.
>
> If we are doing RDMA_REMOVE_DRIVER_REMOVE or RDMA_REMOVE_CLOSE then we
> must always tear down any mcast attachment, free the list and attempt
> to destroy the HW object.
>
> ie a driver could legitimately block QP destruction because it has
> attached mcast's, which would cause the new iteration scheme to fail.
>
> > @@ -150,13 +155,11 @@ static int uverbs_free_xrcd(struct ib_uobject *uobject,
> > struct ib_xrcd *xrcd = uobject->object;
> > struct ib_uxrcd_object *uxrcd =
> > container_of(uobject, struct ib_uxrcd_object, uobject);
> > - int ret;
> > + int ret = atomic_read(&uxrcd->refcnt) ? -EBUSY : 0;
>
> I counted 5 of these, and it is a fairly ugly expression.
>
> int ret;
>
> ret = ib_destroy_usecnt(&uxrcd->refcnt, why, uobject);
> if (ret)
> return ret;
>
> is tidier and clearer.
Ugh, all those macros and extra wrappers make our stack completely
indigestible and hard to maintain. I already lost with endless number of
them just to hide some kernel primitives and many of my reviews are
tedious task to understand if new code fits or not fits some new cool
wrapper introduced exactly X minutes ago.
>
> Anyhow, I made the above edits to this patch, but the diff is a bit
> big so I will keep it on the wip branch until I hear from you.
Most probably Yishai will respond on that.
Thanks
Attachment:
signature.asc
Description: PGP signature
