[openib-general] [PATCH] ib_mad: In ib_mad_complete_recv, decrement agent refcount when not fully reassembled and when no request found

[Hal Rosenstock]

On Mon, 2004-10-25 at 13:14, Sean Hefty wrote:
> On Sun, 24 Oct 2004 13:38:01 -0400
> Hal Rosenstock <halr at voltaire.com> wrote:
> 
> > ib_mad: In ib_mad_complete_recv, decrement agent reference count when
> > receive is not fully reassembled, and also when solicited and no
> > matching request is found. This allows deregistration to complete rather
> > than waiting for an event which never occurs.
> > ... 
> >  	/* Fully reassemble receive before processing */
> >  	recv = reassemble_recv(mad_agent_priv, recv);
> > -	if (!recv)
> > +	if (!recv) {
> > +		atomic_dec(&mad_agent_priv->refcount);
> >  		return;
> > +	}
> 
> I'm not sure about this.  

I wasn't sure about this either.

> If we just start assembling a MAD, we're probably going to want to maintain 
> a reference on the MAD agent while the reassembly is occurring, in order to 
> handle timeouts.  We'll have a better idea of what the RMPP code will do 
> once it's actually written however, so that reference could be a different one.

Yes, but it gets incremented once every received segment and never
decremented (prior to this patch). This is to handle the increment at
line 1003 before ib_mad_complete_recv is called in
ib_mad_recv_done_handler.

>   If we do keep this code, it should probably be an atomic_dec_and_test, 
> followed by a wake_up.

Similar to below, I don't think it is the last reference held on the
agent.

> >  	/* Complete corresponding request */
> >  	if (solicited) {
> > @@ -884,6 +886,7 @@
> >  		if (!mad_send_wr) {
> >  			spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
> >  			ib_free_recv_mad(&recv->header.recv_wc);
> > +			atomic_dec(&mad_agent_priv->refcount);
> >  			return;
> 
> I think that we want this to be atomic_dec_and_test().  
> (Similar to the call near the end of this function.)  If a match is found, 
> we can get away with a simple atomic_dec, since the send will still hold a 
> reference on the mad agent.  But if no match is found, then I think this 
> may be the last reference being held.

I didn't change the match path only the non match. It's not the last
reference held as one other reference is held on the agent which is
given up at deregistration time.

-- Hal