On Wed, Aug 21, 2019 at 02:53:29PM +0000, Jason Gunthorpe wrote:
> On Wed, Aug 21, 2019 at 05:21:06PM +0300, Yuval Shaia wrote:
> > From: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
> >
> > This new refcnt will points to the refcnt member of the HW object and will
> > behaves as expected by refcnt, i.e. will be increased and decreased as a
> > result of usage changes and will destroy the object when reaches to zero.
> > For a non-shared object refcnt will remain NULL.
> >
> > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
> > Signed-off-by: Shamir Rabinovitch <srabinov7@xxxxxxxxx>
> > drivers/infiniband/core/rdma_core.c | 23 +++++++++++++++++++++--
> > include/rdma/ib_verbs.h | 7 +++++++
> > 2 files changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> > index ccf4d069c25c..651625f632d7 100644
> > +++ b/drivers/infiniband/core/rdma_core.c
> > @@ -516,7 +516,26 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
> > const struct uverbs_obj_idr_type *idr_type =
> > container_of(uobj->uapi_object->type_attrs,
> > struct uverbs_obj_idr_type, type);
> > - int ret = idr_type->destroy_object(uobj, why, attrs);
> > + static DEFINE_MUTEX(lock);
> > + int ret, count;
> > +
> > + mutex_lock(&lock);
> > +
> > + if (uobj->refcnt) {
> > + count = atomic_dec_return(uobj->refcnt);
> > + WARN_ON(count < 0); /* use after free! */
>
> Use a proper refcount_t
uobj->refcnt points to HW object's refcnt (e.x ib_pd.refcnt)
>
> > + if (count) {
> > + mutex_unlock(&lock);
> > + goto skip;
> > + }
> > + }
> > +
> > + ret = idr_type->destroy_object(uobj, why, attrs);
> > +
> > + if (ret)
> > + atomic_inc(uobj->refcnt);
> > +
> > + mutex_unlock(&lock);
> >
> > /*
> > * We can only fail gracefully if the user requested to destroy the
> > @@ -525,7 +544,7 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
> > */
> > if (ib_is_destroy_retryable(ret, why, uobj))
> > return ret;
> > -
> > +skip:
>
> This doesn't seem to properly define who owns the rdmacg count - it
> should belong to the HW object not to the uobject.
>
> > +
> > + /*
> > + * ib_X HW object sharing support
> > + * - NULL for HW objects that are not shareable
> > + * - Pointer to ib_X reference counter for shareable HW objects
> > + */
> > + atomic_t *refcnt; /* ib_X object ref count */
>
> Gross, shouldn't this actually be in the hw object?
It is belongs to the HW object, this one just *points* to it and it is
defined here since we like to maintain it when destroy_hw_idr_uobject is
called.
The actual assignment is only for object that *supports* import and it is
set in function ib_uverbs_import_ib_pd (patch "IB/uverbs: Add PD import
verb").
Hope it make sense.
>
> Jason
