IB/ipoib: fixup multicast locking issues

Create a an ipoib_flush_ah and ipoib_stop_ah routines to use at appropriate times to flush out all remaining ah entries before we shut the device down.

Because neighbors and mcast entries can each have a reference on any given ah, we must make sure to free all of those first before our ah will actually have a 0 refcount and be able to be reaped.

This factoring is needed in preparation for having per-device work queues. The original per-device workqueue code resulted in the following error message:

<ibdev>: ib_dealloc_pd failed

That error was tracked down to this issue. With the changes to which workqueues were flushed when, there were no flushes of the per device workqueue after the last ah's were freed, resulting in an attempt to dealloc the pd with outstanding resources still allocated. This code puts the explicit flushes in the needed places to avoid that problem.

Signed-off-by Doug Ledford <[email protected]>

---
  drivers/infiniband/ulp/ipoib/ipoib_ib.c | 46 ++++++++++++++++++++-------------
  1 file changed, 28 insertions(+), 18 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_ib.c +19 -18

index 72626c34817..cb02466a0eb 100644
@@ -659,6 +659,24 @@ void ipoib_reap_ah(struct work_struct *work)
  				   round_jiffies_relative(HZ));
  }
 
+static void ipoib_flush_ah(struct net_device *dev, int flush)
+{
+	struct ipoib_dev_priv *priv = netdev_priv(dev);
+
+	cancel_delayed_work(&priv->ah_reap_task);
+	if (flush)
+		flush_workqueue(ipoib_workqueue);
+	ipoib_reap_ah(&priv->ah_reap_task.work);
+}
+
+static void ipoib_stop_ah(struct net_device *dev, int flush)
+{
+	struct ipoib_dev_priv *priv = netdev_priv(dev);
+
+	set_bit(IPOIB_STOP_REAPER, &priv->flags);
+	ipoib_flush_ah(dev, flush);
+}
+
  static void ipoib_ib_tx_timer_func(unsigned long ctx)
  {
  	drain_tx_cq((struct net_device *)ctx);

@@ -877,24 +895,7 @@ timeout:
  	if (ib_modify_qp(priv->qp, &qp_attr, IB_QP_STATE))
  		ipoib_warn(priv, "Failed to modify QP to RESET state\n");
 
-	/* Wait for all AHs to be reaped */
-	set_bit(IPOIB_STOP_REAPER, &priv->flags);
-	cancel_delayed_work(&priv->ah_reap_task);
-	if (flush)
-		flush_workqueue(ipoib_workqueue);
-
-	begin = jiffies;
-
-	while (!list_empty(&priv->dead_ahs)) {
-		__ipoib_reap_ah(dev);
-
-		if (time_after(jiffies, begin + HZ)) {
-			ipoib_warn(priv, "timing out; will leak address handles\n");
-			break;
-		}
-
-		msleep(1);
-	}
+	ipoib_flush_ah(dev, flush);
 
  	ib_req_notify_cq(priv->recv_cq, IB_CQ_NEXT_COMP);
 

drivers/infiniband/ulp/ipoib/ipoib_ib.c +1 -0

  	}
 
  	if (level >= IPOIB_FLUSH_NORMAL)

@@ -1100,6 +1102,14 @@ void ipoib_ib_dev_cleanup(struct net_device *dev)
  	ipoib_mcast_stop_thread(dev, 1);
  	ipoib_mcast_dev_flush(dev);
 
+	/*

Quoted 80 lines · Doug Ledford

Create a an ipoib_flush_ah and ipoib_stop_ah routines to use at appropriate times to flush out all remaining ah entries before we shut the device down.

Because neighbors and mcast entries can each have a reference on any given ah, we must make sure to free all of those first before our ah will actually have a 0 refcount and be able to be reaped.

This factoring is needed in preparation for having per-device work queues. The original per-device workqueue code resulted in the following error message:

<ibdev>: ib_dealloc_pd failed

That error was tracked down to this issue. With the changes to which workqueues were flushed when, there were no flushes of the per device workqueue after the last ah's were freed, resulting in an attempt to dealloc the pd with outstanding resources still allocated. This code puts the explicit flushes in the needed places to avoid that problem.

Signed-off-by Doug Ledford <[email protected]>

---
  drivers/infiniband/ulp/ipoib/ipoib_ib.c | 46 ++++++++++++++++++++-------------
  1 file changed, 28 insertions(+), 18 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_ib.c +19 -18

index 72626c34817..cb02466a0eb 100644
@@ -659,6 +659,24 @@ void ipoib_reap_ah(struct work_struct *work)
  				   round_jiffies_relative(HZ));
  }
 
+static void ipoib_flush_ah(struct net_device *dev, int flush)
+{
+	struct ipoib_dev_priv *priv = netdev_priv(dev);
+
+	cancel_delayed_work(&priv->ah_reap_task);
+	if (flush)
+		flush_workqueue(ipoib_workqueue);
+	ipoib_reap_ah(&priv->ah_reap_task.work);
+}
+
+static void ipoib_stop_ah(struct net_device *dev, int flush)
+{
+	struct ipoib_dev_priv *priv = netdev_priv(dev);
+
+	set_bit(IPOIB_STOP_REAPER, &priv->flags);
+	ipoib_flush_ah(dev, flush);
+}
+
  static void ipoib_ib_tx_timer_func(unsigned long ctx)
  {
  	drain_tx_cq((struct net_device *)ctx);

@@ -877,24 +895,7 @@ timeout:
  	if (ib_modify_qp(priv->qp, &qp_attr, IB_QP_STATE))
  		ipoib_warn(priv, "Failed to modify QP to RESET state\n");
 
-	/* Wait for all AHs to be reaped */
-	set_bit(IPOIB_STOP_REAPER, &priv->flags);
-	cancel_delayed_work(&priv->ah_reap_task);
-	if (flush)
-		flush_workqueue(ipoib_workqueue);
-
-	begin = jiffies;
-
-	while (!list_empty(&priv->dead_ahs)) {
-		__ipoib_reap_ah(dev);
-
-		if (time_after(jiffies, begin + HZ)) {
-			ipoib_warn(priv, "timing out; will leak address handles\n");
-			break;
-		}
-
-		msleep(1);
-	}
+	ipoib_flush_ah(dev, flush);
 
  	ib_req_notify_cq(priv->recv_cq, IB_CQ_NEXT_COMP);
 

drivers/infiniband/ulp/ipoib/ipoib_ib.c static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, · line 1041

Quoted 5 lines

drivers/infiniband/ulp/ipoib/ipoib_ib.c +1 -0

@@ -1037,6 +1038,7 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv,
  	if (level == IPOIB_FLUSH_LIGHT) {
  		ipoib_mark_paths_invalid(dev);
  		ipoib_mcast_dev_flush(dev);
+		ipoib_flush_ah(dev, 0);

Why do you need to call the flush function here?

I can't see the reason to use the flush not from the stop_ah, meaning without setting the IPOIB_STOP_REAPER, the flush can send twice the same work.

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_ib.c +1 -0

  	}
 
  	if (level >= IPOIB_FLUSH_NORMAL)

@@ -1100,6 +1102,14 @@ void ipoib_ib_dev_cleanup(struct net_device *dev)
  	ipoib_mcast_stop_thread(dev, 1);
  	ipoib_mcast_dev_flush(dev);
 
+	/*

drivers/infiniband/ulp/ipoib/ipoib_ib.c void ipoib_ib_dev_cleanup(struct net_device *dev) · line 1115

Quoted 10 lines

drivers/infiniband/ulp/ipoib/ipoib_ib.c +7 -0

+	 * All of our ah references aren't free until after
+	 * ipoib_mcast_dev_flush(), ipoib_flush_paths, and
+	 * the neighbor garbage collection is stopped and reaped.
+	 * That should all be done now, so make a final ah flush.
+	 */
+	ipoib_stop_ah(dev, 1);
+
  	ipoib_transport_dev_cleanup(dev);
  }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 79 lines · Erez Shitrit

Quoted 78 lines · Doug Ledford

Create a an ipoib_flush_ah and ipoib_stop_ah routines to use at appropriate times to flush out all remaining ah entries before we shut the device down.

Because neighbors and mcast entries can each have a reference on any given ah, we must make sure to free all of those first before our ah will actually have a 0 refcount and be able to be reaped.

This factoring is needed in preparation for having per-device work queues. The original per-device workqueue code resulted in the following error message:

<ibdev>: ib_dealloc_pd failed

That error was tracked down to this issue. With the changes to which workqueues were flushed when, there were no flushes of the per device workqueue after the last ah's were freed, resulting in an attempt to dealloc the pd with outstanding resources still allocated. This code puts the explicit flushes in the needed places to avoid that problem.

Signed-off-by Doug Ledford <[email protected]>

---
   drivers/infiniband/ulp/ipoib/ipoib_ib.c | 46 ++++++++++++++++++++-------------
   1 file changed, 28 insertions(+), 18 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_ib.c +19 -18

index 72626c34817..cb02466a0eb 100644
@@ -659,6 +659,24 @@ void ipoib_reap_ah(struct work_struct *work)
   				   round_jiffies_relative(HZ));
   }
 
+static void ipoib_flush_ah(struct net_device *dev, int flush)
+{
+	struct ipoib_dev_priv *priv = netdev_priv(dev);
+
+	cancel_delayed_work(&priv->ah_reap_task);
+	if (flush)
+		flush_workqueue(ipoib_workqueue);
+	ipoib_reap_ah(&priv->ah_reap_task.work);
+}
+
+static void ipoib_stop_ah(struct net_device *dev, int flush)
+{
+	struct ipoib_dev_priv *priv = netdev_priv(dev);
+
+	set_bit(IPOIB_STOP_REAPER, &priv->flags);
+	ipoib_flush_ah(dev, flush);
+}
+
   static void ipoib_ib_tx_timer_func(unsigned long ctx)
   {
   	drain_tx_cq((struct net_device *)ctx);

@@ -877,24 +895,7 @@ timeout:
   	if (ib_modify_qp(priv->qp, &qp_attr, IB_QP_STATE))
   		ipoib_warn(priv, "Failed to modify QP to RESET state\n");
 
-	/* Wait for all AHs to be reaped */
-	set_bit(IPOIB_STOP_REAPER, &priv->flags);
-	cancel_delayed_work(&priv->ah_reap_task);
-	if (flush)
-		flush_workqueue(ipoib_workqueue);
-
-	begin = jiffies;
-
-	while (!list_empty(&priv->dead_ahs)) {
-		__ipoib_reap_ah(dev);
-
-		if (time_after(jiffies, begin + HZ)) {
-			ipoib_warn(priv, "timing out; will leak address handles\n");
-			break;
-		}
-
-		msleep(1);
-	}
+	ipoib_flush_ah(dev, flush);
 

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_ib.c static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, · line 1041

Quoted 7 lines

drivers/infiniband/ulp/ipoib/ipoib_ib.c +1 -0

   	ib_req_notify_cq(priv->recv_cq, IB_CQ_NEXT_COMP);
 

@@ -1037,6 +1038,7 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv,
   	if (level == IPOIB_FLUSH_LIGHT) {
   		ipoib_mark_paths_invalid(dev);
   		ipoib_mcast_dev_flush(dev);
+		ipoib_flush_ah(dev, 0);

Why do you need to call the flush function here?

To remove all of the ah's that were reduced to a 0 refcount by the previous two functions prior to restarting operations. When we remove an ah, it calls ib_destroy_ah which calls all the way down into the low level driver. This was to make sure that old, stale data was removed all the way down to the card level before we started new queries for paths and ahs.

Quoted 3 lines

I can't see the reason to use the flush not from the stop_ah, meaning without setting the IPOIB_STOP_REAPER, the flush can send twice the same work.

No, it can't. The ah flush routine does not search through ahs to find ones to flush. When you delete neighbors and mcasts, they release their references to ahs. When the refcount goes to 0, the put routine puts the ah on the to-be-deleted ah list. All the flush does is take that list and delete the items. If you run the flush twice, the first run deletes all the items on the to-be-deleted list, the second run sees an empty list and does nothing.

As for using flush versus stop: the flush function cancels any delayed ah_flush work so that it isn't racing with the normally scheduled

ah_flush, then flushes the workqueue to make sure anything that might result in an ah getting freed is done, then flushes, then schedules a new delayed flush_ah work 1 second later. So, it does exactly what a flush should do: it removes what there is currently to remove, and in the case of a periodically scheduled garbage collection, schedules a new periodic flush at the maximum interval.

It is not appropriate to call stop_ah at this point because it will cancel the delayed work, flush the ahs, then never reschedule the garbage collection. If we called stop here, we would have to call start later. But that's not really necessary as the flush cancels the scheduled work and reschedules it for a second later.

Quoted 22 lines

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_ib.c +1 -0

   	}
 
   	if (level >= IPOIB_FLUSH_NORMAL)

@@ -1100,6 +1102,14 @@ void ipoib_ib_dev_cleanup(struct net_device *dev)
   	ipoib_mcast_stop_thread(dev, 1);
   	ipoib_mcast_dev_flush(dev);
 
+	/*

drivers/infiniband/ulp/ipoib/ipoib_ib.c void ipoib_ib_dev_cleanup(struct net_device *dev) · line 1115

Quoted 10 lines

drivers/infiniband/ulp/ipoib/ipoib_ib.c +7 -0

+	 * All of our ah references aren't free until after
+	 * ipoib_mcast_dev_flush(), ipoib_flush_paths, and
+	 * the neighbor garbage collection is stopped and reaped.
+	 * That should all be done now, so make a final ah flush.
+	 */
+	ipoib_stop_ah(dev, 1);
+
   	ipoib_transport_dev_cleanup(dev);
   }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Doug Ledford

