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

[Tom Tucker]

On Wed, 2006-03-15 at 16:34 -0800, Sean Hefty wrote:
> 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.

Yep. But the alternative is a deadlock in the event thread (the event
that removes the last reference is behind the event that blocks trying
to destroy the id). This happens because when both peers try to
disconnect concurrently. 

There is an iWARP provider requirement that CLOSE is absolutely the last
event...I was wary too, so to test my implementation I ran 6 threads 3
IB, 3 iWARP on two dual CPU boxes. 


> 
> > +/* 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