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:

quoted

On Thu, Sep 23, 2021 at 08:49:06AM +0300, Leon Romanovsky wrote:

quoted

On Wed, Sep 22, 2021 at 11:41:19AM -0300, Jason Gunthorpe wrote:

quoted

On Wed, Sep 22, 2021 at 11:01:39AM +0300, Leon Romanovsky wrote:

quoted

quoted

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

Jason