Quoted 7 lines

Review static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, · line 1041

Quoted 6 lines

(quoted diff) +1 -0

@@ -1037,6 +1038,7 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv,
   	if (level == IPOIB_FLUSH_LIGHT) {
   		ipoib_mark_paths_invalid(dev);
   		ipoib_mcast_dev_flush(dev);
+		ipoib_flush_ah(dev, 0);

Why do you need to call the flush function here?

Quoted 6 lines

To remove all of the ah's that were reduced to a 0 refcount by the previous two functions prior to restarting operations. When we remove an ah, it calls ib_destroy_ah which calls all the way down into the low level driver. This was to make sure that old, stale data was removed all the way down to the card level before we started new queries for paths and ahs.

Yes. but it is not needed.

The bug happened when the driver was removed (via modprobe -r etc.), and there were ah's in the dead_ah list, that was fixed by you in the function ipoib_ib_dev_cleanup, the call that you added here is not relevant to the bug (and IMHO is not needed at all)

So, the the task of cleaning the dead_ah is already there, no need to recall it, it will be called anyway 1 sec at the most from now.

You can try that, take of that call, no harm or memory leak will happened.

Quoted 11 lines

Quoted 3 lines

I can't see the reason to use the flush not from the stop_ah, meaning without setting the IPOIB_STOP_REAPER, the flush can send twice the same work.

No, it can't. The ah flush routine does not search through ahs to find ones to flush. When you delete neighbors and mcasts, they release their references to ahs. When the refcount goes to 0, the put routine puts the ah on the to-be-deleted ah list. All the flush does is take that list and delete the items. If you run the flush twice, the first run deletes all the items on the to-be-deleted list, the second run sees an empty list and does nothing.

Quoted 2 lines

As for using flush versus stop: the flush function cancels any delayed ah_flush work so that it isn't racing with the normally scheduled

when you call cancel_delayed_work to work that can schedule itself, it is not help, the work can be at the middle of the run and re-schedule itself...

Quoted 8 lines

ah_flush, then flushes the workqueue to make sure anything that might result in an ah getting freed is done, then flushes, then schedules a new delayed flush_ah work 1 second later. So, it does exactly what a flush should do: it removes what there is currently to remove, and in the case of a periodically scheduled garbage collection, schedules a new periodic flush at the maximum interval.

Quoted 29 lines

It is not appropriate to call stop_ah at this point because it will cancel the delayed work, flush the ahs, then never reschedule the garbage collection. If we called stop here, we would have to call start later. But that's not really necessary as the flush cancels the scheduled work and reschedules it for a second later.

Quoted 22 lines

Quoted 8 lines

(quoted diff) +1 -0

   	}
 
   	if (level >= IPOIB_FLUSH_NORMAL)

@@ -1100,6 +1102,14 @@ void ipoib_ib_dev_cleanup(struct net_device *dev)
   	ipoib_mcast_stop_thread(dev, 1);
   	ipoib_mcast_dev_flush(dev);
 
+	/*

Review void ipoib_ib_dev_cleanup(struct net_device *dev) · line 1115

Quoted 10 lines

(quoted diff) +7 -0

+	 * All of our ah references aren't free until after
+	 * ipoib_mcast_dev_flush(), ipoib_flush_paths, and
+	 * the neighbor garbage collection is stopped and reaped.
+	 * That should all be done now, so make a final ah flush.
+	 */
+	ipoib_stop_ah(dev, 1);
+
   	ipoib_transport_dev_cleanup(dev);
   }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 13 lines · Erez Shitrit

Quoted 12 lines · Doug Ledford

Quoted 6 lines

Review static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, · line 1041

Quoted 5 lines

(quoted diff) +1 -0

@@ -1037,6 +1038,7 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv,
    	if (level == IPOIB_FLUSH_LIGHT) {
    		ipoib_mark_paths_invalid(dev);
    		ipoib_mcast_dev_flush(dev);
+		ipoib_flush_ah(dev, 0);

Why do you need to call the flush function here?

To remove all of the ah's that were reduced to a 0 refcount by the previous two functions prior to restarting operations. When we remove an ah, it calls ib_destroy_ah which calls all the way down into the low level driver. This was to make sure that old, stale data was removed all the way down to the card level before we started new queries for paths and ahs.

Quoted 1 line

Yes. but it is not needed.

That depends on the card. For the modern cards (mlx4, mlx5, qib), it isn't needed but doesn't hurt either. For older cards (in particular, mthca), the driver actually frees up card resources at the time of the call.

Quoted 4 lines

The bug happened when the driver was removed (via modprobe -r etc.), and there were ah's in the dead_ah list, that was fixed by you in the function ipoib_ib_dev_cleanup, the call that you added here is not relevant to the bug (and IMHO is not needed at all)

I never said that this hunk was part of the original bug I saw before.

Quoted 4 lines

So, the the task of cleaning the dead_ah is already there, no need to recall it, it will be called anyway 1 sec at the most from now.

You can try that, take of that call, no harm or memory leak will happened.

I have no doubt that it will get freed later. As I said, I never considered this particular hunk part of that original bug. But, as I point out above, there is no harm in it for any hardware, and depending on hardware it can help to make sure there isn't a shortage of resources. Given that fact, I see no reason to remove it.

Quoted 13 lines

Quoted 13 lines

Quoted 3 lines

I can't see the reason to use the flush not from the stop_ah, meaning without setting the IPOIB_STOP_REAPER, the flush can send twice the same work.

No, it can't. The ah flush routine does not search through ahs to find ones to flush. When you delete neighbors and mcasts, they release their references to ahs. When the refcount goes to 0, the put routine puts the ah on the to-be-deleted ah list. All the flush does is take that list and delete the items. If you run the flush twice, the first run deletes all the items on the to-be-deleted list, the second run sees an empty list and does nothing.

As for using flush versus stop: the flush function cancels any delayed ah_flush work so that it isn't racing with the normally scheduled

Quoted 3 lines

when you call cancel_delayed_work to work that can schedule itself, it is not help, the work can be at the middle of the run and re-schedule itself...

If it is in the middle of a run and reschedules itself, then it will schedule itself at precisely the same time we would have anyway, and we will get flushed properly, so the net result of this particular race is that we end up doing exactly what we wanted to do anyway.

Quoted 39 lines

Quoted 7 lines

ah_flush, then flushes the workqueue to make sure anything that might result in an ah getting freed is done, then flushes, then schedules a new delayed flush_ah work 1 second later. So, it does exactly what a flush should do: it removes what there is currently to remove, and in the case of a periodically scheduled garbage collection, schedules a new periodic flush at the maximum interval.

Quoted 28 lines

It is not appropriate to call stop_ah at this point because it will cancel the delayed work, flush the ahs, then never reschedule the garbage collection. If we called stop here, we would have to call start later. But that's not really necessary as the flush cancels the scheduled work and reschedules it for a second later.

Quoted 22 lines

Quoted 8 lines

(quoted diff) +1 -0

    	}
 
    	if (level >= IPOIB_FLUSH_NORMAL)

@@ -1100,6 +1102,14 @@ void ipoib_ib_dev_cleanup(struct net_device *dev)
    	ipoib_mcast_stop_thread(dev, 1);
    	ipoib_mcast_dev_flush(dev);
 
+	/*

Review void ipoib_ib_dev_cleanup(struct net_device *dev) · line 1115

Quoted 10 lines

(quoted diff) +7 -0

+	 * All of our ah references aren't free until after
+	 * ipoib_mcast_dev_flush(), ipoib_flush_paths, and
+	 * the neighbor garbage collection is stopped and reaped.
+	 * That should all be done now, so make a final ah flush.
+	 */
+	ipoib_stop_ah(dev, 1);
+
    	ipoib_transport_dev_cleanup(dev);
    }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 21 lines · Doug Ledford

Quoted 19 lines · Erez Shitrit

Quoted 16 lines · Doug Ledford

Quoted 9 lines

