[openib-general] [PATCH 2/3] iWARP and RDMA CMA

[Tom Tucker]

Comments enclosed...

On Wed, 2006-03-08 at 11:09 -0800, Sean Hefty wrote:
> Only one series issue, along with a handful of nits.
> 
> Tom Tucker wrote:
> > +#include <rdma/ib_sa.h>
> 
> Why is ib_sa.h needed?

droppings... tx

> 
> > +static int cma_iw_handler(struct iw_cm_id* iw_id, struct iw_cm_event* event)
> > +{
> > +	struct rdma_id_private *id_priv = iw_id->context;
> > +	enum rdma_cm_event_type event_type = 0;
> 
> For consistency with the cma_ib_handler, can you rename 'event' to 'iw_event' 
> and 'event_type' to event?
done.
> 
> > +	int ret = 0;
> > +
> > +	atomic_inc(&id_priv->dev_remove);
> > +
> > +	switch (event->event) {
> > +	case IW_CM_EVENT_LLP_DISCONNECT:
> > +	case IW_CM_EVENT_LLP_RESET:
> > +	case IW_CM_EVENT_LLP_TIMEOUT:
> > +	case IW_CM_EVENT_CLOSE:
> > +		event_type = RDMA_CM_EVENT_DISCONNECTED;
> > +		break;
> > +
> > +	case IW_CM_EVENT_CONNECT_REPLY: {
> > +		if (event->status)
> > +			event_type = RDMA_CM_EVENT_REJECTED;
> > +		else
> > +			event_type = RDMA_CM_EVENT_ESTABLISHED;
> > +		break;
> > +	}
> > +
> > +	case IW_CM_EVENT_ESTABLISHED:
> > +		event_type = RDMA_CM_EVENT_ESTABLISHED;
> > +		break;
> > +
> > +	default:
> > +		BUG_ON(1);
> > +		break;
> > +
> > +	}	
> 
> Nit: please remove the extra spacing and braces in the above code.
done.
> 
> > +
> > +	ret = cma_notify_user(id_priv, 
> > +			      event_type, 
> > +			      event->status, 
> > +			      event->private_data,
> > +			      event->private_data_len);
> 
> Nit: elsewhere in the code uses multiple parameters per line.
done.
> 
> > +	if (ret) {
> > +		/* Destroy the CM ID by returning a non-zero value. */
> > +		id_priv->cm_id.iw = NULL;
> > +		cma_exch(id_priv, CMA_DESTROYING);
> > +		cma_release_remove(id_priv);
> > +		rdma_destroy_id(&id_priv->id);
> > +		return ret;
> > +	}
> > +
> > +	cma_release_remove(id_priv);
> > +	return ret;
> > +}
> > +
> > +static int iw_conn_req_handler(struct iw_cm_id *cm_id, 
> > +			       struct iw_cm_event *iw_event)
> > +{
> > +	struct rdma_cm_id* new_cm_id;
> > +	struct rdma_id_private *listen_id, *conn_id;
> > +	struct sockaddr_in* sin;
> > +	int ret;
> > +
> > +	listen_id = cm_id->context;
> > +	atomic_inc(&listen_id->dev_remove);
> > +	if (!cma_comp(listen_id, CMA_LISTEN)) {
> > +		ret = -ECONNABORTED;
> > +		goto out;
> > +	}
> > +
> > +	/* Create a new RDMA id the new IW CM ID */
> > +	new_cm_id = rdma_create_id(listen_id->id.event_handler, 
> > +				   listen_id->id.context,
> > +				   RDMA_PS_TCP);
> > +	if (!new_cm_id) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +	conn_id = container_of(new_cm_id, struct rdma_id_private, id);
> > +	atomic_inc(&conn_id->dev_remove);
> > +	conn_id->state = CMA_CONNECT;
> > +
> > +	/* New connection inherits device and address from parent */
> > +	memcpy(&new_cm_id->route.addr.dev_addr,
> > +	       &listen_id->id.route.addr.dev_addr,
> > +	       sizeof(new_cm_id->route.addr.dev_addr));
> > +	/* New connection inherits device from parent */
> > +	cma_acquire_dev(conn_id);
> 
> We are not assured that we'll be able to acquire the device.  It could have been 
> removed as we entered the req_handler.
good catch. done.
> 
> > +
> > +	conn_id->cm_id.iw = cm_id;
> > +	cm_id->context = conn_id;
> > +	cm_id->cm_handler = cma_iw_handler;
> > +
> > +	sin = (struct sockaddr_in*)&new_cm_id->route.addr.src_addr;
> > +	*sin = iw_event->local_addr;
> > +
> > +	sin = (struct sockaddr_in*)&new_cm_id->route.addr.dst_addr;
> > +	*sin = iw_event->remote_addr;
> > +
> > +	ret = cma_notify_user(conn_id, RDMA_CM_EVENT_CONNECT_REQUEST, 0,
> > +			      iw_event->private_data,
> > +			      iw_event->private_data_len);
> > +	if (ret) {
> > +		/* Destroy the CM ID by returning a non-zero value. */
> > +		conn_id->cm_id.iw = NULL;
> > +		cma_exch(conn_id, CMA_DESTROYING);
> > +		cma_release_remove(conn_id);
> > +		rdma_destroy_id(&conn_id->id);
> > +	}
> > +
> > +out:
> > +	cma_release_remove(listen_id);
> > +	return ret;
> > +}
> > +
> {snip}
> > +static int cma_connect_iw(struct rdma_id_private *id_priv,
> > +			  struct rdma_conn_param *conn_param)
> > +{
> > +	struct iw_cm_id* cm_id;
> > +	struct sockaddr_in* sin;
> > +	int ret;
> > +
> > +	cm_id = iw_create_cm_id(id_priv->id.device, cma_iw_handler, id_priv);
> > +	if (IS_ERR(cm_id)) {
> > +		ret = PTR_ERR(cm_id);
> > +		goto out;
> > +	}
> > +
> > +	id_priv->cm_id.iw = cm_id;
> > +
> > +	sin = (struct sockaddr_in*)&id_priv->id.route.addr.src_addr;
> > +	cm_id->local_addr = *sin;
> > +
> > +	sin = (struct sockaddr_in*)&id_priv->id.route.addr.dst_addr;
> > +	cm_id->remote_addr = *sin;
> > +
> > +	cm_id->qp = id_priv->id.qp;
> > +	cm_id->qp_num = id_priv->qp_num;
> 
> It seems strange to me to set values field values in the cm_id in this way. 
> Maybe these should be parameters to iw_cm_connect()?

I agree. The connect, accept and reject should take a conn_param
structure. I have a plan to do this, but it is part of a larger change
that includes llp_connect, etc...  
> 
> > +
> > +	ret = iw_cm_connect(cm_id, conn_param->private_data, 
> > +			    conn_param->private_data_len);
> > +						
> > +out:
> > +	return ret;
> > +}
> {snip}
> >  		break;
> > +	case RDMA_TRANSPORT_IWARP:
> > +		id_priv->cm_id.iw->qp = id_priv->id.qp;
> > +		id_priv->cm_id.iw->qp_num = id_priv->qp_num;
> > +
> > +		ret = iw_cm_accept(id_priv->cm_id.iw, conn_param->private_data,
> > +				   conn_param->private_data_len);
> 
> See comment above.  Maybe add QP/QPN as parameters to iw_cm_accept()?
> 
> {snip}
> > +static void ucma_copy_iw_route(struct rdma_ucm_query_route_resp *resp,
> > +			       struct rdma_route *route)
> > +{
> > +	struct rdma_dev_addr *dev_addr;
> > +
> > +	resp->num_paths = 0;
> > +	dev_addr = &route->addr.dev_addr;
> > +	memset(&resp->ib_route[0], 0, sizeof(resp->ib_route[0]));
> > +	memcpy(&resp->ib_route[0].dgid, iw_addr_get_dgid(dev_addr),
> > +		       sizeof(union ib_gid));
> > +	memcpy(&resp->ib_route[0].sgid, iw_addr_get_sgid(dev_addr),
> > +		       sizeof(union ib_gid));
> Nit: spacing is off here.  Also, please use sizeof resp->ib_route[0].dgid.
> 
> It's somewhat confusing to re-use ib_route to convey iWarp data.  Can we use a 
> union here?

Yes. I agree, but I didn't want to mess with any user mode code and this
is effectively invisible. I thought we should tackle this in conjunction
with a revisit of device addressing in general (e.g. pretending that
iWARP devices have GID's in order to reuse the port cache, etc...)

> 
> > +	resp->ib_route[0].pkey = 0;
> memset() should have cleared this already.

good point.
> 
> - Sean