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

[Arne Redlich]

Am Dienstag, den 18.07.2006, 17:31 +0300 schrieb 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?

It's basically the foundation for patch #2 - return a *sensible* REJ
reason and not just IB_CONSUMER_REJ.

> > --
> > 
> > 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.

Because it's just plain easier to handle such code - just looking at the
parameters will tell you what it does despite some magic value you'll
have to track down. While this is imperative for public APIs, it's still
nice for private ones, no?

>  -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?

Oops, you're right. Don't know how this one slipped in?

Arne
-- 
Arne Redlich
Xiranet Communications GmbH