Review static void __ipoib_ib_dev_flush(struct · line 1042

Quoted 7 lines

(quoted diff) +1 -0

@@ -1037,6 +1038,7 @@ static void __ipoib_ib_dev_flush(struct
 ipoib_dev_priv *priv,
        if (level == IPOIB_FLUSH_LIGHT) {
                ipoib_mark_paths_invalid(dev);
                ipoib_mcast_dev_flush(dev);
+               ipoib_flush_ah(dev, 0);

Why do you need to call the flush function here?

To remove all of the ah's that were reduced to a 0 refcount by the previous two functions prior to restarting operations. When we remove an ah, it calls ib_destroy_ah which calls all the way down into the low level driver. This was to make sure that old, stale data was removed all the way down to the card level before we started new queries for paths and ahs.

Quoted 6 lines

Quoted 1 line

Yes. but it is not needed.

That depends on the card. For the modern cards (mlx4, mlx5, qib), it isn't needed but doesn't hurt either. For older cards (in particular, mthca), the driver actually frees up card resources at the time of the call.

Can you please elaborate more here, I took a look in the mthca and didn't see that.
anyway, what i don't understand is why you need to do that now, the ah is already in the dead_ah_list so, in at the most 1 sec will be cleared and if the driver goes down your other hunk fixed that.

Quoted 30 lines · Erez Shitrit

Quoted 21 lines · Doug Ledford

Quoted 19 lines · Erez Shitrit

Quoted 16 lines · Doug Ledford

Quoted 9 lines

Review static void __ipoib_ib_dev_flush(struct · line 1042

Quoted 7 lines

(quoted diff) +1 -0

@@ -1037,6 +1038,7 @@ static void __ipoib_ib_dev_flush(struct
 ipoib_dev_priv *priv,
       if (level == IPOIB_FLUSH_LIGHT) {
               ipoib_mark_paths_invalid(dev);
               ipoib_mcast_dev_flush(dev);
+               ipoib_flush_ah(dev, 0);

Why do you need to call the flush function here?

To remove all of the ah's that were reduced to a 0 refcount by the previous two functions prior to restarting operations. When we remove an ah, it calls ib_destroy_ah which calls all the way down into the low level driver. This was to make sure that old, stale data was removed all the way down to the card level before we started new queries for paths and ahs.

Quoted 6 lines

Quoted 1 line

Yes. but it is not needed.

That depends on the card. For the modern cards (mlx4, mlx5, qib), it isn't needed but doesn't hurt either. For older cards (in particular, mthca), the driver actually frees up card resources at the time of the call.

Quoted 5 lines

Can you please elaborate more here, I took a look in the mthca and didn't see that.
anyway, what i don't understand is why you need to do that now, the ah is already in the dead_ah_list so, in at the most 1 sec will be cleared and if the driver goes down your other hunk fixed that.

Doug, ten days and no response from you... lets finalize the review on this series so we have it safely done for 4.1 -- on top of it Erez prepares a set of IPoIB fixes from our internal tree and we want that for 4.1 too. Please address.

Quoted 30 lines · Or Gerlitz

Quoted 22 lines · Erez Shitrit

Quoted 15 lines · Doug Ledford

Quoted 14 lines · Erez Shitrit

Quoted 13 lines · Doug Ledford

Quoted 7 lines

Review static void __ipoib_ib_dev_flush(struct · line 1042

Quoted 6 lines

(quoted diff) +1 -0

@@ -1037,6 +1038,7 @@ static void __ipoib_ib_dev_flush(struct
 ipoib_dev_priv *priv,
        if (level == IPOIB_FLUSH_LIGHT) {
                ipoib_mark_paths_invalid(dev);
                ipoib_mcast_dev_flush(dev);
+               ipoib_flush_ah(dev, 0);

Why do you need to call the flush function here?

To remove all of the ah's that were reduced to a 0 refcount by the previous two functions prior to restarting operations. When we remove an ah, it calls ib_destroy_ah which calls all the way down into the low level driver. This was to make sure that old, stale data was removed all the way down to the card level before we started new queries for paths and ahs.

Quoted 5 lines

Quoted 1 line

Yes. but it is not needed.

That depends on the card. For the modern cards (mlx4, mlx5, qib), it isn't needed but doesn't hurt either. For older cards (in particular, mthca), the driver actually frees up card resources at the time of the call.

Quoted 5 lines

Can you please elaborate more here, I took a look in the mthca and didn't see that.
anyway, what i don't understand is why you need to do that now, the ah is already in the dead_ah_list so, in at the most 1 sec will be cleared and if the driver goes down your other hunk fixed that.

Quoted 4 lines

Doug, ten days and no response from you... lets finalize the review on this series so we have it safely done for 4.1 -- on top of it Erez prepares a set of IPoIB fixes from our internal tree and we want that for 4.1 too. Please address.

I didn’t have much to say here. I said that mthca can have card resources freed by this call, which is backed up by this code in mthca_ah.c

int mthca_destroy_ah(struct mthca_dev *dev, struct mthca_ah *ah)

{
         switch (ah->type) {
         case MTHCA_AH_ON_HCA:
                 mthca_free(&dev->av_table.alloc,
                            (ah->avdma - dev->av_table.ddr_av_base) /
                            MTHCA_AV_SIZE);
                 break;

I’m not entirely sure how Erez missed that, but it’s there and it’s what gets called when we destroy an ah (depending on the card of course). So, that represents one case where freeing the resources in a non-lazy fashion has a direct benefit. And there is no cited drawback to freeing the resources in a non-lazy fashion on a net event, so I don’t see what there is to discuss further on the issue.

—
Doug Ledford <[email protected]>
GPG Key ID: 0E572FDD

Quoted 17 lines · Doug Ledford

Quoted 5 lines

Doug, ten days and no response from you... lets finalize the review on this series so we have it safely done for 4.1 -- on top of it Erez prepares a set of IPoIB fixes from our internal tree and we want that for 4.1 too. Please address.

I didn’t have much to say here. I said that mthca can have card resources freed by this call, which is backed up by this code in mthca_ah.c

int mthca_destroy_ah(struct mthca_dev *dev, struct mthca_ah *ah)

{
        switch (ah->type) {
        case MTHCA_AH_ON_HCA:
                mthca_free(&dev->av_table.alloc,
                           (ah->avdma - dev->av_table.ddr_av_base) /
                           MTHCA_AV_SIZE);
                break;

Quoted 1 line

I’m not entirely sure how Erez missed that, but it’s there and it’s what gets called when we destroy an ah (depending on the card of course). So, that represents one case where freeing the resources in a non-lazy fashion has a direct benefit. And there is no cited drawback to freeing the resources in a non-lazy fashion on a net event, so I don’t see what there is to discuss further on the issue.

sorry, but i still don't see the connection to the device type.
It will be deleted/freed with the regular flow, like it does in the rest of the life cycle cases of the ah (in neigh_dtor, path_free, etc.), so why here it should be directly after the event?

Quoted 19 lines · Erez Shitrit

Quoted 16 lines · Doug Ledford

Quoted 4 lines

Doug, ten days and no response from you... lets finalize the review on this series so we have it safely done for 4.1 -- on top of it Erez prepares a set of IPoIB fixes from our internal tree and we want that for 4.1 too. Please address.

I didn’t have much to say here. I said that mthca can have card resources freed by this call, which is backed up by this code in mthca_ah.c

int mthca_destroy_ah(struct mthca_dev *dev, struct mthca_ah *ah)

{
         switch (ah->type) {
         case MTHCA_AH_ON_HCA:
                 mthca_free(&dev->av_table.alloc,
                            (ah->avdma - dev->av_table.ddr_av_base) /
                            MTHCA_AV_SIZE);
                 break;

Quoted 3 lines

Quoted 1 line

I’m not entirely sure how Erez missed that, but it’s there and it’s what gets called when we destroy an ah (depending on the card of course). So, that represents one case where freeing the resources in a non-lazy fashion has a direct benefit. And there is no cited drawback to freeing the resources in a non-lazy fashion on a net event, so I don’t see what there is to discuss further on the issue.

sorry, but i still don't see the connection to the device type.
It will be deleted/freed with the regular flow, like it does in the rest of the life cycle cases of the ah (in neigh_dtor, path_free, etc.), so why here it should be directly after the event?

Because it’s the right thing to do. The only reason to do lazy deletion is when there is a performance benefit to batching. There is no performance benefit to batching here. And because on certain hardware the action frees resources on the card, which are limited, doing non-lazy deletion can be beneficial. Given that there is no downside to doing the deletions in a non-lazy fashion, and that there can be an upside depending on hardware, there is no reason to stick with the lazy deletions.

—
Doug Ledford <[email protected]>
GPG Key ID: 0E572FDD

Quoted 17 lines · Doug Ledford

Quoted 5 lines

Doug, ten days and no response from you... lets finalize the review on this series so we have it safely done for 4.1 -- on top of it Erez prepares a set of IPoIB fixes from our internal tree and we want that for 4.1 too. Please address.

I didn’t have much to say here. I said that mthca can have card resources freed by this call, which is backed up by this code in mthca_ah.c

int mthca_destroy_ah(struct mthca_dev *dev, struct mthca_ah *ah)

{
       switch (ah->type) {
       case MTHCA_AH_ON_HCA:
               mthca_free(&dev->av_table.alloc,
                          (ah->avdma - dev->av_table.ddr_av_base) /
                          MTHCA_AV_SIZE);
               break;

Quoted 1 line

I’m not entirely sure how Erez missed that, but it’s there and it’s what gets called when we destroy an ah (depending on the card of course). So, that represents one case where freeing the resources in a non-lazy fashion has a direct benefit. And there is no cited drawback to freeing the resources in a non-lazy fashion on a net event, so I don’t see what there is to discuss further on the issue.

sorry, but i still don't see the connection to the device type.
It will be deleted/freed with the regular flow, like it does in the rest of the life cycle cases of the ah (in neigh_dtor, path_free, etc.), so why here it should be directly after the event?

drivers/infiniband/ulp/ipoib/ipoib_verbs.c +6 -0

@@ -236,12 +247,19 @@ out_free_send_cq:
 out_free_recv_cq:
        ib_destroy_cq(priv->recv_cq);
 
+out_cm_dev_cleanup:
+       ipoib_cm_dev_cleanup(dev);
+
+out_free_wq:
+       destroy_workqueue(priv->wq);
+       priv->wq = NULL;

drivers/infiniband/ulp/ipoib/ipoib_verbs.c +3 -0

@@ -265,11 +283,18 @@ void ipoib_transport_dev_cleanup(struct net_device *dev)
 
        ipoib_cm_dev_cleanup(dev);
 
+       if (priv->wq) {
+               flush_workqueue(priv->wq);
+               destroy_workqueue(priv->wq);

Commit a9c8ba5884 ("IPoIB: Fix usage of uninitialized multicast objects") added a new flag MCAST_JOIN_STARTED, but was not very strict in how it was used. We didn't always initialize the completion struct before we set the flag, and we didn't always call complete on the completion struct from all paths that complete it. And when we did complete it, sometimes we continued to touch the mcast entry after the completion, opening us up to possible use after free issues.

This made it less than totally effective, and certainly made its use confusing. And in the flush function we would use the presence of this flag to signal that we should wait on the completion struct, but we never cleared this flag, ever.

In order to make things clearer and aid in resolving the rtnl deadlock bug I've been chasing, I cleaned this up a bit.

1) Remove the MCAST_JOIN_STARTED flag entirely
2) Change MCAST_FLAG_BUSY so it now only means a join is in-flight
3) Test mcast->mc directly to see if we have completed ib_sa_join_multicast (using IS_ERR_OR_NULL)
4) Make sure that before setting MCAST_FLAG_BUSY we always initialize

     the mcast->done completion struct

5) Make sure that before calling complete(&mcast->done), we always clear the MCAST_FLAG_BUSY bit
6) Take the mcast_mutex before we call ib_sa_multicast_join and also take the mutex in our join callback. This forces ib_sa_multicast_join to return and set mcast->mc before we process the callback. This way, our callback can safely clear mcast->mc if there is an error on the join and we will do the right thing as a result in mcast_dev_flush.
7) Because we need the mutex to synchronize mcast->mc, we can no longer call mcast_sendonly_join directly from mcast_send and instead must add sendonly join processing to the mcast_join_task
8) Make MCAST_RUN mean that we have a working mcast subsystem, not that we have a running task. We know when we need to reschedule our join task thread and don't need a flag to tell us.
9) Add a helper for rescheduling the join task thread

A number of different races are resolved with these changes. These races existed with the old MCAST_FLAG_BUSY usage, the MCAST_JOIN_STARTED flag was an attempt to address them, and while it helped, a determined effort could still trip things up.

One race looks something like this:

Thread 1 Thread 2
ib_sa_join_multicast (as part of running restart mcast task)
alloc member
call callback
ifconfig ib0 down
wait_for_completion
callback call completes
wait_for_completion in
mcast_dev_flush completes
mcast->mc is PTR_ERR_OR_NULL so we skip ib_sa_leave_multicast
return from callback
return from ib_sa_join_multicast
set mcast->mc = return from ib_sa_multicast

We now have a permanently unbalanced join/leave issue that trips up the refcounting in core/multicast.c

Another like this:

Thread 1 Thread 2 Thread 3
ib_sa_multicast_join

                                             ifconfig ib0 down
					    priv->broadcast = NULL
                            join_complete
			                    wait_for_completion
			   mcast->mc is not yet set, so don't clear
return from ib_sa_join_multicast and set mcast->mc
			   complete
			   return -EAGAIN (making mcast->mc invalid)
			   		    call ib_sa_multicast_leave
					    on invalid mcast->mc, hang
					    forever

By holding the mutex around ib_sa_multicast_join and taking the mutex early in the callback, we force mcast->mc to be valid at the time we run the callback. This allows us to clear mcast->mc if there is an error and the join is going to fail. We do this before we complete the mcast. In this way, mcast_dev_flush always sees consistent state in regards to mcast->mc membership at the time that the wait_for_completion() returns.

Signed-off-by Doug Ledford <[email protected]>

---
  drivers/infiniband/ulp/ipoib/ipoib.h           |  11 +-
  drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 355 ++++++++++++++++---------
  2 files changed, 238 insertions(+), 128 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib.h +9 -2

index 9ef432ae72e..c79dcd5ee8a 100644
@@ -98,9 +98,15 @@ enum {
 
  	IPOIB_MCAST_FLAG_FOUND	  = 0,	/* used in set_multicast_list */
  	IPOIB_MCAST_FLAG_SENDONLY = 1,
-	IPOIB_MCAST_FLAG_BUSY	  = 2,	/* joining or already joined */
+	/*
+	 * For IPOIB_MCAST_FLAG_BUSY
+	 * When set, in flight join and mcast->mc is unreliable
+	 * When clear and mcast->mc IS_ERR_OR_NULL, need to restart or
+	 *   haven't started yet
+	 * When clear and mcast->mc is valid pointer, join was successful
+	 */
+	IPOIB_MCAST_FLAG_BUSY	  = 2,
  	IPOIB_MCAST_FLAG_ATTACHED = 3,
-	IPOIB_MCAST_JOIN_STARTED  = 4,
 
  	MAX_SEND_CQE		  = 16,
  	IPOIB_CM_COPYBREAK	  = 256,

@@ -148,6 +154,7 @@ struct ipoib_mcast {
 
  	unsigned long created;
  	unsigned long backoff;
+	unsigned long delay_until;
 
  	unsigned long flags;
  	unsigned char logcount;

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +0 -0

index bb1b69904f9..277e7ac7c4d 100644
@@ -66,6 +66,48 @@ struct ipoib_mcast_iter {
  	unsigned int       send_only;

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +214 -126

+		return;
+
+	/*
+	 * We will be scheduling *something*, so cancel whatever is
+	 * currently scheduled first
+	 */
+	cancel_delayed_work(&priv->mcast_task);
+	if (mcast && delay) {
+		/*
+		 * We had a failure and want to schedule a retry later
+		 */
+		mcast->backoff *= 2;
+		if (mcast->backoff > IPOIB_MAX_BACKOFF_SECONDS)
+			mcast->backoff = IPOIB_MAX_BACKOFF_SECONDS;
+		mcast->delay_until = jiffies + (mcast->backoff * HZ);
+		/*
+		 * Mark this mcast for its delay, but restart the
+		 * task immediately.  The join task will make sure to
+		 * clear out all entries without delays, and then
+		 * schedule itself to run again when the earliest
+		 * delay expires
+		 */
+		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
+	} else if (delay) {
+		/*
+		 * Special case of retrying after a failure to
+		 * allocate the broadcast multicast group, wait
+		 * 1 second and try again
+		 */
+		queue_delayed_work(priv->wq, &priv->mcast_task, HZ);
+	} else
+		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
+}
+
  static void ipoib_mcast_free(struct ipoib_mcast *mcast)
  {
  	struct net_device *dev = mcast->dev;

@@ -103,6 +145,7 @@ static struct ipoib_mcast *ipoib_mcast_alloc(struct net_device *dev,
 
  	mcast->dev = dev;
  	mcast->created = jiffies;
+	mcast->delay_until = jiffies;
  	mcast->backoff = 1;
 
  	INIT_LIST_HEAD(&mcast->list);

@@ -270,17 +313,31 @@ ipoib_mcast_sendonly_join_complete(int status,
  {
  	struct ipoib_mcast *mcast = multicast->context;
  	struct net_device *dev = mcast->dev;
+	struct ipoib_dev_priv *priv = netdev_priv(dev);
+
+	/*
+	 * We have to take the mutex to force mcast_sendonly_join to
+	 * return from ib_sa_multicast_join and set mcast->mc to a
+	 * valid value.  Otherwise we were racing with ourselves in
+	 * that we might fail here, but get a valid return from
+	 * ib_sa_multicast_join after we had cleared mcast->mc here,
+	 * resulting in mis-matched joins and leaves and a deadlock
+	 */
+	mutex_lock(&mcast_mutex);
 
  	/* We trap for port events ourselves. */
-	if (status == -ENETRESET)
-		return 0;
+	if (status == -ENETRESET) {
+		status = 0;
+		goto out;
+	}
 
  	if (!status)
  		status = ipoib_mcast_join_finish(mcast, &multicast->rec);
 
  	if (status) {
  		if (mcast->logcount++ < 20)
-			ipoib_dbg_mcast(netdev_priv(dev), "multicast join failed for %pI6, status %d\n",
+			ipoib_dbg_mcast(netdev_priv(dev), "sendonly multicast "
+					"join failed for %pI6, status %d\n",
  					mcast->mcmember.mgid.raw, status);
 
  		/* Flush out any queued packets */

@@ -290,11 +347,18 @@ ipoib_mcast_sendonly_join_complete(int status,
  			dev_kfree_skb_any(skb_dequeue(&mcast->pkt_queue));
  		}
  		netif_tx_unlock_bh(dev);
-
-		/* Clear the busy flag so we try again */
-		status = test_and_clear_bit(IPOIB_MCAST_FLAG_BUSY,
-					    &mcast->flags);
+		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
+	} else {
+		mcast->backoff = 1;
+		mcast->delay_until = jiffies;
+		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
  	}
+out:
+	clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
+	if (status)
+		mcast->mc = NULL;
+	complete(&mcast->done);
+	mutex_unlock(&mcast_mutex);
  	return status;
  }
 

@@ -312,19 +376,18 @@ static int ipoib_mcast_sendonly_join(struct ipoib_mcast *mcast)
  	int ret = 0;
 
  	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags)) {
-		ipoib_dbg_mcast(priv, "device shutting down, no multicast joins\n");
+		ipoib_dbg_mcast(priv, "device shutting down, no sendonly "
+				"multicast joins\n");
+		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
+		complete(&mcast->done);
  		return -ENODEV;
  	}
 
-	if (test_and_set_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags)) {
-		ipoib_dbg_mcast(priv, "multicast entry busy, skipping\n");
-		return -EBUSY;
-	}
-
  	rec.mgid     = mcast->mcmember.mgid;
  	rec.port_gid = priv->local_gid;
  	rec.pkey     = cpu_to_be16(priv->pkey);
 
+	mutex_lock(&mcast_mutex);
  	mcast->mc = ib_sa_join_multicast(&ipoib_sa_client, priv->ca,
  					 priv->port, &rec,
  					 IB_SA_MCMEMBER_REC_MGID	|

@@ -337,12 +400,14 @@ static int ipoib_mcast_sendonly_join(struct ipoib_mcast *mcast)
  	if (IS_ERR(mcast->mc)) {
  		ret = PTR_ERR(mcast->mc);
  		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-		ipoib_warn(priv, "ib_sa_join_multicast failed (ret = %d)\n",
-			   ret);
+		ipoib_warn(priv, "ib_sa_join_multicast for sendonly join "
+			   "failed (ret = %d)\n", ret);
+		complete(&mcast->done);
  	} else {
-		ipoib_dbg_mcast(priv, "no multicast record for %pI6, starting join\n",
-				mcast->mcmember.mgid.raw);
+		ipoib_dbg_mcast(priv, "no multicast record for %pI6, starting "
+				"sendonly join\n", mcast->mcmember.mgid.raw);
  	}
+	mutex_unlock(&mcast_mutex);
 
  	return ret;
  }

@@ -390,6 +455,16 @@ static int ipoib_mcast_join_complete(int status,
  	ipoib_dbg_mcast(priv, "join completion for %pI6 (status %d)\n",
  			mcast->mcmember.mgid.raw, status);
 
+	/*
+	 * We have to take the mutex to force mcast_join to
+	 * return from ib_sa_multicast_join and set mcast->mc to a
+	 * valid value.  Otherwise we were racing with ourselves in
+	 * that we might fail here, but get a valid return from
+	 * ib_sa_multicast_join after we had cleared mcast->mc here,
+	 * resulting in mis-matched joins and leaves and a deadlock
+	 */
+	mutex_lock(&mcast_mutex);
+
  	/* We trap for port events ourselves. */
  	if (status == -ENETRESET) {
  		status = 0;

@@ -401,10 +476,8 @@ static int ipoib_mcast_join_complete(int status,
 
  	if (!status) {
  		mcast->backoff = 1;
-		mutex_lock(&mcast_mutex);
-		if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
-			queue_delayed_work(priv->wq, &priv->mcast_task, 0);
-		mutex_unlock(&mcast_mutex);
+		mcast->delay_until = jiffies;
+		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
 
  		/*
  		 * Defer carrier on work to priv->wq to avoid a

@@ -412,37 +485,26 @@ static int ipoib_mcast_join_complete(int status,
  		 */
  		if (mcast == priv->broadcast)
  			queue_work(priv->wq, &priv->carrier_on_task);
-
-		status = 0;
-		goto out;
-	}
-
-	if (mcast->logcount++ < 20) {
-		if (status == -ETIMEDOUT || status == -EAGAIN) {
-			ipoib_dbg_mcast(priv, "multicast join failed for %pI6, status %d\n",
-					mcast->mcmember.mgid.raw, status);
-		} else {
-			ipoib_warn(priv, "multicast join failed for %pI6, status %d\n",
-				   mcast->mcmember.mgid.raw, status);
+	} else {
+		if (mcast->logcount++ < 20) {
+			if (status == -ETIMEDOUT || status == -EAGAIN) {
+				ipoib_dbg_mcast(priv, "multicast join failed for %pI6, status %d\n",
+						mcast->mcmember.mgid.raw, status);
+			} else {
+				ipoib_warn(priv, "multicast join failed for %pI6, status %d\n",
+					   mcast->mcmember.mgid.raw, status);
+			}
  		}
-	}
-
-	mcast->backoff *= 2;
-	if (mcast->backoff > IPOIB_MAX_BACKOFF_SECONDS)
-		mcast->backoff = IPOIB_MAX_BACKOFF_SECONDS;
 
-	/* Clear the busy flag so we try again */
-	status = test_and_clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-
-	mutex_lock(&mcast_mutex);
-	spin_lock_irq(&priv->lock);
-	if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
-		queue_delayed_work(priv->wq, &priv->mcast_task,
-				   mcast->backoff * HZ);
-	spin_unlock_irq(&priv->lock);
-	mutex_unlock(&mcast_mutex);
+		/* Requeue this join task with a backoff delay */
+		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
+	}
  out:
+	clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
+	if (status)
+		mcast->mc = NULL;
  	complete(&mcast->done);
+	mutex_unlock(&mcast_mutex);
  	return status;
  }
 

@@ -491,29 +553,18 @@ static void ipoib_mcast_join(struct net_device *dev, struct ipoib_mcast *mcast,
  		rec.hop_limit	  = priv->broadcast->mcmember.hop_limit;
  	}
 
-	set_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-	init_completion(&mcast->done);
-	set_bit(IPOIB_MCAST_JOIN_STARTED, &mcast->flags);
-
+	mutex_lock(&mcast_mutex);
  	mcast->mc = ib_sa_join_multicast(&ipoib_sa_client, priv->ca, priv->port,
  					 &rec, comp_mask, GFP_KERNEL,
  					 ipoib_mcast_join_complete, mcast);
  	if (IS_ERR(mcast->mc)) {
  		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-		complete(&mcast->done);
  		ret = PTR_ERR(mcast->mc);
  		ipoib_warn(priv, "ib_sa_join_multicast failed, status %d\n", ret);
-
-		mcast->backoff *= 2;
-		if (mcast->backoff > IPOIB_MAX_BACKOFF_SECONDS)
-			mcast->backoff = IPOIB_MAX_BACKOFF_SECONDS;
-
-		mutex_lock(&mcast_mutex);
-		if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
-			queue_delayed_work(priv->wq, &priv->mcast_task,
-					   mcast->backoff * HZ);
-		mutex_unlock(&mcast_mutex);
+		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
+		complete(&mcast->done);
  	}
+	mutex_unlock(&mcast_mutex);
  }
 
  void ipoib_mcast_join_task(struct work_struct *work)

@@ -522,6 +573,9 @@ void ipoib_mcast_join_task(struct work_struct *work)
  		container_of(work, struct ipoib_dev_priv, mcast_task.work);
  	struct net_device *dev = priv->dev;
  	struct ib_port_attr port_attr;
+	unsigned long delay_until = 0;
+	struct ipoib_mcast *mcast = NULL;
+	int create = 1;
 
  	if (!test_bit(IPOIB_MCAST_RUN, &priv->flags))
  		return;

@@ -539,64 +593,102 @@ void ipoib_mcast_join_task(struct work_struct *work)
  	else
  		memcpy(priv->dev->dev_addr + 4, priv->local_gid.raw, sizeof (union ib_gid));
 
+	/*
+	 * We have to hold the mutex to keep from racing with the join
+	 * completion threads on setting flags on mcasts, and we have
+	 * to hold the priv->lock because dev_flush will remove entries
+	 * out from underneath us, so at a minimum we need the lock
+	 * through the time that we do the for_each loop of the mcast
+	 * list or else dev_flush can make us oops.
+	 */
+	mutex_lock(&mcast_mutex);
+	spin_lock_irq(&priv->lock);
+	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
+		goto out;
+
  	if (!priv->broadcast) {
  		struct ipoib_mcast *broadcast;
 
-		if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
-			return;
-
-		broadcast = ipoib_mcast_alloc(dev, 1);
+		broadcast = ipoib_mcast_alloc(dev, 0);
  		if (!broadcast) {
  			ipoib_warn(priv, "failed to allocate broadcast group\n");
-			mutex_lock(&mcast_mutex);
-			if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
-				queue_delayed_work(priv->wq, &priv->mcast_task,
-						   HZ);
-			mutex_unlock(&mcast_mutex);
-			return;
+			/*
+			 * Restart us after a 1 second delay to retry
+			 * creating our broadcast group and attaching to
+			 * it.  Until this succeeds, this ipoib dev is
+			 * completely stalled (multicast wise).
+			 */
+			__ipoib_mcast_schedule_join_thread(priv, NULL, 1);
+			goto out;
  		}
 
-		spin_lock_irq(&priv->lock);
  		memcpy(broadcast->mcmember.mgid.raw, priv->dev->broadcast + 4,
  		       sizeof (union ib_gid));
  		priv->broadcast = broadcast;
 
  		__ipoib_mcast_add(dev, priv->broadcast);
-		spin_unlock_irq(&priv->lock);
  	}
 
  	if (!test_bit(IPOIB_MCAST_FLAG_ATTACHED, &priv->broadcast->flags)) {
-		if (!test_bit(IPOIB_MCAST_FLAG_BUSY, &priv->broadcast->flags))
-			ipoib_mcast_join(dev, priv->broadcast, 0);
-		return;
+		if (IS_ERR_OR_NULL(priv->broadcast->mc) &&
+		    !test_bit(IPOIB_MCAST_FLAG_BUSY, &priv->broadcast->flags)) {
+			mcast = priv->broadcast;
+			create = 0;
+			if (mcast->backoff > 1 &&
+			    time_before(jiffies, mcast->delay_until)) {
+				delay_until = mcast->delay_until;
+				mcast = NULL;
+			}
+		}
+		goto out;
  	}
 
-	while (1) {
-		struct ipoib_mcast *mcast = NULL;
-
-		spin_lock_irq(&priv->lock);
-		list_for_each_entry(mcast, &priv->multicast_list, list) {
-			if (!test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags)
-			    && !test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags)
-			    && !test_bit(IPOIB_MCAST_FLAG_ATTACHED, &mcast->flags)) {
+	/*
+	 * We'll never get here until the broadcast group is both allocated
+	 * and attached
+	 */
+	list_for_each_entry(mcast, &priv->multicast_list, list) {
+		if (IS_ERR_OR_NULL(mcast->mc) &&
+		    !test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags) &&
+		    !test_bit(IPOIB_MCAST_FLAG_ATTACHED, &mcast->flags)) {
+			if (mcast->backoff == 1 ||
+			    time_after_eq(jiffies, mcast->delay_until))
  				/* Found the next unjoined group */
  				break;
-			}
+			else if (!delay_until ||
+				 time_before(mcast->delay_until, delay_until))
+				delay_until = mcast->delay_until;
  		}
-		spin_unlock_irq(&priv->lock);
-
-		if (&mcast->list == &priv->multicast_list) {
-			/* All done */
-			break;
-		}
-
-		ipoib_mcast_join(dev, mcast, 1);
-		return;
  	}
 
-	ipoib_dbg_mcast(priv, "successfully joined all multicast groups\n");
+	if (&mcast->list == &priv->multicast_list) {
+		/*
+		 * All done, unless we have delayed work from
+		 * backoff retransmissions, but we will get
+		 * restarted when the time is right, so we are
+		 * done for now
+		 */
+		mcast = NULL;
+		ipoib_dbg_mcast(priv, "successfully joined all "
+				"multicast groups\n");
+	}
 
-	clear_bit(IPOIB_MCAST_RUN, &priv->flags);
+out:
+	if (mcast) {
+		init_completion(&mcast->done);
+		set_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
+	}
+	spin_unlock_irq(&priv->lock);
+	mutex_unlock(&mcast_mutex);
+	if (mcast) {
+		if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
+			ipoib_mcast_sendonly_join(mcast);
+		else
+			ipoib_mcast_join(dev, mcast, create);
+	}
+	if (delay_until)
+		queue_delayed_work(priv->wq, &priv->mcast_task,
+				   delay_until - jiffies);
  }
 
  int ipoib_mcast_start_thread(struct net_device *dev)

@@ -606,8 +698,8 @@ int ipoib_mcast_start_thread(struct net_device *dev)
  	ipoib_dbg_mcast(priv, "starting multicast thread\n");
 
  	mutex_lock(&mcast_mutex);
-	if (!test_and_set_bit(IPOIB_MCAST_RUN, &priv->flags))
-		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
+	set_bit(IPOIB_MCAST_RUN, &priv->flags);
+	__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
  	mutex_unlock(&mcast_mutex);
 
  	return 0;

@@ -635,7 +727,12 @@ static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
  	int ret = 0;
 
  	if (test_and_clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags))
+		ipoib_warn(priv, "ipoib_mcast_leave on an in-flight join\n");
+
+	if (!IS_ERR_OR_NULL(mcast->mc))
  		ib_sa_free_multicast(mcast->mc);
+	else
+		ipoib_dbg(priv, "ipoib_mcast_leave with mcast->mc invalid\n");
 
  	if (test_and_clear_bit(IPOIB_MCAST_FLAG_ATTACHED, &mcast->flags)) {
  		ipoib_dbg_mcast(priv, "leaving MGID %pI6\n",

@@ -646,7 +743,9 @@ static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
  				      be16_to_cpu(mcast->mcmember.mlid));
  		if (ret)
  			ipoib_warn(priv, "ib_detach_mcast failed (result = %d)\n", ret);
-	}
+	} else if (!test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
+		ipoib_dbg(priv, "leaving with no mcmember but not a "
+			  "SENDONLY join\n");
 
  	return 0;
  }

@@ -687,6 +786,7 @@ void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb)
  		memcpy(mcast->mcmember.mgid.raw, mgid, sizeof (union ib_gid));
  		__ipoib_mcast_add(dev, mcast);
  		list_add_tail(&mcast->list, &priv->multicast_list);
+		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
  	}
 
  	if (!mcast->ah) {

@@ -696,13 +796,6 @@ void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb)
  			++dev->stats.tx_dropped;
  			dev_kfree_skb_any(skb);
  		}
-
-		if (test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags))
-			ipoib_dbg_mcast(priv, "no address vector, "
-					"but multicast join already started\n");
-		else if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
-			ipoib_mcast_sendonly_join(mcast);
-
  		/*
  		 * If lookup completes between here and out:, don't
  		 * want to send packet twice.

@@ -761,9 +854,12 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
 
  	spin_unlock_irqrestore(&priv->lock, flags);
 
-	/* seperate between the wait to the leave*/
+	/*
+	 * make sure the in-flight joins have finished before we attempt
+	 * to leave
+	 */
  	list_for_each_entry_safe(mcast, tmcast, &remove_list, list)
-		if (test_bit(IPOIB_MCAST_JOIN_STARTED, &mcast->flags))
+		if (test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags))
  			wait_for_completion(&mcast->done);
 
  	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {

@@ -794,20 +890,14 @@ void ipoib_mcast_restart_task(struct work_struct *work)
  	unsigned long flags;
  	struct ib_sa_mcmember_rec rec;
 
-	ipoib_dbg_mcast(priv, "restarting multicast task\n");
+	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
+		/*
+		 * shortcut...on shutdown flush is called next, just
+		 * let it do all the work
+		 */
+		return;
 
-	/*
-	 * We're running on the priv->wq right now, so we can't call
-	 * mcast_stop_thread as it wants to flush the wq and that
-	 * will deadlock.  We don't actually *need* to stop the
-	 * thread here anyway, so just clear the run flag, cancel
-	 * any delayed work, do our work, remove the old entries,
-	 * then restart the thread.
-	 */
-	mutex_lock(&mcast_mutex);
-	clear_bit(IPOIB_MCAST_RUN, &priv->flags);
-	cancel_delayed_work(&priv->mcast_task);
-	mutex_unlock(&mcast_mutex);
+	ipoib_dbg_mcast(priv, "restarting multicast task\n");
 
  	local_irq_save(flags);
  	netif_addr_lock(dev);

@@ -893,14 +983,27 @@ void ipoib_mcast_restart_task(struct work_struct *work)
  	netif_addr_unlock(dev);
  	local_irq_restore(flags);
 
-	/* We have to cancel outside of the spinlock */
+	/*
+	 * make sure the in-flight joins have finished before we attempt
+	 * to leave
+	 */
+	list_for_each_entry_safe(mcast, tmcast, &remove_list, list)
+		if (test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags))
+			wait_for_completion(&mcast->done);
+
  	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
  		ipoib_mcast_leave(mcast->dev, mcast);
  		ipoib_mcast_free(mcast);
  	}
 
-	if (test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
-		ipoib_mcast_start_thread(dev);
+	/*

Quoted 131 lines · Doug Ledford

Commit a9c8ba5884 ("IPoIB: Fix usage of uninitialized multicast objects") added a new flag MCAST_JOIN_STARTED, but was not very strict in how it was used. We didn't always initialize the completion struct before we set the flag, and we didn't always call complete on the completion struct from all paths that complete it. And when we did complete it, sometimes we continued to touch the mcast entry after the completion, opening us up to possible use after free issues.

This made it less than totally effective, and certainly made its use confusing. And in the flush function we would use the presence of this flag to signal that we should wait on the completion struct, but we never cleared this flag, ever.

In order to make things clearer and aid in resolving the rtnl deadlock bug I've been chasing, I cleaned this up a bit.

1) Remove the MCAST_JOIN_STARTED flag entirely
2) Change MCAST_FLAG_BUSY so it now only means a join is in-flight
3) Test mcast->mc directly to see if we have completed ib_sa_join_multicast (using IS_ERR_OR_NULL)
4) Make sure that before setting MCAST_FLAG_BUSY we always initialize

     the mcast->done completion struct

5) Make sure that before calling complete(&mcast->done), we always clear the MCAST_FLAG_BUSY bit
6) Take the mcast_mutex before we call ib_sa_multicast_join and also take the mutex in our join callback. This forces ib_sa_multicast_join to return and set mcast->mc before we process the callback. This way, our callback can safely clear mcast->mc if there is an error on the join and we will do the right thing as a result in mcast_dev_flush.
7) Because we need the mutex to synchronize mcast->mc, we can no longer call mcast_sendonly_join directly from mcast_send and instead must add sendonly join processing to the mcast_join_task
8) Make MCAST_RUN mean that we have a working mcast subsystem, not that we have a running task. We know when we need to reschedule our join task thread and don't need a flag to tell us.
9) Add a helper for rescheduling the join task thread

A number of different races are resolved with these changes. These races existed with the old MCAST_FLAG_BUSY usage, the MCAST_JOIN_STARTED flag was an attempt to address them, and while it helped, a determined effort could still trip things up.

One race looks something like this:

Thread 1 Thread 2
ib_sa_join_multicast (as part of running restart mcast task)
alloc member
call callback
ifconfig ib0 down
wait_for_completion
callback call completes
wait_for_completion in
mcast_dev_flush completes
mcast->mc is PTR_ERR_OR_NULL so we skip ib_sa_leave_multicast
return from callback
return from ib_sa_join_multicast
set mcast->mc = return from ib_sa_multicast

We now have a permanently unbalanced join/leave issue that trips up the refcounting in core/multicast.c

Another like this:

Thread 1 Thread 2 Thread 3
ib_sa_multicast_join

                                             ifconfig ib0 down
					    priv->broadcast = NULL
                            join_complete
			                    wait_for_completion
			   mcast->mc is not yet set, so don't clear
return from ib_sa_join_multicast and set mcast->mc
			   complete
			   return -EAGAIN (making mcast->mc invalid)
			   		    call ib_sa_multicast_leave
					    on invalid mcast->mc, hang
					    forever

By holding the mutex around ib_sa_multicast_join and taking the mutex early in the callback, we force mcast->mc to be valid at the time we run the callback. This allows us to clear mcast->mc if there is an error and the join is going to fail. We do this before we complete the mcast. In this way, mcast_dev_flush always sees consistent state in regards to mcast->mc membership at the time that the wait_for_completion() returns.

Signed-off-by Doug Ledford <[email protected]>

---
  drivers/infiniband/ulp/ipoib/ipoib.h           |  11 +-
  drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 355 ++++++++++++++++---------
  2 files changed, 238 insertions(+), 128 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib.h +9 -2

index 9ef432ae72e..c79dcd5ee8a 100644
@@ -98,9 +98,15 @@ enum {
 
  	IPOIB_MCAST_FLAG_FOUND	  = 0,	/* used in set_multicast_list */
  	IPOIB_MCAST_FLAG_SENDONLY = 1,
-	IPOIB_MCAST_FLAG_BUSY	  = 2,	/* joining or already joined */
+	/*
+	 * For IPOIB_MCAST_FLAG_BUSY
+	 * When set, in flight join and mcast->mc is unreliable
+	 * When clear and mcast->mc IS_ERR_OR_NULL, need to restart or
+	 *   haven't started yet
+	 * When clear and mcast->mc is valid pointer, join was successful
+	 */
+	IPOIB_MCAST_FLAG_BUSY	  = 2,
  	IPOIB_MCAST_FLAG_ATTACHED = 3,
-	IPOIB_MCAST_JOIN_STARTED  = 4,
 
  	MAX_SEND_CQE		  = 16,
  	IPOIB_CM_COPYBREAK	  = 256,

@@ -148,6 +154,7 @@ struct ipoib_mcast {
 
  	unsigned long created;
  	unsigned long backoff;
+	unsigned long delay_until;
 
  	unsigned long flags;
  	unsigned char logcount;

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +0 -0

index bb1b69904f9..277e7ac7c4d 100644
@@ -66,6 +66,48 @@ struct ipoib_mcast_iter {
  	unsigned int       send_only;
  };
 

drivers/infiniband/ulp/ipoib/ipoib_multicast.c struct ipoib_mcast_iter { · line 76

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +8 -0

+/*
+ * This should be called with the mcast_mutex held
+ */
+static void __ipoib_mcast_schedule_join_thread(struct ipoib_dev_priv *priv,
+					       struct ipoib_mcast *mcast,
+					       bool delay)
+{
+	if (!test_bit(IPOIB_MCAST_RUN, &priv->flags))

You don't need the flag IPOIB_MCAST_RUN, it is duplicated of IPOIB_FLAG_OPER_UP
probably, need to be taken from all places (including ipoib.h file).

Quoted 132 lines · Erez Shitrit

Quoted 131 lines · Doug Ledford

Commit a9c8ba5884 ("IPoIB: Fix usage of uninitialized multicast objects") added a new flag MCAST_JOIN_STARTED, but was not very strict in how it was used. We didn't always initialize the completion struct before we set the flag, and we didn't always call complete on the completion struct from all paths that complete it. And when we did complete it, sometimes we continued to touch the mcast entry after the completion, opening us up to possible use after free issues.

This made it less than totally effective, and certainly made its use confusing. And in the flush function we would use the presence of this flag to signal that we should wait on the completion struct, but we never cleared this flag, ever.

In order to make things clearer and aid in resolving the rtnl deadlock bug I've been chasing, I cleaned this up a bit.

1) Remove the MCAST_JOIN_STARTED flag entirely
2) Change MCAST_FLAG_BUSY so it now only means a join is in-flight
3) Test mcast->mc directly to see if we have completed ib_sa_join_multicast (using IS_ERR_OR_NULL)
4) Make sure that before setting MCAST_FLAG_BUSY we always initialize

      the mcast->done completion struct

5) Make sure that before calling complete(&mcast->done), we always clear the MCAST_FLAG_BUSY bit
6) Take the mcast_mutex before we call ib_sa_multicast_join and also take the mutex in our join callback. This forces ib_sa_multicast_join to return and set mcast->mc before we process the callback. This way, our callback can safely clear mcast->mc if there is an error on the join and we will do the right thing as a result in mcast_dev_flush.
7) Because we need the mutex to synchronize mcast->mc, we can no longer call mcast_sendonly_join directly from mcast_send and instead must add sendonly join processing to the mcast_join_task
8) Make MCAST_RUN mean that we have a working mcast subsystem, not that we have a running task. We know when we need to reschedule our join task thread and don't need a flag to tell us.
9) Add a helper for rescheduling the join task thread

