Commit f7c8416c authored by Jason Gunthorpe's avatar Jason Gunthorpe
Browse files

RDMA/core: Simplify destruction of FD uobjects 

FD uobjects have a weird split between the struct file and uobject
world. Simplify this to make them pure uobjects and use a generic release
method for all struct file operations.

This fixes the control flow so that mlx5_cmd_cleanup_async_ctx() is always
called before erasing the linked list contents to make the concurrancy
simpler to understand.

For this to work the uobject destruction must fence anything that it is
cleaning up - the design must not rely on struct file lifetime.

Only deliver_event() relies on the struct file to when adding new events
to the queue, add a is_destroyed check under lock to block it.

Link: https://lore.kernel.org/r/1578504126-9400-3-git-send-email-yishaih@mellanox.com


Signed-off-by: default avatarYishai Hadas <yishaih@mellanox.com>
Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
parent 6898d1c6

drivers/infiniband/core/rdma_core.c

+21 −13
Original line number Diff line number Diff line
@@ -353,9 +353,9 @@ lookup_get_fd_uobject(const struct uverbs_api_object *obj, 


	uobject = f->private_data;

	/*

	 * fget(id) ensures we are not currently running uverbs_close_fd,

	 * and the caller is expected to ensure that uverbs_close_fd is never

	 * done while a call top lookup is possible.

	 * fget(id) ensures we are not currently running

	 * uverbs_uobject_fd_release(), and the caller is expected to ensure

	 * that release is never done while a call to lookup is possible.

	 */

	if (f->f_op != fd_type->fops) {

		fput(f);
@@ -548,7 +548,7 @@ static int __must_check destroy_hw_fd_uobject(struct ib_uobject *uobj, 
{

	const struct uverbs_obj_fd_type *fd_type = container_of(

		uobj->uapi_object->type_attrs, struct uverbs_obj_fd_type, type);

	int ret = fd_type->context_closed(uobj, why);

	int ret = fd_type->destroy_object(uobj, why);



	if (ib_is_destroy_retryable(ret, why, uobj))

		return ret;
@@ -587,9 +587,9 @@ static int alloc_commit_fd_uobject(struct ib_uobject *uobj) 


	/*

	 * The kref for uobj is moved into filp->private data and put in

	 * uverbs_close_fd(). Once alloc_commit() succeeds uverbs_close_fd()

	 * must be guaranteed to be called from the provided fops release

	 * callback.

	 * uverbs_close_fd(). Once alloc_commit() succeeds

	 * uverbs_uobject_fd_release() must be guaranteed to be called from

	 * the provided fops release callback.

	 */

	filp = anon_inode_getfile(fd_type->name,

				  fd_type->fops,
@@ -600,7 +600,7 @@ static int alloc_commit_fd_uobject(struct ib_uobject *uobj) 


	uobj->object = filp;



	/* Matching put will be done in uverbs_close_fd() */

	/* Matching put will be done in uverbs_uobject_fd_release() */

	kref_get(&uobj->ufile->ref);



	/* This shouldn't be used anymore. Use the file object instead */
@@ -608,7 +608,7 @@ static int alloc_commit_fd_uobject(struct ib_uobject *uobj) 


	/*

	 * NOTE: Once we install the file we loose ownership of our kref on

	 * uobj. It will be put by uverbs_close_fd()

	 * uobj. It will be put by uverbs_uobject_fd_release()

	 */

	fd_install(fd, filp);
@@ -676,7 +676,10 @@ static void lookup_put_fd_uobject(struct ib_uobject *uobj, 
	struct file *filp = uobj->object;



	WARN_ON(mode != UVERBS_LOOKUP_READ);

	/* This indirectly calls uverbs_close_fd and free the object */

	/*

	 * This indirectly calls uverbs_uobject_fd_release() and free the

	 * object

	 */

	fput(filp);

}
@@ -742,9 +745,13 @@ const struct uverbs_obj_type_class uverbs_idr_class = { 
};

EXPORT_SYMBOL(uverbs_idr_class);



void uverbs_close_fd(struct file *f)

/*

 * Users of UVERBS_TYPE_ALLOC_FD should set this function as the struct

 * file_operations release method.

 */

int uverbs_uobject_fd_release(struct inode *inode, struct file *filp)

{

	struct ib_uobject *uobj = f->private_data;

	struct ib_uobject *uobj = filp->private_data;

	struct ib_uverbs_file *ufile = uobj->ufile;

	struct uverbs_attr_bundle attrs = {

		.context = uobj->context,
@@ -768,8 +775,9 @@ void uverbs_close_fd(struct file *f) 


	/* Pairs with filp->private_data in alloc_begin_fd_uobject */

	uverbs_uobject_put(uobj);

	return 0;

}

EXPORT_SYMBOL(uverbs_close_fd);

EXPORT_SYMBOL(uverbs_uobject_fd_release);



/*

 * Drop the ucontext off the ufile and completely disconnect it from the

drivers/infiniband/core/rdma_core.h

+0 −8
Original line number Diff line number Diff line
@@ -50,14 +50,6 @@ void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile, 


int uobj_destroy(struct ib_uobject *uobj, struct uverbs_attr_bundle *attrs);



/* Indicate this fd is no longer used by this consumer, but its memory isn't

 * necessarily released yet. When the last reference is put, we release the

 * memory. After this call is executed, calling uverbs_uobject_get isn't

 * allowed.

 * This must be called from the release file_operations of the file!

 */

void uverbs_close_fd(struct file *f);



/*

 * Get an ib_uobject that corresponds to the given id from ufile, assuming

 * the object is from the given type. Lock it to the required access when

drivers/infiniband/core/uverbs_main.c

+1 −22
Original line number Diff line number Diff line
@@ -373,32 +373,11 @@ static int ib_uverbs_async_event_close(struct inode *inode, struct file *filp) 
	return 0;

}



static int ib_uverbs_comp_event_close(struct inode *inode, struct file *filp)

{

	struct ib_uobject *uobj = filp->private_data;

	struct ib_uverbs_completion_event_file *file = container_of(

		uobj, struct ib_uverbs_completion_event_file, uobj);

	struct ib_uverbs_event *entry, *tmp;



	spin_lock_irq(&file->ev_queue.lock);

	list_for_each_entry_safe(entry, tmp, &file->ev_queue.event_list, list) {

		if (entry->counter)

			list_del(&entry->obj_list);

		kfree(entry);

	}

	file->ev_queue.is_closed = 1;

	spin_unlock_irq(&file->ev_queue.lock);



	uverbs_close_fd(filp);



	return 0;

}



const struct file_operations uverbs_event_fops = {

	.owner	 = THIS_MODULE,

	.read	 = ib_uverbs_comp_event_read,

	.poll    = ib_uverbs_comp_event_poll,

	.release = ib_uverbs_comp_event_close,

	.release = uverbs_uobject_fd_release,

	.fasync  = ib_uverbs_comp_event_fasync,

	.llseek	 = no_llseek,

};

drivers/infiniband/core/uverbs_std_types.c

+15 −8
Original line number Diff line number Diff line
@@ -202,22 +202,29 @@ static int uverbs_free_pd(struct ib_uobject *uobject, 
	return 0;

}



static int uverbs_hot_unplug_completion_event_file(struct ib_uobject *uobj,

static int

uverbs_completion_event_file_destroy_uobj(struct ib_uobject *uobj,

					  enum rdma_remove_reason why)

{

	struct ib_uverbs_completion_event_file *comp_event_file =

	struct ib_uverbs_completion_event_file *file =

		container_of(uobj, struct ib_uverbs_completion_event_file,

			     uobj);

	struct ib_uverbs_event_queue *event_queue = &comp_event_file->ev_queue;

	struct ib_uverbs_event_queue *event_queue = &file->ev_queue;

	struct ib_uverbs_event *entry, *tmp;



	spin_lock_irq(&event_queue->lock);

	event_queue->is_closed = 1;

	spin_unlock_irq(&event_queue->lock);



	if (why == RDMA_REMOVE_DRIVER_REMOVE) {

	wake_up_interruptible(&event_queue->poll_wait);

	kill_fasync(&event_queue->async_queue, SIGIO, POLL_IN);



	spin_lock_irq(&event_queue->lock);

	list_for_each_entry_safe(entry, tmp, &event_queue->event_list, list) {

		if (entry->counter)

			list_del(&entry->obj_list);

		kfree(entry);

	}

	spin_unlock_irq(&event_queue->lock);

	return 0;

};
@@ -230,7 +237,7 @@ EXPORT_SYMBOL(uverbs_destroy_def_handler); 
DECLARE_UVERBS_NAMED_OBJECT(

	UVERBS_OBJECT_COMP_CHANNEL,

	UVERBS_TYPE_ALLOC_FD(sizeof(struct ib_uverbs_completion_event_file),

			     uverbs_hot_unplug_completion_event_file,

			     uverbs_completion_event_file_destroy_uobj,

			     &uverbs_event_fops,

			     "[infinibandevent]",

			     O_RDONLY));

drivers/infiniband/core/uverbs_uapi.c

+3 −3
Original line number Diff line number Diff line
@@ -195,9 +195,9 @@ static int uapi_merge_obj_tree(struct uverbs_api *uapi, 
		 * disassociation, and the FD types require the driver to use

		 * struct file_operations.owner to prevent the driver module

		 * code from unloading while the file is open. This provides

		 * enough safety that uverbs_close_fd() will continue to work.

		 * Drivers using FD are responsible to handle disassociation of

		 * the device on their own.

		 * enough safety that uverbs_uobject_fd_release() will

		 * continue to work.  Drivers using FD are responsible to

		 * handle disassociation of the device on their own.

		 */

		if (WARN_ON(is_driver &&

			    obj->type_attrs->type_class != &uverbs_idr_class &&

CRA Git | Maintained and supported by SUSTech CRA and CCSE