[openib-general] [PATCH 1/2] ib_cm: cm_destroy_id() cleanup

[Michael S. Tsirkin]

Quoting r. Arne Redlich <arne.redlich at xiranet.com>:
> Subject: Re: [PATCH 1/2] ib_cm: cm_destroy_id() cleanup
> 
> Am Dienstag, den 18.07.2006, 06:59 -0700 schrieb Roland Dreier:
> >  > +		cm_destroy_id(&cm_id_priv->id, (ret == -ENOMEM) ? 0 : 1);
> > 
> > This is rather obfuscated.  How about just
> > 
> > 		cm_destroy_id(&cm_id_priv->id, ret != -ENOMEM);
> > 
> >  - R.
> 
> Sure. Fixed below for your convenience.
> Would you consider pushing these to 2.6.18? If so, I'd happily provide
> patches against rc1 or your git tree.
> 
> Thanks,
> Arne

Well, Sean is the judge here, but what's the motivation for this patch?

> --
> 
> In its current incarnation, cm_destroy_id() will not send a REJ if fed a
> magic number (err = -ENOMEM). This patch replaces this magic number with
> a more generic "reject" parameter.
> 
> Signed-off-by: Arne Redlich <arne.redlich at xiranet.com> 

Are you sure this is a good idea? cm_destroy_id is after all an internal
function so I don't see why it must be generic. -ENOMEM is the value user
returns to avoid rej, so it seems cleaner to just do the check where it's
actually needed and avoid extra branches - and I expect this will be easier to
extend if we have more codes.

> Index: infiniband/core/cm.c
> ===================================================================
> --- infiniband/core/cm.c	(revision 8565)
> +++ infiniband/core/cm.c	(working copy)
> @@ -702,7 +702,7 @@ static void cm_reset_to_idle(struct cm_i
>  	}
>  }
>  
> -static void cm_destroy_id(struct ib_cm_id *cm_id, int err)
> +static void cm_destroy_id(struct ib_cm_id *cm_id, int reject)
>  {
>  	struct cm_id_private *cm_id_priv;
>  	struct cm_work *work;
> @@ -737,14 +737,15 @@ retest:
>  			       NULL, 0);
>  		break;
>  	case IB_CM_REQ_RCVD:
> -		if (err == -ENOMEM) {
> +		if (reject) {
> +			spin_unlock_irqrestore(&cm_id_priv->lock, flags);
> +			ib_send_cm_rej(cm_id, IB_CM_REJ_CONSUMER_DEFINED,
> +				       NULL, 0, NULL, 0);
> +		} else {
>  			/* Do not reject to allow future retries. */
>  			cm_reset_to_idle(cm_id_priv);
>  			spin_unlock_irqrestore(&cm_id_priv->lock, flags);
>  		} else {
> -			spin_unlock_irqrestore(&cm_id_priv->lock, flags);
> -			ib_send_cm_rej(cm_id, IB_CM_REJ_CONSUMER_DEFINED,
> -				       NULL, 0, NULL, 0);
>  		}

This looks like "if {} else {} else {}" - am I missing something?

>  		break;
>  	case IB_CM_MRA_REQ_RCVD:
> @@ -789,7 +790,7 @@ retest:
>  
>  void ib_destroy_cm_id(struct ib_cm_id *cm_id)
>  {
> -	cm_destroy_id(cm_id, 0);
> +	cm_destroy_id(cm_id, 1);
>  }
>  EXPORT_SYMBOL(ib_destroy_cm_id);
>  
> @@ -1179,7 +1180,7 @@ static void cm_process_work(struct cm_id
>  	}
>  	cm_deref_id(cm_id_priv);
>  	if (ret)
> -		cm_destroy_id(&cm_id_priv->id, ret);
> +		cm_destroy_id(&cm_id_priv->id, ret != -ENOMEM);
>  }
>  
>  static void cm_format_mra(struct cm_mra_msg *mra_msg,

-- 
MST