A number of different races are resolved with these changes. These races existed with the old MCAST_FLAG_BUSY usage, the MCAST_JOIN_STARTED flag was an attempt to address them, and while it helped, a determined effort could still trip things up.

One race looks something like this:

Thread 1 Thread 2
ib_sa_join_multicast (as part of running restart mcast task)
alloc member
call callback
ifconfig ib0 down
wait_for_completion
callback call completes
wait_for_completion in
mcast_dev_flush completes
mcast->mc is PTR_ERR_OR_NULL so we skip ib_sa_leave_multicast
return from callback
return from ib_sa_join_multicast
set mcast->mc = return from ib_sa_multicast

We now have a permanently unbalanced join/leave issue that trips up the refcounting in core/multicast.c

Another like this:

Thread 1 Thread 2 Thread 3
ib_sa_multicast_join

                                              ifconfig ib0 down
					    priv->broadcast = NULL
                             join_complete
			                    wait_for_completion
			   mcast->mc is not yet set, so don't clear
return from ib_sa_join_multicast and set mcast->mc
			   complete
			   return -EAGAIN (making mcast->mc invalid)
			   		    call ib_sa_multicast_leave
					    on invalid mcast->mc, hang
					    forever

By holding the mutex around ib_sa_multicast_join and taking the mutex early in the callback, we force mcast->mc to be valid at the time we run the callback. This allows us to clear mcast->mc if there is an error and the join is going to fail. We do this before we complete the mcast. In this way, mcast_dev_flush always sees consistent state in regards to mcast->mc membership at the time that the wait_for_completion() returns.

