Re: [PATCH V2 for-next 2/7] IB/core: Add support for idr types

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

On Sun, Mar 19, 2017 at 05:59:00PM +0200, Matan Barak wrote:

> +static int uverbs_try_lock_object(struct ib_uobject *uobj, bool write)
> +{
> +	/*
> +	 * When a read is required, we use a positive counter. Each read
> +	 * request checks that the value != -1 and increment it. Write
> +	 * requires an exclusive access, thus we check that the counter is
> +	 * zero (nobody claimed this object) and we set it to -1.
> +	 * Releasing a read lock is done by simply decreasing the counter.
> +	 * As for writes, since only a single write is permitted, setting
> +	 * it to zero is enough for releasing it.
> +	 */
> +	if (!write)
> +		return __atomic_add_unless(&uobj->usecnt, 1, -1) == -1 ?
> +			-EBUSY : 0;
> +
> +	/* lock is either WRITE or DESTROY - should be exclusive */
> +	return atomic_cmpxchg(&uobj->usecnt, 0, -1) == 0 ? 0 : -EBUSY;

I wonder if a WARN_ON should be here, is it ever not an error to try
to write lock twice?

> +static struct ib_uobject *alloc_uobj(struct ib_ucontext *context,
> +				     const struct uverbs_obj_type *type)
> +{
> +	struct ib_uobject *uobj = kmalloc(type->obj_size, GFP_KERNEL);

Do you think this should be kzalloc?

> +/* Returns the ib_uobject or an error. The caller should check for IS_ERR. */
> +static struct ib_uobject *lookup_get_idr_uobject(const struct uverbs_obj_type *type,
> +						 struct ib_ucontext *ucontext,
> +						 int id, bool write)
> +{
> +	struct ib_uobject *uobj;
> +
> +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> +			 "ib_uverbs: lookup_get_idr_uobject is called without proper synchronization");
> +	/* object won't be released as we're protected in rcu */
> +	uobj = idr_find(&ucontext->ufile->idr, id);
> +	if (!uobj)
> +		return ERR_PTR(-ENOENT);
> +

A function called lookup_get should also do the get. I think this
would look iodmatically better like:

	rcu_read_lock();
	uobj = idr_find(&ucontext->ufile->idr, id);
	if (uobj)
		uverbs_uobject_get(uobj);
	rcu_read_unlock();

And drop the overlap from rdma_lookup_get_uobject

> +	if (uobj->type != type)
> +		return ERR_PTR(-EINVAL);
> +

This should probably be done in rdma_lookup_get_uobject?

> +/*
> + * Objects type classes which support a detach state (object is still alive but
> + * it's not attached to any context need to make sure:
> + * (a) If function pointers are used in any modules, module_get was called on
> + *     this module.
> + * (b) detach isn't called concurrently with context_cleanup
> + */

Little confused about this comment on module_get.. Our typical
strategy has been to forbid many kinds of cross-module calls during
detatchment. eg by setting function pointers to null. That is a robust
approach, I would not like to see more module_gets  ..

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