[openib-general] [PATCH 3/3] iWARP CM

[Caitlin Bestler]

openib-general-bounces at openib.org wrote:
> Tom Tucker wrote:
>> +#include <rdma/ib_cache.h>
>> +#include <rdma/ib_cm.h>
> 
> Is this needed?
> 
>> +#include <rdma/iw_cm.h>
>> +
>> +#include "cm_msgs.h"
> 
> Is this needed?
> 
>> +struct iwcm_id_private;
> 
> I think this can be removed.
> 
>> +struct iwcm_device;
>> +struct iwcm_port {
>> +	struct iwcm_device *iwcm_dev;
>> +	struct sockaddr_in local_addr;
>> +	u8 port_num;
>> +};
> 
> Can the port have more than one address?
> 
>> +/* Called whenever a reference added for a cm_id */ static inline
>> +void iwcm_addref_id(struct iwcm_id_private *cm_id_priv) {
>> +	atomic_inc(&cm_id_priv->refcount);
>> +}
> 
> Is a wrapper around atomic_inc() needed?
> 
>> +struct iw_cm_id *iw_create_cm_id(struct ib_device *device,
>> +				 iw_cm_handler cm_handler,
>> +				 void *context)
>> +{
>> +	struct iwcm_id_private *iwcm_id_priv;
>> +
>> +	iwcm_id_priv = kmalloc(sizeof *iwcm_id_priv, GFP_KERNEL); +
if
>> (!iwcm_id_priv) +		return ERR_PTR(-ENOMEM);
>> +
>> +	memset(iwcm_id_priv, 0, sizeof *iwcm_id_priv);
> 
> Use kzalloc().
> 
>> +	iwcm_id_priv->id.state = IW_CM_STATE_IDLE;
>> +	iwcm_id_priv->id.device = device;
>> +	iwcm_id_priv->id.cm_handler = cm_handler;
>> +	iwcm_id_priv->id.context = context;
>> +	iwcm_id_priv->id.event_handler = cm_event_handler; +
>> +	spin_lock_init(&iwcm_id_priv->lock);
>> +	init_waitqueue_head(&iwcm_id_priv->wait);
>> +	atomic_set(&iwcm_id_priv->refcount, 1);
>> +
>> +	return &iwcm_id_priv->id;
>> +
>> +}
>> +EXPORT_SYMBOL(iw_create_cm_id);
> 
>> +void iw_destroy_cm_id(struct iw_cm_id *cm_id) {
>> +	struct iwcm_id_private *iwcm_id_priv;
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	iwcm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
+
>> +	spin_lock_irqsave(&iwcm_id_priv->lock, flags);
>> +	switch (cm_id->state) {
>> +	case IW_CM_STATE_LISTEN:
>> +		cm_id->state = IW_CM_STATE_IDLE;
>> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
>> +		ret = cm_id->device->iwcm->destroy_listen(cm_id); +
break;
>> +
>> +	case IW_CM_STATE_CONN_RECV:
>> +	case IW_CM_STATE_CONN_SENT:
>> +	case IW_CM_STATE_ESTABLISHED:
>> +		cm_id->state = IW_CM_STATE_IDLE;
>> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
>> +		ret = cm_id->device->iwcm->disconnect(cm_id,1);
>> +		break;
>> +
>> +	case IW_CM_STATE_IDLE:
>> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags); +
break;
>> +
>> +	default:
>> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
>> +		printk(KERN_ERR "%s:%s:%u Illegal state %d for
iw_cm_id.\n",
>> +		       __FILE__, __FUNCTION__, __LINE__, cm_id->state);
>> +		;
> 
> random ';'
> 
> Nit: my preference would be to remove the extra blank lines
> after the break statements.
> 
>> +	}
>> +
>> +	atomic_dec(&iwcm_id_priv->refcount);
>> +	wait_event(iwcm_id_priv->wait,
>> +!atomic_read(&iwcm_id_priv->refcount));
>> +
>> +	kfree(iwcm_id_priv);
>> +}
>> +EXPORT_SYMBOL(iw_destroy_cm_id);
> 
>> +int iw_cm_listen(struct iw_cm_id *cm_id, int backlog) {
>> +	struct iwcm_id_private *iwcm_id_priv;
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	if (cm_id->device == 0 || cm_id->device->iwcm == 0)
>> +		return -EINVAL;
> 
> Use NULL in place of '0'.  Are these checks needed?
>> +
>> +	iwcm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
>> +	spin_lock_irqsave(&iwcm_id_priv->lock, flags);
>> +	if (cm_id->state != IW_CM_STATE_IDLE) {
>> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
>> +		return -EBUSY;
> 
> -EINVAL seems like a better return code here.
> 
>> +	}
>> +	cm_id->state = IW_CM_STATE_LISTEN;
>> +	spin_unlock_irqrestore(&iwcm_id_priv->lock, flags); +
>> +	ret = cm_id->device->iwcm->create_listen(cm_id, backlog);
>> +	if (ret != 0)
> 
> Nit: how about just if (ret)?
> 
>> +		cm_id->state = IW_CM_STATE_IDLE;
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(iw_cm_listen);
> 
>> +int iw_cm_getpeername(struct iw_cm_id *cm_id,
>> +		      struct sockaddr_in* local_addr,
>> +		      struct sockaddr_in* remote_addr) {
>> +	if (cm_id->device == 0)
>> +		return -EINVAL;
>> +
>> +	if (cm_id->device->iwcm == 0)
>> +		return -EINVAL;
> 
> Use NULL - are checks needed given state check below?  Is
> synchronization around this call needed, since it checks the state?
>> +
>> +	/* Make sure there's a connection */
>> +	if (cm_id->state != IW_CM_STATE_ESTABLISHED)
>> +		return -ENOTCONN;
>> +
>> +	return cm_id->device->iwcm->getpeername(cm_id, local_addr,
>> +remote_addr); } EXPORT_SYMBOL(iw_cm_getpeername);
> 
>> +int iw_cm_reject(struct iw_cm_id *cm_id,
>> +		 const void *private_data,
>> +		 u8 private_data_len)
>> +{
>> +	struct iwcm_id_private *iwcm_id_priv;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	if (cm_id->device == 0 || cm_id->device->iwcm == 0)
>> +		return -EINVAL;
> 
> Use NULL - are checks needed?
> 
>> +
>> +	iwcm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
+
>> +	spin_lock_irqsave(&iwcm_id_priv->lock, flags);
>> +	switch (cm_id->state) {
>> +	case IW_CM_STATE_CONN_RECV:
>> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
>> +		ret = cm_id->device->iwcm->reject(cm_id, private_data, +

>> private_data_len); +		cm_id->state = IW_CM_STATE_IDLE;
> 
> This changes the state after the lock has been released.
> 
>> +		break;
>> +	default:
>> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags); +
ret =
>> -EINVAL; +	}
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(iw_cm_reject);
> 
>> +int iw_cm_accept(struct iw_cm_id *cm_id,
>> +		 const void *private_data,
>> +		 u8 private_data_len)
>> +{
>> +	struct iwcm_id_private *iwcm_id_priv;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	if (cm_id->device == 0 || cm_id->device->iwcm == 0)
>> +		return -EINVAL;
> 
> ditto...
> 
>> +
>> +	iwcm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
+
>> +	spin_lock_irqsave(&iwcm_id_priv->lock, flags);
>> +	switch (cm_id->state) {
>> +	case IW_CM_STATE_CONN_RECV:
>> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
>> +		ret = cm_id->device->iwcm->accept(cm_id, private_data, +

>> private_data_len); +		if (ret == 0) {
>> +			struct iw_cm_event event;
>> +			event.event = IW_CM_EVENT_ESTABLISHED;
>> +			event.provider_id = cm_id->provider_id;
>> +			event.status = 0;
>> +			event.local_addr = cm_id->local_addr;
>> +			event.remote_addr = cm_id->remote_addr;
> 
> If the cm_id contains the provider_id, local_addr, and
> remote_addr, does it need to be returned as part of the event?
>

It should be possible to create the cm_id to listen for all
IP addresses that the Ethernet device is capable of accepting.
That would mean that event.local_Addr should be set from the
actual packet.

While not used for RDMA applications yet, it is common for
IP based applications to differentiate the service provided
based upon which local address the remote peer requested.
This is especially true for web servers.

This relates back to the question on whether a port has
more than one address. It depends on your definition of a
port.  If the definition of a port is a netdevice then it
is only necessary to provide one of the addresses. Existing
techniques allow fetching the other aliases that refer to
the same ethernet interface. There is no reason for the RDMA
drivers to take on the complexity of reporting all possible
IP addresses when the answer already exists in for non-RDMA.