Signed-off-by Doug Ledford <[email protected]>

---
   drivers/infiniband/ulp/ipoib/ipoib.h           |  11 +-
   drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 355 ++++++++++++++++---------
   2 files changed, 238 insertions(+), 128 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib.h +9 -2

index 9ef432ae72e..c79dcd5ee8a 100644
@@ -98,9 +98,15 @@ enum {
 
   	IPOIB_MCAST_FLAG_FOUND	  = 0,	/* used in set_multicast_list */
   	IPOIB_MCAST_FLAG_SENDONLY = 1,
-	IPOIB_MCAST_FLAG_BUSY	  = 2,	/* joining or already joined */
+	/*
+	 * For IPOIB_MCAST_FLAG_BUSY
+	 * When set, in flight join and mcast->mc is unreliable
+	 * When clear and mcast->mc IS_ERR_OR_NULL, need to restart or
+	 *   haven't started yet
+	 * When clear and mcast->mc is valid pointer, join was successful
+	 */
+	IPOIB_MCAST_FLAG_BUSY	  = 2,
   	IPOIB_MCAST_FLAG_ATTACHED = 3,
-	IPOIB_MCAST_JOIN_STARTED  = 4,
 
   	MAX_SEND_CQE		  = 16,
   	IPOIB_CM_COPYBREAK	  = 256,

@@ -148,6 +154,7 @@ struct ipoib_mcast {
 
   	unsigned long created;
   	unsigned long backoff;
+	unsigned long delay_until;
 
   	unsigned long flags;
   	unsigned char logcount;

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +0 -0

index bb1b69904f9..277e7ac7c4d 100644
@@ -66,6 +66,48 @@ struct ipoib_mcast_iter {
   	unsigned int       send_only;
   };
 

Quoted 11 lines

drivers/infiniband/ulp/ipoib/ipoib_multicast.c struct ipoib_mcast_iter { · line 76

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +8 -0

+/*
+ * This should be called with the mcast_mutex held
+ */
+static void __ipoib_mcast_schedule_join_thread(struct ipoib_dev_priv *priv,
+					       struct ipoib_mcast *mcast,
+					       bool delay)
+{
+	if (!test_bit(IPOIB_MCAST_RUN, &priv->flags))

You don't need the flag IPOIB_MCAST_RUN, it is duplicated of IPOIB_FLAG_OPER_UP
probably, need to be taken from all places (including ipoib.h file).

This is probably true, however I skipped it for this series of patches. It wasn't a requirement of proper operation, and depending on where in the patch series you tried to inject this change, it had unintended negative consequences. In particular, up until patch 7/9, mcast_restart_task used to do a hand rolled stop thread and a matching start_thread and so couldn't use FLAG_OPER_UP because we needed FLAG_OPER_UP to tell us whether or not to restart the thread.

Allow the ipoib layer to attempt to join all outstanding multicast groups at once. The ib_sa layer will serialize multiple attempts to join the same group, but will process attempts to join different groups in parallel. Take advantage of that.

In order to make this happen, change the mcast_join_thread to loop through all needed joins, sending a join request for each one that we still need to join. There are a few special cases we handle though:

1) Don't attempt to join anything but the broadcast group until the join of the broadcast group has succeeded.
2) No longer restart the join task at the end of completion handling. If we completed successfully, we are done. The join task now needs kicked either by mcast_send or mcast_restart_task or mcast_start_thread, but should not need started anytime else except when scheduling a backoff attempt to rejoin.
3) No longer use separate join/completion routines for regular and sendonly joins, pass them all through the same routine and just do the right thing based on the SENDONLY join flag.
4) Only try to join a SENDONLY join twice, then drop the packets and quit trying. We leave the mcast group in the list so that if we get a new packet, all that we have to do is queue up the packet and restart the join task and it will automatically try to join twice and then either send or flush the queue again.

