Re: [PATCH V2 1/4] ib_core: changes to support XRC RCV qp's

[Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On Tuesday 18 May 2010 21:33, Sean Hefty wrote:
> >+struct ib_xrc_rcv_qp_table_entry *
> >+ib_xrc_rcv_tbl_find(struct ib_device *dev, u32 qpn)
> >+{
> >+        return radix_tree_lookup(&dev->xrc_rcv_qp_table, qpn);
> >+}
> 
> nit - but do we need a wrapper around this single call?
I prefer it this way, so that we have a complete xrc_rcv_table interface.
 
> >+	list_for_each_entry(tmp, &qp->list, list)
> >+		if (tmp->context == context) {
> >+			found = 1;
> >+			break;
> >+		}
> >+	/* add only a single entry per user context */
> >+	if (unlikely(found)) {
> >+		err = 0;
> >+		goto free_out;
> >+	}
> 
> Maybe this becomes clear later, but can a user add multiple entries?  Can't we
> just consider this a usage error?  Actually, why does it matter if the same
> context is added multiple times?

This list of entries is also used to disperse XRC RCV qp events to all registered
processes -- and we only want a single event dispatched per process.

> >+	dev->destroy_xrc_rcv_qp(qp->xrcd, qpn);
> >+	kfree(qp);
> >+	list_for_each_entry_safe(reg_entry, tmp, &xrc_local, list) {
> >+		list_del(&reg_entry->list);
> >+		kfree(reg_entry);
> >+		atomic_dec(&qp->xrcd->usecnt);
> 
> qp was just freed a few lines up
Ouch!  Thanks for the catch, Sean.  I'll fix it before resubmitting.


Regards,
Jack
--
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