[openib-general] Re: [PATCH] [CMA] RDMA CM abstraction module

[Sean Hefty]

Thanks for the feedback.  See below.

Michael S. Tsirkin wrote:
> Wouldnt is be a good idea to start names with rdma_cm
> or rdma_cma or something like that?
> For example, rdma_event_type is a bit confusing since this actually only
> includes CM events. Similiar comments apply to other names.

I had that originally, but changed it.  I figured that names like rdma_connect() 
and rdma_listen() were clear enough that they were for connection management.

>>+struct rdma_id;
> 
> I propose renaming this to rdma_connection or something
> else more specific than just "id". Makes sense?

I can change this to rdma_cm_id or rdma_cma or something else...

>>+int rdma_resolve_route(struct rdma_id *id, int timeout_ms);
> 
> Not sure I understand what this does, since the only extra parameter is
> timeout_ms.

For IB, this results in a path record query based on the GIDs that were set with 
the rdma_id from rdma_resolve_addr().  The GIDs are in 
rdma_id.route.addr.ibaddr.  The output is saved to rdma_id.route.path_rec.  My 
intent is to make this call optional in the future.

>>+int rdma_create_qp(struct rdma_id *id, struct ib_pd *pd,
>>+		   struct ib_qp_init_attr *qp_init_attr);
>>+
>>+void rdma_destroy_qp(struct rdma_id *id);
> 
> Not sure what the intended usage is.
> When does the user need to call this?

The CMA needs to associate a QP with the rdma_id, and CMA will transition the QP 
through its connection states.  The rdma_create_qp() is called to allocate a QP 
and transition it to the INIT state, so users can post receives to the QP.  The 
destroy call is a pass-through call provided simply for symmetry.

>>+#include <linux/in.h>
>>+#include <linux/in6.h>
>>+#include <linux/inetdevice.h>
>>+#include <net/arp.h>
>>+#include <net/neighbour.h>
>>+#include <net/route.h>
>>+#include <rdma/rdma_cm.h>
>>+#include <rdma/ib_cache.h>
>>+#include <rdma/ib_cm.h>
>>+#include <rdma/ib_sa.h>
> 
> Are all of these headers really needed?
> For example, I dont see arp.h used anywhere.
> Am I missing something?

They were needed at one point, but might not all be needed now.  I will see 
which ones can be removed.  Some were only needed for address translation, which 
was originally part of this file while I worked out its API.

> What about replacing switch with one case statements with if statements.
> Like this:
> 
> 	if (id->device->node_type == IB_NODE_CA)
> 		ret = cma_init_ib_qp(id_priv, qp);
> 	else
> 		ret = -ENOSYS;

I tried to make it easy to modify the code to support iWarp, or some other RDMA 
device.  I'd prefer to leave these checks as switch statements for that reason, 
or just remove them completely.

> I also wander why do we really need all these node_type checks.
> The code above seems to imply that rdma_create_qp will fail
> on non-CA. Why is that?

The code doesn't set the right parameters to INIT for an iWarp QP.

>>+static inline void cma_deref_dev(struct rdma_id_private *id_priv)
>>+{
>>+//	if (atomic_dec_and_test(&id_priv->dev_remove))
>>+//		wake_up(&id_priv->wait);
>>+//	return atomic_dec_and_test(&id_priv->dev_remove) ?
>>+//	       cma_notify_user(id_priv, RDMA_EVENT_DEVICE_REMOVAL, -ENODEV,
>>+//			       NULL, 0) : 0;
>>+}
> 
> 
> The above seems to need some cleanup.

This has been cleaned up in my latest version.  It was part of the initial 
device removal handling code that didn't work.  I decided to just try to get 
connection establishment working, and then come back to fix device removal.

- Sean