Signed-off-by Doug Ledford <[email protected]>

---
  drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 250 ++++++++-----------------
  1 file changed, 82 insertions(+), 168 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +15 -113

index 277e7ac7c4d..c670d9c2cda 100644
@@ -307,111 +307,6 @@ static int ipoib_mcast_join_finish(struct ipoib_mcast *mcast,
  	return 0;
  }
 
-static int
-ipoib_mcast_sendonly_join_complete(int status,
-				   struct ib_sa_multicast *multicast)
-{
-	struct ipoib_mcast *mcast = multicast->context;
-	struct net_device *dev = mcast->dev;
-	struct ipoib_dev_priv *priv = netdev_priv(dev);
-
-	/*
-	 * We have to take the mutex to force mcast_sendonly_join to
-	 * return from ib_sa_multicast_join and set mcast->mc to a
-	 * valid value.  Otherwise we were racing with ourselves in
-	 * that we might fail here, but get a valid return from
-	 * ib_sa_multicast_join after we had cleared mcast->mc here,
-	 * resulting in mis-matched joins and leaves and a deadlock
-	 */
-	mutex_lock(&mcast_mutex);
-
-	/* We trap for port events ourselves. */
-	if (status == -ENETRESET) {
-		status = 0;
-		goto out;
-	}
-
-	if (!status)
-		status = ipoib_mcast_join_finish(mcast, &multicast->rec);
-
-	if (status) {
-		if (mcast->logcount++ < 20)
-			ipoib_dbg_mcast(netdev_priv(dev), "sendonly multicast "
-					"join failed for %pI6, status %d\n",
-					mcast->mcmember.mgid.raw, status);
-
-		/* Flush out any queued packets */
-		netif_tx_lock_bh(dev);
-		while (!skb_queue_empty(&mcast->pkt_queue)) {
-			++dev->stats.tx_dropped;
-			dev_kfree_skb_any(skb_dequeue(&mcast->pkt_queue));
-		}
-		netif_tx_unlock_bh(dev);
-		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
-	} else {
-		mcast->backoff = 1;
-		mcast->delay_until = jiffies;
-		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
-	}
-out:
-	clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-	if (status)
-		mcast->mc = NULL;
-	complete(&mcast->done);
-	mutex_unlock(&mcast_mutex);
-	return status;
-}
-
-static int ipoib_mcast_sendonly_join(struct ipoib_mcast *mcast)
-{
-	struct net_device *dev = mcast->dev;
-	struct ipoib_dev_priv *priv = netdev_priv(dev);
-	struct ib_sa_mcmember_rec rec = {
-#if 0				/* Some SMs don't support send-only yet */
-		.join_state = 4
-#else
-		.join_state = 1
-#endif
-	};
-	int ret = 0;
-
-	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags)) {
-		ipoib_dbg_mcast(priv, "device shutting down, no sendonly "
-				"multicast joins\n");
-		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-		complete(&mcast->done);
-		return -ENODEV;
-	}
-
-	rec.mgid     = mcast->mcmember.mgid;
-	rec.port_gid = priv->local_gid;
-	rec.pkey     = cpu_to_be16(priv->pkey);
-
-	mutex_lock(&mcast_mutex);
-	mcast->mc = ib_sa_join_multicast(&ipoib_sa_client, priv->ca,
-					 priv->port, &rec,
-					 IB_SA_MCMEMBER_REC_MGID	|
-					 IB_SA_MCMEMBER_REC_PORT_GID	|
-					 IB_SA_MCMEMBER_REC_PKEY	|
-					 IB_SA_MCMEMBER_REC_JOIN_STATE,
-					 GFP_ATOMIC,
-					 ipoib_mcast_sendonly_join_complete,
-					 mcast);
-	if (IS_ERR(mcast->mc)) {
-		ret = PTR_ERR(mcast->mc);
-		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-		ipoib_warn(priv, "ib_sa_join_multicast for sendonly join "
-			   "failed (ret = %d)\n", ret);
-		complete(&mcast->done);
-	} else {
-		ipoib_dbg_mcast(priv, "no multicast record for %pI6, starting "
-				"sendonly join\n", mcast->mcmember.mgid.raw);
-	}
-	mutex_unlock(&mcast_mutex);
-
-	return ret;
-}
-
  void ipoib_mcast_carrier_on_task(struct work_struct *work)
  {
  	struct ipoib_dev_priv *priv = container_of(work, struct ipoib_dev_priv,

@@ -452,7 +347,9 @@ static int ipoib_mcast_join_complete(int status,
  	struct net_device *dev = mcast->dev;
  	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
-	ipoib_dbg_mcast(priv, "join completion for %pI6 (status %d)\n",
+	ipoib_dbg_mcast(priv, "%sjoin completion for %pI6 (status %d)\n",
+			test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ?
+			"sendonly " : "",
  			mcast->mcmember.mgid.raw, status);
 
  	/*

@@ -477,27 +374,52 @@ static int ipoib_mcast_join_complete(int status,
  	if (!status) {
  		mcast->backoff = 1;
  		mcast->delay_until = jiffies;
-		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
 
  		/*
  		 * Defer carrier on work to priv->wq to avoid a
-		 * deadlock on rtnl_lock here.
+		 * deadlock on rtnl_lock here.  Requeue our multicast
+		 * work too, which will end up happening right after
+		 * our carrier on task work and will allow us to
+		 * send out all of the non-broadcast joins
  		 */
-		if (mcast == priv->broadcast)
+		if (mcast == priv->broadcast) {
  			queue_work(priv->wq, &priv->carrier_on_task);
+			__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
+		}
  	} else {
  		if (mcast->logcount++ < 20) {
  			if (status == -ETIMEDOUT || status == -EAGAIN) {
-				ipoib_dbg_mcast(priv, "multicast join failed for %pI6, status %d\n",
+				ipoib_dbg_mcast(priv, "%smulticast join failed for %pI6, status %d\n",
+						test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ? "sendonly " : "",
  						mcast->mcmember.mgid.raw, status);
  			} else {
-				ipoib_warn(priv, "multicast join failed for %pI6, status %d\n",
+				ipoib_warn(priv, "%smulticast join failed for %pI6, status %d\n",
+						test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ? "sendonly " : "",
  					   mcast->mcmember.mgid.raw, status);
  			}
  		}
 
-		/* Requeue this join task with a backoff delay */
-		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
+		if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) &&

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +53 -52

+			mcast->backoff = 1;
+			netif_tx_lock_bh(dev);
+			while (!skb_queue_empty(&mcast->pkt_queue)) {
+				++dev->stats.tx_dropped;
+				dev_kfree_skb_any(skb_dequeue(&mcast->pkt_queue));
+			}
+			netif_tx_unlock_bh(dev);
+		} else
+			/* Requeue this join task with a backoff delay */
+			__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
  	}
  out:
  	clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);

@@ -650,45 +572,45 @@ void ipoib_mcast_join_task(struct work_struct *work)
  	list_for_each_entry(mcast, &priv->multicast_list, list) {
  		if (IS_ERR_OR_NULL(mcast->mc) &&
  		    !test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags) &&
-		    !test_bit(IPOIB_MCAST_FLAG_ATTACHED, &mcast->flags)) {
+		    (!test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ||
+		     !skb_queue_empty(&mcast->pkt_queue))) {
  			if (mcast->backoff == 1 ||
-			    time_after_eq(jiffies, mcast->delay_until))
+			    time_after_eq(jiffies, mcast->delay_until)) {
  				/* Found the next unjoined group */
-				break;
-			else if (!delay_until ||
+				init_completion(&mcast->done);
+				set_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
+				if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
+					create = 0;
+				else
+					create = 1;
+				spin_unlock_irq(&priv->lock);
+				mutex_unlock(&mcast_mutex);
+				ipoib_mcast_join(dev, mcast, create);
+				mutex_lock(&mcast_mutex);
+				spin_lock_irq(&priv->lock);
+			} else if (!delay_until ||
  				 time_before(mcast->delay_until, delay_until))
  				delay_until = mcast->delay_until;
  		}
  	}
 
-	if (&mcast->list == &priv->multicast_list) {
-		/*
-		 * All done, unless we have delayed work from
-		 * backoff retransmissions, but we will get
-		 * restarted when the time is right, so we are
-		 * done for now
-		 */
-		mcast = NULL;
-		ipoib_dbg_mcast(priv, "successfully joined all "
-				"multicast groups\n");
-	}
+	mcast = NULL;
+	ipoib_dbg_mcast(priv, "successfully started all multicast joins\n");
 
  out:
+	if (delay_until) {
+		cancel_delayed_work(&priv->mcast_task);
+		queue_delayed_work(priv->wq, &priv->mcast_task,
+				   delay_until - jiffies);
+	}
  	if (mcast) {
  		init_completion(&mcast->done);
  		set_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
  	}
  	spin_unlock_irq(&priv->lock);
  	mutex_unlock(&mcast_mutex);
-	if (mcast) {
-		if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
-			ipoib_mcast_sendonly_join(mcast);
-		else
-			ipoib_mcast_join(dev, mcast, create);
-	}
-	if (delay_until)
-		queue_delayed_work(priv->wq, &priv->mcast_task,
-				   delay_until - jiffies);
+	if (mcast)
+		ipoib_mcast_join(dev, mcast, create);
  }
 
  int ipoib_mcast_start_thread(struct net_device *dev)

