[ofa-general] Re: [PATCH 8/11] core: XRC receive-only QPs

[Jack Morgenstein]

On Wednesday 02 July 2008 18:04, Jack Morgenstein wrote:
> On Tuesday 24 June 2008 00:53, Roland Dreier wrote:
> >  > @@ -769,6 +775,7 @@ struct ib_ucontext {
> >  >  	struct list_head	srq_list;
> >  >  	struct list_head	ah_list;
> >  >  	struct list_head	xrc_domain_list;
> >  > +	struct list_head	xrc_reg_qp_list;
> >  >  	int			closing;
> >  >  };
> > 
> > Wouldn't it be cleaner to keep the like of recv QPs per xrcd?  Then you
> > wouldn't have to do stuff like:
> > 
> >  > +	list_for_each_entry(tmp, &file->ucontext->xrc_reg_qp_list, list)
> >  > +		if (cmd.xrcd_handle == tmp->domain_handle) {
> > 
> > instead you could just do a list_empty() test.
> > 
> Currently, there is no per-process object for xrc domains. I needed a list
> of qp's registered ***per-process***, so that I could prevent closing a domain (for that process)
> while that process still had registered QPs on that domain.  This was especially true if
> the current process was the last one using the domain -- so that the domain itself would be deleted
> when close_domain was invoked.
> 
> With the check in place, if the user tried to close the domain while there were QPs registered,
> he gets an error.  If the user does not close the domain, and does not unregister all the QPs
> for that process, this gets done in ib_uverbs_cleanup_ucontext() (in the proper order), so there
> are no resource leaks.
> 
> I think that here we have "6 of one, half a dozen of the other":
> If I register QPs per xrcd, I either have to create and manage an xrcd
> user-space specific object (e.g., struct ib_uxrcd_object) -- instead of the generic ib_uobject,
> or I need to save the process ID per QP in an xrcd list of QPs -- and then I would need to
> check for equality of PIDs, rather than domain handles.
> 
> I think the way things are now is relatively simpler.

On second thought, I'll look at this again for a day or two.  It is a medium-complexity code change, which
needs to be thoroughly tested out.

I'll need to define an ib_uxrcd_object, and place the xrc_reg_qp_list in that uobject.  When cleaning up the
process, I'll just need to unregister all the xrc_rcv_qp's in a loop within the xrc domain cleanup loop
(i.e.	list_for_each_entry_safe(uobj, tmp, &context->xrc_domain_list, list) {
		struct ib_xrcd *xrcd = uobj->object;
===> xrc_rcv_qp list cleanup here, in a "for each" loop
		idr_remove_uobj(&ib_uverbs_xrc_domain_idr, uobj);
		ib_uverbs_dealloc_xrcd(file->device->ib_dev, xrcd);
		kfree(uobj);
	}
	mutex_unlock(&file->device->ib_dev->xrcd_table_mutex);
)

When registering/unregistering xrc_rcv_qp's, I need to manage the qp list within the ib_uxrcd_object.

I do agree that this would be logically cleaner, since this way the receive qp's per process xrc domain
are contained in the process' xrc domain object.

- Jack