Re: [PATCH rc] RDMA/cma: Ensure rdma_addr_cancel() happens before issuing more requests

[Leon Romanovsky <leon@kernel.org>]

On Thu, Sep 23, 2021 at 05:03:58PM -0300, Jason Gunthorpe wrote:
> On Thu, Sep 23, 2021 at 09:15:44PM +0300, Leon Romanovsky wrote:
> > On Thu, Sep 23, 2021 at 08:45:57AM -0300, Jason Gunthorpe wrote:
> > > On Thu, Sep 23, 2021 at 08:49:06AM +0300, Leon Romanovsky wrote:
> > > > On Wed, Sep 22, 2021 at 11:41:19AM -0300, Jason Gunthorpe wrote:
> > > > > On Wed, Sep 22, 2021 at 11:01:39AM +0300, Leon Romanovsky wrote:
> > > > > 
> > > > > > > +			/* The FSM can return back to RDMA_CM_ADDR_BOUND after
> > > > > > > +			 * rdma_resolve_ip() is called, eg through the error
> > > > > > > +			 * path in addr_handler. If this happens the existing
> > > > > > > +			 * request must be canceled before issuing a new one.
> > > > > > > +			 */
> > > > > > > +			if (id_priv->used_resolve_ip)
> > > > > > > +				rdma_addr_cancel(&id->route.addr.dev_addr);
> > > > > > > +			else
> > > > > > > +				id_priv->used_resolve_ip = 1;
> > > > > > 
> > > > > > Why don't you never clear this field?
> > > > > 
> > > > > The only case where it can be cleared is if we have called
> > > > > rdma_addr_cancel(), and since this is the only place that does it and
> > > > > immediately calls rdma_resolve_ip() again, there is no reason to ever
> > > > > clear it.
> > > > 
> > > > IMHO, it is better to clear instead to rely on "the only place" semantic.
> > > 
> > > Then the code looks really silly:
> > > 
> > > 	if (id_priv->used_resolve_ip) {
> > > 		rdma_addr_cancel(&id->route.addr.dev_addr);
> > >                 id_priv->used_resolve_ip = 0;
> > >         }
> > >         id_priv->used_resolve_ip = 1;
> > 
> > So write comment why you don't need to clear used_resolve_ip, but don't
> > leave it as it is now, where readers need to guess.
> >
> 
> I think it is a bit wordy, but I put this:
> 
> 			/*
> 			 * The FSM can return back to RDMA_CM_ADDR_BOUND after
> 			 * rdma_resolve_ip() is called, eg through the error
> 			 * path in addr_handler(). If this happens the existing
> 			 * request must be canceled before issuing a new one.
> 			 * Since canceling a request is a bit slow and this
> 			 * oddball path is rare, keep track once a request has
> 			 * been issued. The track turns out to be a permanent
> 			 * state since this is the only cancel as it is
> 			 * immediately before rdma_resolve_ip().
> 			 */
> 
> And into for-rc

Thanks

> 
> Jason