On Thu, Jun 28, 2018 at 07:42:50AM +0300, Leon Romanovsky wrote:
> 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.
Such is the kernel development..
In this case the function is encompassing a locking invariant as well
and should evolve to include a lock asserted test.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