@@ -731,8 +653,6 @@ static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
 
  	if (!IS_ERR_OR_NULL(mcast->mc))
  		ib_sa_free_multicast(mcast->mc);
-	else
-		ipoib_dbg(priv, "ipoib_mcast_leave with mcast->mc invalid\n");
 
  	if (test_and_clear_bit(IPOIB_MCAST_FLAG_ATTACHED, &mcast->flags)) {
  		ipoib_dbg_mcast(priv, "leaving MGID %pI6\n",

@@ -768,43 +688,37 @@ void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb)
  	}
 
  	mcast = __ipoib_mcast_find(dev, mgid);
-	if (!mcast) {
-		/* Let's create a new send only group now */
-		ipoib_dbg_mcast(priv, "setting up send only multicast group for %pI6\n",
-				mgid);
-
-		mcast = ipoib_mcast_alloc(dev, 0);
+	if (!mcast || !mcast->ah) {
  		if (!mcast) {
-			ipoib_warn(priv, "unable to allocate memory for "
-				   "multicast structure\n");
-			++dev->stats.tx_dropped;
-			dev_kfree_skb_any(skb);
-			goto out;
-		}
-
-		set_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags);
-		memcpy(mcast->mcmember.mgid.raw, mgid, sizeof (union ib_gid));
-		__ipoib_mcast_add(dev, mcast);
-		list_add_tail(&mcast->list, &priv->multicast_list);
-		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
-	}
+			/* Let's create a new send only group now */
+			ipoib_dbg_mcast(priv, "setting up send only multicast group for %pI6\n",
+					mgid);
+
+			mcast = ipoib_mcast_alloc(dev, 0);
+			if (!mcast) {
+				ipoib_warn(priv, "unable to allocate memory "
+					   "for multicast structure\n");
+				++dev->stats.tx_dropped;
+				dev_kfree_skb_any(skb);
+				goto unlock;
+			}
 
-	if (!mcast->ah) {
+			set_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags);
+			memcpy(mcast->mcmember.mgid.raw, mgid,
+			       sizeof (union ib_gid));
+			__ipoib_mcast_add(dev, mcast);
+			list_add_tail(&mcast->list, &priv->multicast_list);
+		}
  		if (skb_queue_len(&mcast->pkt_queue) < IPOIB_MAX_MCAST_QUEUE)
  			skb_queue_tail(&mcast->pkt_queue, skb);
  		else {
  			++dev->stats.tx_dropped;
  			dev_kfree_skb_any(skb);
  		}
-		/*
-		 * If lookup completes between here and out:, don't
-		 * want to send packet twice.
-		 */
-		mcast = NULL;
-	}

Quoted 200 lines · Doug Ledford

Allow the ipoib layer to attempt to join all outstanding multicast groups at once. The ib_sa layer will serialize multiple attempts to join the same group, but will process attempts to join different groups in parallel. Take advantage of that.

In order to make this happen, change the mcast_join_thread to loop through all needed joins, sending a join request for each one that we still need to join. There are a few special cases we handle though:

1) Don't attempt to join anything but the broadcast group until the join of the broadcast group has succeeded.
2) No longer restart the join task at the end of completion handling. If we completed successfully, we are done. The join task now needs kicked either by mcast_send or mcast_restart_task or mcast_start_thread, but should not need started anytime else except when scheduling a backoff attempt to rejoin.
3) No longer use separate join/completion routines for regular and sendonly joins, pass them all through the same routine and just do the right thing based on the SENDONLY join flag.
4) Only try to join a SENDONLY join twice, then drop the packets and quit trying. We leave the mcast group in the list so that if we get a new packet, all that we have to do is queue up the packet and restart the join task and it will automatically try to join twice and then either send or flush the queue again.

Signed-off-by Doug Ledford <[email protected]>

---
  drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 250 ++++++++-----------------
  1 file changed, 82 insertions(+), 168 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +21 -113

index 277e7ac7c4d..c670d9c2cda 100644
@@ -307,111 +307,6 @@ static int ipoib_mcast_join_finish(struct ipoib_mcast *mcast,
  	return 0;
  }
 
