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

[Sean Hefty]

Tom Tucker wrote:
> +static inline void iwcm_deref_id(struct iwcm_id_private *cm_id_priv)
> +{
> +	if (atomic_dec_and_test(&cm_id_priv->refcount))
> +		kfree(cm_id_priv);
> +}

I'm wary of code that does this.  This can typically result in a race condition 
where the user can receive a callback after destroy returns.

> +/* Clean up all resources associated with the connection. Set the
> + * cm_id state to DESTROYING so queued events will be ignored and
> + * subsequent events will not be queued. 
> + */

The problem is that a callback could be just about to be invoked, when the user 
calls destroy.  Destroy executes and returns before the callback thread hits 
their code.  We need to block in destroy until all callbacks complete. 
Otherwise, the user can never safely release any resources associated with this 
cm_id.

> +void iw_destroy_cm_id(struct iw_cm_id *cm_id)
> +{
> +	struct iwcm_id_private *iwcm_id_priv;
> +	unsigned long flags;
> +	struct ib_qp *qp;
> +	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_DESTROYING;
> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
> +		ret = cm_id->device->iwcm->destroy_listen(cm_id);
> +		break;
> +	case IW_CM_STATE_CONN_RECV:
> +		cm_id->state = IW_CM_STATE_DESTROYING;
> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
> +		/* Need to call reject on behalf of client */
> +		(void)iw_cm_reject(cm_id, NULL, 0);
> +		break;
> +	case IW_CM_STATE_CONN_SENT:
> +		cm_id->state = IW_CM_STATE_DESTROYING;
> +		cm_id->qp = NULL;
> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
> +		break;
> +	case IW_CM_STATE_ESTABLISHED:
> +		cm_id->state = IW_CM_STATE_DESTROYING;
> +		qp = cm_id->qp;
> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
> +		ret = iwcm_modify_qp_err(qp);
> +		break;
> +	case IW_CM_STATE_IDLE:
> +		cm_id->state = IW_CM_STATE_DESTROYING;
> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
> +		break;
> +	case IW_CM_STATE_CLOSING:
> +		/* Lost race with cm_disconnect_handler. Need to 
> +		 * release 'connect' reference, since setting state to
> +		 * DESTROYING will case CLOSE event to be ignored.
> +		 */
> +		cm_id->state = IW_CM_STATE_DESTROYING;
> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
> +		iwcm_deref_id(iwcm_id_priv);
> +		break;
> +	default:
> +		/* Lost race with cm_close_handler. */ 
> +		cm_id->state = IW_CM_STATE_DESTROYING;
> +		spin_unlock_irqrestore(&iwcm_id_priv->lock, flags);
> +	}
> +	iwcm_deref_id(iwcm_id_priv);
> +}
> +EXPORT_SYMBOL(iw_destroy_cm_id);

- Sean