| Message ID | 20190821142125.5706-6-yuval.shaia@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Shared PD and MR | expand |
On Wed, Aug 21, 2019 at 05:21:06PM +0300, Yuval Shaia wrote: > From: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > 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@oracle.com> > Signed-off-by: Shamir Rabinovitch <srabinov7@gmail.com> > 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 > + 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? Jason
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@oracle.com> > > > > 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@oracle.com> > > Signed-off-by: Shamir Rabinovitch <srabinov7@gmail.com> > > 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
On Tue, Aug 27, 2019 at 07:28:14PM +0300, Yuval Shaia wrote: > 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@oracle.com> > > > > > > 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@oracle.com> > > > Signed-off-by: Shamir Rabinovitch <srabinov7@gmail.com> > > > 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) Hurm. That refcount is kind of broken/racey as is, I'm not clear if it can be used for this. More changes would probably be needed.. It would be more understandable to start with a dedicated refcount and then have a patch to consolidate > > > + > > > + /* > > > + * 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. I mean, you should just extract it from the hw_object directly, some how. Ie have a get_refcount accessor in the object definition. Jason
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c index ccf4d069c25c..651625f632d7 100644 --- a/drivers/infiniband/core/rdma_core.c +++ 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! */ + 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: if (why == RDMA_REMOVE_ABORT) return 0; diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 7b429b2e7cf6..7e69866fc419 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1496,6 +1496,13 @@ struct ib_uobject { struct rcu_head rcu; /* kfree_rcu() overhead */ const struct uverbs_api_object *uapi_object; + + /* + * 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 */ }; struct ib_udata {