[ofa-general] RE: [RFC V4 PATCH 3/5] rdma/cma: simply locking needed for serialization of callbacks

[Sean Hefty]

>-static int cma_disable_remove(struct rdma_id_private *id_priv,
>+static int cma_disable_callback(struct rdma_id_private *id_priv,
> 			      enum cma_state state)
> {
> 	unsigned long flags;
> 	int ret;
>
>+	mutex_lock(&id_priv->handler_mutex);
> 	spin_lock_irqsave(&id_priv->lock, flags);
>-	if (id_priv->state == state) {
>-		atomic_inc(&id_priv->dev_remove);
>+	if (id_priv->state == state)
> 		ret = 0;
>-	} else
>+	 else {
>+		mutex_unlock(&id_priv->handler_mutex);
> 		ret = -EINVAL;
>+	}
> 	spin_unlock_irqrestore(&id_priv->lock, flags);
> 	return ret;
> }

I wasn't clear on this before, but we shouldn't need to take the spinlock here
at all now.  We needed it before in order to check the state and increment
dev_remove in one operation.  Once the spinlock was released the state could
have changed, but dev_remove would have halted the device removal thread.  Under
the new method, device removal is halted while we hold the handler_mutex.

>@@ -2566,8 +2560,8 @@ static int cma_ib_mc_handler(int status,
> 	int ret;
>
> 	id_priv = mc->id_priv;
>-	if (cma_disable_remove(id_priv, CMA_ADDR_BOUND) &&
>-	    cma_disable_remove(id_priv, CMA_ADDR_RESOLVED))
>+	if (cma_disable_callback(id_priv, CMA_ADDR_BOUND) &&
>+	    cma_disable_callback(id_priv, CMA_ADDR_RESOLVED))

This can end up trying to acquire the mutex twice.  We could change this to

mutex_lock();
if (id_priv->state == CMA_ADDR_BOUND || id_priv->state == CMA_ADDR_RESOLVED)

- Sean