-static int
-ipoib_mcast_sendonly_join_complete(int status,
-				   struct ib_sa_multicast *multicast)
-{
-	struct ipoib_mcast *mcast = multicast->context;
-	struct net_device *dev = mcast->dev;
-	struct ipoib_dev_priv *priv = netdev_priv(dev);
-
-	/*
-	 * We have to take the mutex to force mcast_sendonly_join to
-	 * return from ib_sa_multicast_join and set mcast->mc to a
-	 * valid value.  Otherwise we were racing with ourselves in
-	 * that we might fail here, but get a valid return from
-	 * ib_sa_multicast_join after we had cleared mcast->mc here,
-	 * resulting in mis-matched joins and leaves and a deadlock
-	 */
-	mutex_lock(&mcast_mutex);
-
-	/* We trap for port events ourselves. */
-	if (status == -ENETRESET) {
-		status = 0;
-		goto out;
-	}
-
-	if (!status)
-		status = ipoib_mcast_join_finish(mcast, &multicast->rec);
-
-	if (status) {
-		if (mcast->logcount++ < 20)
-			ipoib_dbg_mcast(netdev_priv(dev), "sendonly multicast "
-					"join failed for %pI6, status %d\n",
-					mcast->mcmember.mgid.raw, status);
-
-		/* Flush out any queued packets */
-		netif_tx_lock_bh(dev);
-		while (!skb_queue_empty(&mcast->pkt_queue)) {
-			++dev->stats.tx_dropped;
-			dev_kfree_skb_any(skb_dequeue(&mcast->pkt_queue));
-		}
-		netif_tx_unlock_bh(dev);
-		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
-	} else {
-		mcast->backoff = 1;
-		mcast->delay_until = jiffies;
-		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
-	}
-out:
-	clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-	if (status)
-		mcast->mc = NULL;
-	complete(&mcast->done);
-	mutex_unlock(&mcast_mutex);
-	return status;
-}
-
-static int ipoib_mcast_sendonly_join(struct ipoib_mcast *mcast)
-{
-	struct net_device *dev = mcast->dev;
-	struct ipoib_dev_priv *priv = netdev_priv(dev);
-	struct ib_sa_mcmember_rec rec = {
-#if 0				/* Some SMs don't support send-only yet */
-		.join_state = 4
-#else
-		.join_state = 1
-#endif
-	};
-	int ret = 0;
-
-	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags)) {
-		ipoib_dbg_mcast(priv, "device shutting down, no sendonly "
-				"multicast joins\n");
-		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-		complete(&mcast->done);
-		return -ENODEV;
-	}
-
-	rec.mgid     = mcast->mcmember.mgid;
-	rec.port_gid = priv->local_gid;
-	rec.pkey     = cpu_to_be16(priv->pkey);
-
-	mutex_lock(&mcast_mutex);
-	mcast->mc = ib_sa_join_multicast(&ipoib_sa_client, priv->ca,
-					 priv->port, &rec,
-					 IB_SA_MCMEMBER_REC_MGID	|
-					 IB_SA_MCMEMBER_REC_PORT_GID	|
-					 IB_SA_MCMEMBER_REC_PKEY	|
-					 IB_SA_MCMEMBER_REC_JOIN_STATE,
-					 GFP_ATOMIC,
-					 ipoib_mcast_sendonly_join_complete,
-					 mcast);
-	if (IS_ERR(mcast->mc)) {
-		ret = PTR_ERR(mcast->mc);
-		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-		ipoib_warn(priv, "ib_sa_join_multicast for sendonly join "
-			   "failed (ret = %d)\n", ret);
-		complete(&mcast->done);
-	} else {
-		ipoib_dbg_mcast(priv, "no multicast record for %pI6, starting "
-				"sendonly join\n", mcast->mcmember.mgid.raw);
-	}
-	mutex_unlock(&mcast_mutex);
-
-	return ret;
-}
-
  void ipoib_mcast_carrier_on_task(struct work_struct *work)
  {
  	struct ipoib_dev_priv *priv = container_of(work, struct ipoib_dev_priv,

@@ -452,7 +347,9 @@ static int ipoib_mcast_join_complete(int status,
  	struct net_device *dev = mcast->dev;
  	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
-	ipoib_dbg_mcast(priv, "join completion for %pI6 (status %d)\n",
+	ipoib_dbg_mcast(priv, "%sjoin completion for %pI6 (status %d)\n",
+			test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ?
+			"sendonly " : "",
  			mcast->mcmember.mgid.raw, status);
 
  	/*

@@ -477,27 +374,52 @@ static int ipoib_mcast_join_complete(int status,
  	if (!status) {
  		mcast->backoff = 1;
  		mcast->delay_until = jiffies;
-		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
 
  		/*
  		 * Defer carrier on work to priv->wq to avoid a
-		 * deadlock on rtnl_lock here.
+		 * deadlock on rtnl_lock here.  Requeue our multicast
+		 * work too, which will end up happening right after
+		 * our carrier on task work and will allow us to
+		 * send out all of the non-broadcast joins
  		 */
-		if (mcast == priv->broadcast)
+		if (mcast == priv->broadcast) {
  			queue_work(priv->wq, &priv->carrier_on_task);
+			__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
+		}
  	} else {
  		if (mcast->logcount++ < 20) {
  			if (status == -ETIMEDOUT || status == -EAGAIN) {
-				ipoib_dbg_mcast(priv, "multicast join failed for %pI6, status %d\n",
+				ipoib_dbg_mcast(priv, "%smulticast join failed for %pI6, status %d\n",
+						test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ? "sendonly " : "",
  						mcast->mcmember.mgid.raw, status);
  			} else {
-				ipoib_warn(priv, "multicast join failed for %pI6, status %d\n",
+				ipoib_warn(priv, "%smulticast join failed for %pI6, status %d\n",
+						test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ? "sendonly " : "",
  					   mcast->mcmember.mgid.raw, status);
  			}
  		}
 
-		/* Requeue this join task with a backoff delay */
-		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
+		if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) &&
+		    mcast->backoff >= 2) {
+			/*
+			 * We only retry sendonly joins once before we drop
+			 * the packet and quit trying to deal with the
+			 * group.  However, we leave the group in the
+			 * mcast list as an unjoined group.  If we want to

drivers/infiniband/ulp/ipoib/ipoib_multicast.c static int ipoib_mcast_join_complete(int status, · line 412

Quoted 4 lines

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +4 -0

+			 * try joining again, we simply queue up a packet
+			 * and restart the join thread.  The empty queue
+			 * is why the join thread ignores this group.
+			 */

Question: the sendonly is at the list for ever? looks like that, and it is prior to your patches, so probably it should be sent in some other patch to solve that.

Quoted 192 lines · Erez Shitrit

Quoted 191 lines · Doug Ledford

Allow the ipoib layer to attempt to join all outstanding multicast groups at once. The ib_sa layer will serialize multiple attempts to join the same group, but will process attempts to join different groups in parallel. Take advantage of that.

In order to make this happen, change the mcast_join_thread to loop through all needed joins, sending a join request for each one that we still need to join. There are a few special cases we handle though:

1) Don't attempt to join anything but the broadcast group until the join of the broadcast group has succeeded.
2) No longer restart the join task at the end of completion handling. If we completed successfully, we are done. The join task now needs kicked either by mcast_send or mcast_restart_task or mcast_start_thread, but should not need started anytime else except when scheduling a backoff attempt to rejoin.
3) No longer use separate join/completion routines for regular and sendonly joins, pass them all through the same routine and just do the right thing based on the SENDONLY join flag.
4) Only try to join a SENDONLY join twice, then drop the packets and quit trying. We leave the mcast group in the list so that if we get a new packet, all that we have to do is queue up the packet and restart the join task and it will automatically try to join twice and then either send or flush the queue again.

Signed-off-by Doug Ledford <[email protected]>

---
   drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 250 ++++++++-----------------
   1 file changed, 82 insertions(+), 168 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +14 -111

index 277e7ac7c4d..c670d9c2cda 100644
@@ -307,111 +307,6 @@ static int ipoib_mcast_join_finish(struct ipoib_mcast *mcast,
   	return 0;
   }
 
-static int
-ipoib_mcast_sendonly_join_complete(int status,
-				   struct ib_sa_multicast *multicast)
-{
-	struct ipoib_mcast *mcast = multicast->context;
-	struct net_device *dev = mcast->dev;
-	struct ipoib_dev_priv *priv = netdev_priv(dev);
-
-	/*
-	 * We have to take the mutex to force mcast_sendonly_join to
-	 * return from ib_sa_multicast_join and set mcast->mc to a
-	 * valid value.  Otherwise we were racing with ourselves in
-	 * that we might fail here, but get a valid return from
-	 * ib_sa_multicast_join after we had cleared mcast->mc here,
-	 * resulting in mis-matched joins and leaves and a deadlock
-	 */
-	mutex_lock(&mcast_mutex);
-
-	/* We trap for port events ourselves. */
-	if (status == -ENETRESET) {
-		status = 0;
-		goto out;
-	}
-
-	if (!status)
-		status = ipoib_mcast_join_finish(mcast, &multicast->rec);
-
-	if (status) {
-		if (mcast->logcount++ < 20)
-			ipoib_dbg_mcast(netdev_priv(dev), "sendonly multicast "
-					"join failed for %pI6, status %d\n",
-					mcast->mcmember.mgid.raw, status);
-
-		/* Flush out any queued packets */
-		netif_tx_lock_bh(dev);
-		while (!skb_queue_empty(&mcast->pkt_queue)) {
-			++dev->stats.tx_dropped;
-			dev_kfree_skb_any(skb_dequeue(&mcast->pkt_queue));
-		}
-		netif_tx_unlock_bh(dev);
-		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
-	} else {
-		mcast->backoff = 1;
-		mcast->delay_until = jiffies;
-		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
-	}
-out:
-	clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-	if (status)
-		mcast->mc = NULL;
-	complete(&mcast->done);
-	mutex_unlock(&mcast_mutex);
-	return status;
-}
-
-static int ipoib_mcast_sendonly_join(struct ipoib_mcast *mcast)
-{
-	struct net_device *dev = mcast->dev;
-	struct ipoib_dev_priv *priv = netdev_priv(dev);
-	struct ib_sa_mcmember_rec rec = {
-#if 0				/* Some SMs don't support send-only yet */
-		.join_state = 4
-#else
-		.join_state = 1
-#endif
-	};
-	int ret = 0;
-
-	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags)) {
-		ipoib_dbg_mcast(priv, "device shutting down, no sendonly "
-				"multicast joins\n");
-		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-		complete(&mcast->done);
-		return -ENODEV;
-	}
-
-	rec.mgid     = mcast->mcmember.mgid;
-	rec.port_gid = priv->local_gid;
-	rec.pkey     = cpu_to_be16(priv->pkey);
-
-	mutex_lock(&mcast_mutex);
-	mcast->mc = ib_sa_join_multicast(&ipoib_sa_client, priv->ca,
-					 priv->port, &rec,
-					 IB_SA_MCMEMBER_REC_MGID	|
-					 IB_SA_MCMEMBER_REC_PORT_GID	|
-					 IB_SA_MCMEMBER_REC_PKEY	|
-					 IB_SA_MCMEMBER_REC_JOIN_STATE,
-					 GFP_ATOMIC,
-					 ipoib_mcast_sendonly_join_complete,
-					 mcast);
-	if (IS_ERR(mcast->mc)) {
-		ret = PTR_ERR(mcast->mc);
-		clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
-		ipoib_warn(priv, "ib_sa_join_multicast for sendonly join "
-			   "failed (ret = %d)\n", ret);
-		complete(&mcast->done);
-	} else {
-		ipoib_dbg_mcast(priv, "no multicast record for %pI6, starting "
-				"sendonly join\n", mcast->mcmember.mgid.raw);
-	}
-	mutex_unlock(&mcast_mutex);
-
-	return ret;
-}
-
   void ipoib_mcast_carrier_on_task(struct work_struct *work)
   {
   	struct ipoib_dev_priv *priv = container_of(work, struct ipoib_dev_priv,

@@ -452,7 +347,9 @@ static int ipoib_mcast_join_complete(int status,
   	struct net_device *dev = mcast->dev;
   	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
-	ipoib_dbg_mcast(priv, "join completion for %pI6 (status %d)\n",
+	ipoib_dbg_mcast(priv, "%sjoin completion for %pI6 (status %d)\n",
+			test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ?
+			"sendonly " : "",
   			mcast->mcmember.mgid.raw, status);
 
   	/*

@@ -477,27 +374,52 @@ static int ipoib_mcast_join_complete(int status,
   	if (!status) {
   		mcast->backoff = 1;
   		mcast->delay_until = jiffies;
-		__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
 
   		/*
   		 * Defer carrier on work to priv->wq to avoid a
-		 * deadlock on rtnl_lock here.
+		 * deadlock on rtnl_lock here.  Requeue our multicast
+		 * work too, which will end up happening right after
+		 * our carrier on task work and will allow us to
+		 * send out all of the non-broadcast joins
   		 */
-		if (mcast == priv->broadcast)
+		if (mcast == priv->broadcast) {
   			queue_work(priv->wq, &priv->carrier_on_task);
+			__ipoib_mcast_schedule_join_thread(priv, NULL, 0);
+		}
   	} else {
   		if (mcast->logcount++ < 20) {
   			if (status == -ETIMEDOUT || status == -EAGAIN) {
-				ipoib_dbg_mcast(priv, "multicast join failed for %pI6, status %d\n",
+				ipoib_dbg_mcast(priv, "%smulticast join failed for %pI6, status %d\n",
+						test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ? "sendonly " : "",
   						mcast->mcmember.mgid.raw, status);
   			} else {
-				ipoib_warn(priv, "multicast join failed for %pI6, status %d\n",
+				ipoib_warn(priv, "%smulticast join failed for %pI6, status %d\n",
+						test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) ? "sendonly " : "",
   					   mcast->mcmember.mgid.raw, status);
   			}
   		}
 

Quoted 16 lines

drivers/infiniband/ulp/ipoib/ipoib_multicast.c static int ipoib_mcast_join_complete(int status, · line 412

Quoted 13 lines

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +11 -2

-		/* Requeue this join task with a backoff delay */
-		__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
+		if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags) &&
+		    mcast->backoff >= 2) {
+			/*
+			 * We only retry sendonly joins once before we drop
+			 * the packet and quit trying to deal with the
+			 * group.  However, we leave the group in the
+			 * mcast list as an unjoined group.  If we want to
+			 * try joining again, we simply queue up a packet
+			 * and restart the join thread.  The empty queue
+			 * is why the join thread ignores this group.
+			 */

Question: the sendonly is at the list for ever? looks like that, and it is prior to your patches, so probably it should be sent in some other patch to solve that.