Re: [PATCH V1 for-next 4/7] IB/core: Change idr objects to use the new schema

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Wed, Feb 01, 2017 at 02:39:02PM +0200, Matan Barak wrote:

> +#define idr_get_xxxx(_type, _write, _handle, _context) ({		\
> +	const struct uverbs_obj_idr_type * const type =			\
> +			&uverbs_type_attrs_##_type;			\
> +	struct ib_uobject *uobj = type->type.ops->lookup_get(&type->type, \
> +					_context, _handle, _write);	\
> +									\
> +	IS_ERR(uobj) ? NULL : uobj->object; })

I think you should follow the usual convention and have a full
suite of accessor helpers in a header. This header could even be in
patch #2

static struct ib_uobject *__uobj_get(const struct uverbs_obj_type *type, bool write, struct ib_ucontext *ucontext, int id)
{
  [..]
}
#define uobj_get_read(_type,...) ((struct ib_##_type *)_uobj_get(&uverbs_type_attrs_##_type, false, __VA_ARGS__))
#define uobj_get_write(_type,...) ((struct ib_##_type *)_uobj_get(&uverbs_type_attrs_##_type, false, __VA_ARGS__))

static inline void uobj_put_write(struct ib_uobject *uobj)
{
    uobj->type->ops->lookup_put(uobj, true);
}

static inline void uobj_put_read(struct ib_uobject *uobj)
{
    uobj->type->ops->lookup_put(uobj, false);
}

static inline struct ib_uobject *__uobj_alloc(const struct uverbs_obj_type *type,struct ib_ucontext *ucontext)
{
   return type->alloc_begin(type, ucontext);
}

#define uobj_alloc_begin(_type, ...) ((struct ib_##_type *)_uobj_alloc(&uverbs_type_attrs_##_type, __VA_ARGS__))

and so on

>  static struct ib_pd *idr_read_pd(int pd_handle, struct ib_ucontext *context)
>  {
> -	return idr_read_obj(pd_handle, context, 0);
> +	return idr_get_xxxx(pd, false, pd_handle, context);
>  }

And you can just totally drop all of these inlines and use

  uobj_get_read(pd, pd_handle, context);

At the call site.

>  static struct ib_xrcd *idr_read_xrcd(int xrcd_handle, struct ib_ucontext *context,
>  				     struct ib_uobject **uobj)
>  {
> -	*uobj = idr_read_uobj(xrcd_handle, context, 0);
> -	return *uobj ? (*uobj)->object : NULL;
> +	*uobj = uverbs_type_attrs_xrcd.type.ops->lookup_get(&uverbs_type_attrs_xrcd.type,
> +							context, xrcd_handle,
> +							false);
> +	return IS_ERR(*uobj) ? NULL : (*uobj)->object;

Why didn't this use idr_get_xxx ? Why is it so weird?

>  
> @@ -621,9 +439,11 @@ ssize_t ib_uverbs_dealloc_pd(struct ib_uverbs_file *file,
>  	if (copy_from_user(&cmd, buf, sizeof cmd))
>  		return -EFAULT;
>  
> -	uobj = idr_write_uobj(cmd.pd_handle, file->ucontext);
> -	if (!uobj)
> -		return -EINVAL;
> +	uobj = uverbs_type_attrs_pd.type.ops->lookup_get(&uverbs_type_attrs_pd.type,
> +						     file->ucontext,
> +						     cmd.pd_handle, true);
> +	if (IS_ERR(uobj))
> +		return PTR_ERR(uobj);
>  	pd = uobj->object;
>  
>  	if (atomic_read(&pd->usecnt)) {
> @@ -636,21 +456,12 @@ ssize_t ib_uverbs_dealloc_pd(struct ib_uverbs_file *file,
>  	if (ret)
>  		goto err_put;
>  
> -	uobj->live = 0;
> -	put_uobj_write(uobj);
> -
> -	idr_remove_uobj(uobj);
> -
> -	mutex_lock(&file->mutex);
> -	list_del(&uobj->list);
> -	mutex_unlock(&file->mutex);
> -
> -	put_uobj(uobj);
> +	uobj->type->ops->destroy_commit(uobj);

And here I think you should move the pd->device_dealloc_pd into
destroy_commit by having it call what you called hot_plug, and turn
this simply into:

	return uobj_remove_or_put(uobj, RDMA_REMOVE_DESTROY);

static inline int uobj_remove_or_put(struct ib_uobject *uobj, enum rdma_remove_reason why)
{
    int rc = uobj->type->ops->remove(uobj, why);
    if (!rc)
        uverbs_uobject_put(uobj);

    /* If we are using a forced destruction mode then the driver is
        not permitted to fail. */
    WARN_ON(!rc && why != RDMA_REMOVE_DESTROY);
    return rc;
}

And generally use this pattern for all the destroy verbs.

I didn't study this patch closely since it will change a lot, but I
think this:

 4 files changed, 260 insertions(+), 788 deletions(-)

Pretty much confirms this is the right overall approach.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html