Fix ipoib regressions

My 8 patch set taken into 3.19 caused some regressions. This patch set resolves those issues.

These patches are to resolve issues created by my previous patch set. While that set worked fine in my testing, there were problems with multicast joins after the initial set of joins had completed. Since my testing relied upon the normal set of multicast joins that happen when the interface is first brought up, I missed those problems.

Symptoms vary from failure to send packets due to a failed join, to loss of connectivity after a subnet manager restart, to failure to properly release multicast groups on shutdown resulting in hangs when the mlx4 driver attempts to unload itself via its reboot notifier handler.

This set of patches has passed a number of tests above and beyond my original tests. As suggested by Or Gerlitz I added IPv6 and IPv4 multicast tests. I also added both subnet manager restarts and manual shutdown/restart of individual ports at the switch in order to ensure that the ENETRESET path was properly tested. I included testing, then a subnet manager restart, then a quiescent period for caches to expire, then restarting testing to make sure that arp and neighbor discovery work after the subnet manager restart.

All in all, I have not been able to trip the multicast joins up any longer.

Additionally, the original impetus for my first 8 patch set was that it was simply too easy to break the IPoIB subsystem with this simple loop:

while true; do
ifconfig ib0 up
ifconfig ib0 down
done

Just to be safe, I made sure this problem did not resurface.

v5: fix an oversight in mcast_restart_task that leaked mcast joins
fix a failure to flush the ipoib_workqueue on deregister that meant we could end up running our code after our device had been removed, resulting in an oops
remove a debug message that could be trigger so fast that the kernel printk mechanism would starve out the mcast join task thread resulting in what looked like a mcast failure that was really just delayed action

Doug Ledford (10):
IB/ipoib: fix IPOIB_MCAST_RUN flag usage IB/ipoib: Add a helper to restart the multicast task IB/ipoib: make delayed tasks not hold up everything IB/ipoib: Handle -ENETRESET properly in our callback IB/ipoib: don't restart our thread on ENETRESET IB/ipoib: remove unneeded locks
IB/ipoib: fix race between mcast_dev_flush and mcast_join IB/ipoib: fix ipoib_mcast_restart_task
IB/ipoib: flush the ipoib_workqueue on unregister IB/ipoib: cleanup a couple debug messages

 drivers/infiniband/ulp/ipoib/ipoib.h           |   1 +
 drivers/infiniband/ulp/ipoib/ipoib_main.c      |   2 +
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 234 ++++++++++++++-----------
 3 files changed, 131 insertions(+), 106 deletions(-)

2.1.0

--
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

The usage of IPOIB_MCAST_RUN as a flag is inconsistent. In some places it is used to mean "our device is administratively allowed to send multicast joins/leaves/packets" and in other places it means "our multicast join task thread is currently running and will process your request if you put it on the queue". However, this latter meaning is in fact flawed as there is a race condition between the join task testing the mcast list and finding it empty of remaining work, dropping the mcast mutex and also the priv->lock spinlock, and clearing the IPOIB_MCAST_RUN flag. Further, there are numerous locations that use the flag in the former fashion, and when all tasks complete and the task thread clears the RUN flag, all of those other locations will fail to ever again queue any work. This results in the interface coming up fine initially, but having problems adding new multicast groups after the first round of groups have all been added and the RUN flag is cleared by the join task thread when it thinks it is done. To resolve this issue, convert all locations in the code to treat the RUN flag as an indicator that the multicast portion of this interface is in fact administratively up and joins/leaves/sends can be performed. There is no harm (other than a slight performance penalty) to never clearing this flag and using it in this fashion as it simply means that a few places that used to micro-optimize how often this task was queued on a work queue will now queue the task a few extra times. We can address that suboptimal behavior in future patches.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

index bc50dd0d0e4..91b8fe118ec 100644
@@ -630,8 +630,6 @@ void ipoib_mcast_join_task(struct work_struct *work)
 	}
 
 	ipoib_dbg_mcast(priv, "successfully joined all multicast groups\n");
-
-	clear_bit(IPOIB_MCAST_RUN, &priv->flags);
 }
 
 int ipoib_mcast_start_thread(struct net_device *dev)

@@ -641,8 +639,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);
+	queue_delayed_work(priv->wq, &priv->mcast_task, 0);
 	mutex_unlock(&mcast_mutex);
 
 	return 0;

@@ -725,7 +723,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);
-		if (!test_and_set_bit(IPOIB_MCAST_RUN, &priv->flags))
+		if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
 			queue_delayed_work(priv->wq, &priv->mcast_task, 0);
 	}
 

@@ -951,7 +949,8 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 	/*
 	 * Restart our join task if needed
 	 */
-	ipoib_mcast_start_thread(dev);
+	if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
+		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
 	rtnl_unlock();
 }
 

2.1.0

--
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

Add a simple helper to do the right thing when restarting the multicast thread. Call that helper from all the places that need to restart the thread. Add two places that didn't restart the thread, but should have.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 46 ++++++++++++++++----------
 1 file changed, 29 insertions(+), 17 deletions(-)

index 91b8fe118ec..cb1e495bd74 100644
@@ -66,6 +66,13 @@ struct ipoib_mcast_iter {
 	unsigned int       send_only;
 };
 
+static void __ipoib_mcast_continue_join_thread(struct ipoib_dev_priv *priv,
+					       int delay)
+{
+	if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
+		queue_delayed_work(priv->wq, &priv->mcast_task, delay);
+}
+
 static void ipoib_mcast_free(struct ipoib_mcast *mcast)
 {
 	struct net_device *dev = mcast->dev;

@@ -270,6 +277,7 @@ 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

@@ -309,6 +317,7 @@ out:
 	complete(&mcast->done);
 	if (status == -ENETRESET)
 		status = 0;
+	__ipoib_mcast_continue_join_thread(priv, 0);
 	mutex_unlock(&mcast_mutex);
 	return status;
 }

@@ -360,6 +369,7 @@ static int ipoib_mcast_sendonly_join(struct ipoib_mcast *mcast)
 		complete(&mcast->done);
 		ipoib_warn(priv, "ib_sa_join_multicast for sendonly join "
 			   "failed (ret = %d)\n", ret);
+		__ipoib_mcast_continue_join_thread(priv, 0);
 	} else {
 		ipoib_dbg_mcast(priv, "no multicast record for %pI6, starting "
 				"sendonly join\n", mcast->mcmember.mgid.raw);

@@ -431,8 +441,7 @@ static int ipoib_mcast_join_complete(int status,
 
 	if (!status) {
 		mcast->backoff = 1;
-		if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
-			queue_delayed_work(priv->wq, &priv->mcast_task, 0);
+		__ipoib_mcast_continue_join_thread(priv, 0);
 
 		/*
 		 * Defer carrier on work to priv->wq to avoid a

@@ -454,6 +463,13 @@ static int ipoib_mcast_join_complete(int status,
 		mcast->backoff *= 2;
 		if (mcast->backoff > IPOIB_MAX_BACKOFF_SECONDS)
 			mcast->backoff = IPOIB_MAX_BACKOFF_SECONDS;
+		/*
+		 * XXX - This is wrong.  *Our* join failed, but because the
+		 * join thread does the joins in a serial fashion, if there
+		 * are any joins behind ours waiting to complete, they should
+		 * not be subjected to our backoff delay.
+		 */
+		__ipoib_mcast_continue_join_thread(priv, mcast->backoff * HZ);
 	}
 out:
 	spin_lock_irq(&priv->lock);

@@ -463,9 +479,6 @@ out:
 	complete(&mcast->done);
 	if (status == -ENETRESET)
 		status = 0;
-	if (status && 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);
 

@@ -532,10 +545,13 @@ static void ipoib_mcast_join(struct net_device *dev, struct ipoib_mcast *mcast,
 		mcast->backoff *= 2;
 		if (mcast->backoff > IPOIB_MAX_BACKOFF_SECONDS)
 			mcast->backoff = IPOIB_MAX_BACKOFF_SECONDS;
-
-		if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
-			queue_delayed_work(priv->wq, &priv->mcast_task,
-					   mcast->backoff * HZ);
+		/*
+		 * XXX - This is wrong.  *Our* join failed, but because the
+		 * join thread does the joins in a serial fashion, if there
+		 * are any joins behind ours waiting to complete, they should
+		 * not be subjected to our backoff delay.
+		 */
+		__ipoib_mcast_continue_join_thread(priv, mcast->backoff * HZ);
 	}
 	mutex_unlock(&mcast_mutex);
 }

@@ -573,9 +589,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
 		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);
+			__ipoib_mcast_continue_join_thread(priv, HZ);
 			mutex_unlock(&mcast_mutex);
 			return;
 		}

@@ -723,8 +737,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);
-		if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
-			queue_delayed_work(priv->wq, &priv->mcast_task, 0);
+		__ipoib_mcast_continue_join_thread(priv, 0);
 	}
 
 	if (!mcast->ah) {

@@ -947,10 +960,9 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 		ipoib_mcast_free(mcast);
 	}
 	/*
-	 * Restart our join task if needed
+	 * Restart our join task thread if needed
 	 */
-	if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
-		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
+	__ipoib_mcast_continue_join_thread(priv, 0);
 	rtnl_unlock();
 }
 

2.1.0

--
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

Currently, the multicast join thread only processes one task at a time. It does this by having itself be scheduled as a delayed work item, when it runs, it finds one, and only one, piece of work to process, it then kicks that off via either the normal join process or the sendonly join process, and then it immediately exits. Both of those process chains are responsible for restarting the task when they have completed their specific action. This makes the entire join process serial with only one join supposedly ever outstanding at a time.

However, if we fail a join, and we need to initiate a backoff delay, that delay holds up the entire join process for all joins, not just the failed join. So modify the design such that we can have joins in delay, and the multicast thread will ignore them until their time comes around, and then we can process the rest of the queue without waiting for the delayed items.

---
 drivers/infiniband/ulp/ipoib/ipoib.h           |  1 +
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 86 +++++++++++++++++---------
 2 files changed, 57 insertions(+), 30 deletions(-)

index 8ba80a6d3a4..c79dcd5ee8a 100644
@@ -154,6 +154,7 @@ struct ipoib_mcast {
 
 	unsigned long created;
 	unsigned long backoff;
+	unsigned long delay_until;
 
 	unsigned long flags;
 	unsigned char logcount;

2.1.0

--
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

When the core layer calls our callback with ENETRESET as the error, we clear the status to 0 before returning indicating that we are going to handle the error ourselves. This causes the core layer to not free the mcast->mc structure, but we are releasing our reference to it by clearing mcast->mc. So, preserve our reference to the multicast structure so when we next run ipoib_mcast_dev_flush, it will be able to properly release the mcast->mc entry at the right time in the right way.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index 957e7d2e80c..50d2de2270a 100644
@@ -315,8 +315,10 @@ ipoib_mcast_sendonly_join_complete(int status,
 	mutex_lock(&mcast_mutex);
 
 	/* We trap for port events ourselves. */
-	if (status == -ENETRESET)
+	if (status == -ENETRESET) {
+		status = 0;
 		goto out;
+	}
 
 	if (!status)
 		status = ipoib_mcast_join_finish(mcast, &multicast->rec);

@@ -344,8 +346,6 @@ out:
 	if (status)
 		mcast->mc = NULL;
 	complete(&mcast->done);
-	if (status == -ENETRESET)
-		status = 0;
 	__ipoib_mcast_continue_join_thread(priv, NULL, 0);
 	mutex_unlock(&mcast_mutex);
 	return status;

@@ -462,8 +462,10 @@ static int ipoib_mcast_join_complete(int status,
 	mutex_lock(&mcast_mutex);
 
 	/* We trap for port events ourselves. */
-	if (status == -ENETRESET)
+	if (status == -ENETRESET) {
+		status = 0;
 		goto out;
+	}
 
 	if (!status)
 		status = ipoib_mcast_join_finish(mcast, &multicast->rec);

@@ -499,8 +501,6 @@ out:
 	if (status)
 		mcast->mc = NULL;
 	complete(&mcast->done);
-	if (status == -ENETRESET)
-		status = 0;
 	spin_unlock_irq(&priv->lock);
 	mutex_unlock(&mcast_mutex);
 

2.1.0

--
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

Make the send only join completion handler behave like the normal join completion handler in that we should restart the multicast join thread whenever our join completes with either an error or success, but don't restart the thread if we got a net reset event, let ipoib_event queue up a device flush, which will then call ipoib_mcast_dev_flush to remove all of the current mcast entries, and ipoib_mcase_restart_task to check out current mcast entry list against the netdev list of mcast entries we are supposed to have and requeue the ones we need to get back.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 50d2de2270a..a03b6a04203 100644
@@ -336,17 +336,18 @@ ipoib_mcast_sendonly_join_complete(int status,
 			dev_kfree_skb_any(skb_dequeue(&mcast->pkt_queue));
 		}
 		netif_tx_unlock_bh(dev);
+		__ipoib_mcast_continue_join_thread(priv, mcast, 1);
 	} else {
 		/* Join completed, so reset any backoff parameters */
 		mcast->backoff = 1;
 		mcast->delay_until = jiffies;
+		__ipoib_mcast_continue_join_thread(priv, NULL, 0);
 	}
 out:
 	clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
 	if (status)
 		mcast->mc = NULL;
 	complete(&mcast->done);
-	__ipoib_mcast_continue_join_thread(priv, NULL, 0);
 	mutex_unlock(&mcast_mutex);
 	return status;
 }

2.1.0

--
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

During the various work I've done on this, some extra locking has crept in when things weren't working right. This is one of those spots. Remove the unneeded spinlocks. The mutex is enough to protect against what we need to protect against.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 2 --
 1 file changed, 2 deletions(-)

index a03b6a04203..424c602d5a0 100644
@@ -497,12 +497,10 @@ static int ipoib_mcast_join_complete(int status,
 		__ipoib_mcast_continue_join_thread(priv, mcast, 1);
 	}
 out:
-	spin_lock_irq(&priv->lock);
 	clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
 	if (status)
 		mcast->mc = NULL;
 	complete(&mcast->done);
-	spin_unlock_irq(&priv->lock);
 	mutex_unlock(&mcast_mutex);
 
 	return status;

2.1.0

--
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

There is a classic race between mcast_dev_flush and mcast_join_thread in that the join thread is going to loop through all of the mcast entries in the device mcast list, while dev_flush wants to remove those entries from the list so that they can eventually be freed when they are no longer busy.

As always, the list traversal and the list removal must be done under the same lock to avoid races, so modify the join_task thread code to hold onto the spinlock for the entirety of its run up until it has saved the mcast entry that it's going to work on into a private pointer and set the flags on that entry to mark it as busy. Only then is it safe to drop the lock and let mcast_dev_flush remove entries from the list.

I also reworked the flow of the join_task thread to be more clear that it is making a single pass at work to do and then exiting as soon as it gets that single task queued off.

I then reworked both the regular and send only join routines to know that they will be called with the BUSY flag already set on the mcast entry they are passed, so any error condition means they need to clear the flag and complete the task and they should not check the busy status as it will now always be true.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 112 ++++++++++++-------------
 1 file changed, 56 insertions(+), 56 deletions(-)

index 424c602d5a0..972fa15a225 100644
@@ -368,22 +368,16 @@ static int ipoib_mcast_sendonly_join(struct ipoib_mcast *mcast)
 	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;
 	}
 
-	if (test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags)) {
-		ipoib_dbg_mcast(priv, "multicast entry busy, skipping "
-				"sendonly join\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);
-	init_completion(&mcast->done);
-	set_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
 	mcast->mc = ib_sa_join_multicast(&ipoib_sa_client, priv->ca,
 					 priv->port, &rec,
 					 IB_SA_MCMEMBER_REC_MGID	|

@@ -552,8 +546,6 @@ static void ipoib_mcast_join(struct net_device *dev, struct ipoib_mcast *mcast,
 	}
 
 	mutex_lock(&mcast_mutex);
-	init_completion(&mcast->done);
-	set_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
 	mcast->mc = ib_sa_join_multicast(&ipoib_sa_client, priv->ca, priv->port,
 					 &rec, comp_mask, GFP_KERNEL,
 					 ipoib_mcast_join_complete, mcast);

@@ -574,6 +566,8 @@ 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;
+	struct ipoib_mcast *mcast = NULL;
+	int create = 1;
 
 	if (!test_bit(IPOIB_MCAST_RUN, &priv->flags))
 		return;

@@ -591,16 +585,25 @@ 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_ADMIN_UP, &priv->flags))
+		goto out;
+
 	if (!priv->broadcast) {
 		struct ipoib_mcast *broadcast;
 
-		if (!test_bit(IPOIB_FLAG_ADMIN_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);
 			/*
 			 * Restart us after a 1 second delay to retry
 			 * creating our broadcast group and attaching to

@@ -608,67 +611,64 @@ void ipoib_mcast_join_task(struct work_struct *work)
 			 * completely stalled (multicast wise).
 			 */
 			__ipoib_mcast_continue_join_thread(priv, NULL, 1);
-			mutex_unlock(&mcast_mutex);
-			return;
+			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 (IS_ERR_OR_NULL(priv->broadcast->mc) &&
-		    !test_bit(IPOIB_MCAST_FLAG_BUSY, &priv->broadcast->flags))
-			ipoib_mcast_join(dev, priv->broadcast, 0);
-		return;
-	}
-
-	while (1) {
-		struct ipoib_mcast *mcast = NULL;
-
-		/*
-		 * Need the mutex so our flags are consistent, need the
-		 * priv->lock so we don't race with list removals in either
-		 * mcast_dev_flush or mcast_restart_task
-		 */
-		mutex_lock(&mcast_mutex);
-		spin_lock_irq(&priv->lock);
-		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) &&
-			    (mcast->backoff == 1 ||
-			     time_after_eq(jiffies, mcast->delay_until))) {
-				/* Found the next unjoined group */
-				break;
-			}
+		    !test_bit(IPOIB_MCAST_FLAG_BUSY, &priv->broadcast->flags)) {
+			mcast = priv->broadcast;
+			create = 0;
 		}
-		spin_unlock_irq(&priv->lock);
-		mutex_unlock(&mcast_mutex);
+		goto out;
+	}
 
-		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
-			 */
+	/* 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) &&
+		    (mcast->backoff == 1 ||
+		     time_after_eq(jiffies, mcast->delay_until))) {
+			/* Found the next unjoined group */
 			break;
 		}
+	}
 
+	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");
+	}
+
+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, 1);
-		return;
+			ipoib_mcast_join(dev, mcast, create);
 	}
-
-	ipoib_dbg_mcast(priv, "successfully joined all multicast groups\n");
+	return;
 }
 
 int ipoib_mcast_start_thread(struct net_device *dev)

2.1.0

--
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

There are two intertwined problems here.

First, if we are downing the interface, we will get called as part of the process. When that happens, we don't really want to do anything here as the next thing to happen is ipoib_mcast_dev_flush will get called. We just want to let it do all the work since we don't need to do any address matches and such. So, we should be testing for whether or not the interface is up at the beginning of this function and returning if it isn't.

Second, the attempt to grab the rtnl_lock to prevent racing against ipoib_mcast_dev_flush was mistaken. The removed mcast groups are on a stack local list head and so once we've removed them from the mcast rbtree, ipoib_mcast_dev_flush can no longer find them and race us on removing them. However, because we attempt to take the lock, and then check the status of the interface, if our interface has gone down, we simply return. This is absolutely the wrong thing to do as it means we just leaked all of the mcast entries on our remove list.

The fix is to check for the interface being up at the very beginning of the function, and then to always remove our entries on the remove list no matter what as they will get leaked otherwise, and skip taking the rtnl_lock since it isn't needed anyway.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

index 972fa15a225..10e05437c83 100644
@@ -871,6 +871,9 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 	unsigned long flags;
 	struct ib_sa_mcmember_rec rec;
 
+	if (!test_bit(IPOIB_FLAG_ADMIN_UP, &priv->flags))
+		return;
+
 	ipoib_dbg_mcast(priv, "restarting multicast task\n");
 
 	local_irq_save(flags);

@@ -965,21 +968,6 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 		if (test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags))
 			wait_for_completion(&mcast->done);
 
-	/*
-	 * We have to cancel outside of the spinlock, but we have to
-	 * take the rtnl lock or else we race with the removal of
-	 * entries from the remove list in mcast_dev_flush as part
-	 * of ipoib_stop().  We detect the drop of the ADMIN_UP flag
-	 * to signal that we have hit this particular race, and we
-	 * return since we know we don't need to do anything else
-	 * anyway.
-	 */
-	while (!rtnl_trylock()) {
-		if (!test_bit(IPOIB_FLAG_ADMIN_UP, &priv->flags))
-			return;
-		else
-			msleep(20);
-	}
 	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
 		ipoib_mcast_leave(mcast->dev, mcast);
 		ipoib_mcast_free(mcast);

@@ -988,7 +976,6 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 	 * Restart our join task thread if needed
 	 */
 	__ipoib_mcast_continue_join_thread(priv, NULL, 0);
-	rtnl_unlock();
 }
 
 #ifdef CONFIG_INFINIBAND_IPOIB_DEBUG

2.1.0

--
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

When we unregister our event handler as a prelude to removing our device, we need to go ahead and flush the ipoib_workqueue to make sure there are no delayed flushes for the device we are getting ready to remove.

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++
 1 file changed, 2 insertions(+)

index 6bad17d4d58..8810514cf40 100644
@@ -1643,6 +1643,7 @@ sysfs_failed:
 
 register_failed:
 	ib_unregister_event_handler(&priv->event_handler);
+	flush_workqueue(ipoib_workqueue);
 	/* Stop GC if started before flush */
 	set_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
 	cancel_delayed_work(&priv->neigh_reap_task);

@@ -1709,6 +1710,7 @@ static void ipoib_remove_one(struct ib_device *device)
 
 	list_for_each_entry_safe(priv, tmp, dev_list, list) {
 		ib_unregister_event_handler(&priv->event_handler);
+		flush_workqueue(ipoib_workqueue);
 
 		rtnl_lock();
 		dev_change_flags(priv->dev, priv->dev->flags & ~IFF_UP);

2.1.0

--
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

The one I removed in particular can prevent progress in the driver from happening because it isn't rate limited. In my testing, I have seen the flood of messages from that debug message delay multicast joins for in excess of 30 seconds.

The added debug message was just so I would know when we were leaving a multicast group that we didn't fully join.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

index 10e05437c83..ad2bcd105e4 100644
@@ -711,6 +711,8 @@ 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",

@@ -721,7 +723,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;
 }

@@ -772,11 +776,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");
-
 		/*
 		 * If lookup completes between here and out:, don't
 		 * want to send packet twice.

2.1.0

--
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

Doug, Roland - eight rc1 patches followed ten rc6 fixes - sounds a bit too much to me. If we (and Linus) like or not, probably the right thing to do is revert the rc1 patches and come up with a set of hopefully < 18 patches for 3.20 - using squashes and such, and there's not much time for that too, BTW.

Taking into account Doug's listing of the remaining problems in the original set which are not addressed by the patch Erez suggested, this isn't an option either.

Alex, you acked all the series, haven't you noticed the IPv6 and IPv4 multicast breakage or it worked for you?

Or.

--
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

We can do as we wish upstream, but these are going live in our product ASAP. The original 8 rc1 patches took so long to get upstream that they had already been live in 6.5-z and 6.6 and were slated for 7.1. We had a few customers report problems, but not as many as you might think. The new patches are going to go out and fix those problems, period. I can't wait for 3.20 to fix these issues. C'est la vie.

FWIW, I expect that these patches (plus the one patch I didn't put in this same thread) to be live on production machines probably within 10 days (or less). I'll let you know how that goes.

I'm not going to squash them. I used to write big monolithic patches, back in the day when I was a newbie kernel engineer. Trying to review or understand the nuances of those patches was nigh on impossible. Here, each patch was unique in the issue it addressed and done that way particularly to make it easier to follow the logic later when someone might review this code and these changes. A big monolithic patch that hides these revisions hides the nuances that made getting this right difficult. I'm not going to go back to those days. Git is just fine with these patches being separated.

I agree with you on this point. But, I would also point out, that if you are looking for justification to take this current patch set, that's the real justification. It's all about understanding the problem, and dealing with it properly. My patch set does that. Patch count and line count are secondary to understanding when it comes to determining if a change is acceptable in late rc stage IMO.

Surprisingly enough, not many people use multicast. And the whole reason I didn't see the problem in the first place is that the initial multicast setup that's necessary to make the links come up works OK, it's only new joins after that which are effected. We haven't had any reports of multicast not working except from you. Your report was valid, just no one else cared (I'm sure someone would have eventually, but out of the people using 6.5-z and 6.6 kernels, no one reported multicast issues, just other issues).

Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

Given the level of testing you've done, and the fact that the changes are going to be in a Red Hat release, I'm inclined to try to get all this into 3.19. It may be more than Linus would take, and it's more than I would like to take so late in the cycle, but really it seems safer than reverting back to the status quo ante.

- R.
--
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

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogT3IgR2VybGl0eiBbbWFp bHRvOmdlcmxpdHoub3JAZ21haWwuY29tXQ0KPiBTZW50OiBGcmlkYXksIEphbnVhcnkgMjMsIDIw MTUgMjowMiBBTQ0KPiBUbzogRG91ZyBMZWRmb3JkDQo+IENjOiBsaW51eC1yZG1hQHZnZXIua2Vy bmVsLm9yZzsgUm9sYW5kIERyZWllcjsgQW1pciBWYWRhaTsgRXlhbCBQZXJyeTsgRXJleg0KPiBT aGl0cml0OyBFc3RyaW4sIEFsZXgNCj4gU3ViamVjdDogUmU6IFtQQVRDSCBGSVggRm9yLTMuMTkg djUgMDAvMTBdIEZpeCBpcG9pYiByZWdyZXNzaW9ucw0KPiANCj4gT24gVGh1LCBKYW4gMjIsIDIw MTUgYXQgNDozMSBQTSwgRG91ZyBMZWRmb3JkIDxkbGVkZm9yZEByZWRoYXQuY29tPiB3cm90ZToN Cj4gPiBNeSA4IHBhdGNoIHNldCB0YWtlbiBpbnRvIDMuMTkgY2F1c2VkIHNvbWUgcmVncmVzc2lv bnMuICBUaGlzIHBhdGNoDQo+ID4gc2V0IHJlc29sdmVzIHRob3NlIGlzc3Vlcy4NCj4gDQo+IERv dWcsIFJvbGFuZCAtIGVpZ2h0IHJjMSBwYXRjaGVzIGZvbGxvd2VkIHRlbiByYzYgZml4ZXMgLSBz b3VuZHMgYSBiaXQNCj4gdG9vIG11Y2ggdG8gbWUuIElmIHdlIChhbmQgTGludXMpIGxpa2Ugb3Ig bm90LCBwcm9iYWJseSB0aGUgcmlnaHQNCj4gdGhpbmcgdG8gZG8gaXMgcmV2ZXJ0IHRoZSByYzEg cGF0Y2hlcyBhbmQgY29tZSB1cCB3aXRoIGEgc2V0IG9mDQo+IGhvcGVmdWxseSA8IDE4IHBhdGNo ZXMgZm9yIDMuMjAgLSB1c2luZyBzcXVhc2hlcyBhbmQgc3VjaCwgYW5kIHRoZXJlJ3MNCj4gbm90 IG11Y2ggdGltZSBmb3IgdGhhdCB0b28sIEJUVy4NCj4gDQo+IFRha2luZyBpbnRvIGFjY291bnQg RG91ZydzIGxpc3Rpbmcgb2YgdGhlIHJlbWFpbmluZyBwcm9ibGVtcyBpbiB0aGUNCj4gb3JpZ2lu YWwgc2V0IHdoaWNoIGFyZSBub3QgYWRkcmVzc2VkIGJ5IHRoZSBwYXRjaCBFcmV6IHN1Z2dlc3Rl ZCwgdGhpcw0KPiBpc24ndCBhbiBvcHRpb24gZWl0aGVyLg0KPiANCj4gQWxleCwgeW91IGFja2Vk IGFsbCB0aGUgc2VyaWVzLCBoYXZlbid0IHlvdSBub3RpY2VkIHRoZSBJUHY2IGFuZCBJUHY0DQo+ IG11bHRpY2FzdCBicmVha2FnZSBvciBpdCB3b3JrZWQgZm9yIHlvdT8NCj4gDQoNCkkgZGlkbid0 IHJ1biBlbGFib3JhdGUgbXVsdGljYXN0IHRlc3RzIG9uIHRoZSBwYXRjaC1zZXQgc28gSSBkaWRu J3QgaGl0IGl0Lg0KV2FzIGNvbmNlbnRyYXRlZCBvbiBpbml0aWFsaXphdGlvbiBmbG93LiBUaGF0 IHdhcyBhIG1pc3Rha2UuDQoNCj4gT3IuDQo+IA0KPiA+IERvdWcgTGVkZm9yZCAoMTApOg0KPiA+ ICAgSUIvaXBvaWI6IGZpeCBJUE9JQl9NQ0FTVF9SVU4gZmxhZyB1c2FnZQ0KPiA+ICAgSUIvaXBv aWI6IEFkZCBhIGhlbHBlciB0byByZXN0YXJ0IHRoZSBtdWx0aWNhc3QgdGFzaw0KPiA+ICAgSUIv aXBvaWI6IG1ha2UgZGVsYXllZCB0YXNrcyBub3QgaG9sZCB1cCBldmVyeXRoaW5nDQo+ID4gICBJ Qi9pcG9pYjogSGFuZGxlIC1FTkVUUkVTRVQgcHJvcGVybHkgaW4gb3VyIGNhbGxiYWNrDQo+ID4g ICBJQi9pcG9pYjogZG9uJ3QgcmVzdGFydCBvdXIgdGhyZWFkIG9uIEVORVRSRVNFVA0KPiA+ICAg SUIvaXBvaWI6IHJlbW92ZSB1bm5lZWRlZCBsb2Nrcw0KPiA+ICAgSUIvaXBvaWI6IGZpeCByYWNl IGJldHdlZW4gbWNhc3RfZGV2X2ZsdXNoIGFuZCBtY2FzdF9qb2luDQo+ID4gICBJQi9pcG9pYjog Zml4IGlwb2liX21jYXN0X3Jlc3RhcnRfdGFzaw0KPiA+ICAgSUIvaXBvaWI6IGZsdXNoIHRoZSBp cG9pYl93b3JrcXVldWUgb24gdW5yZWdpc3Rlcg0KPiA+ICAgSUIvaXBvaWI6IGNsZWFudXAgYSBj b3VwbGUgZGVidWcgbWVzc2FnZXMNCj4gPg0KPiA+ICBkcml2ZXJzL2luZmluaWJhbmQvdWxwL2lw b2liL2lwb2liLmggICAgICAgICAgIHwgICAxICsNCj4gPiAgZHJpdmVycy9pbmZpbmliYW5kL3Vs cC9pcG9pYi9pcG9pYl9tYWluLmMgICAgICB8ICAgMiArDQo+ID4gIGRyaXZlcnMvaW5maW5pYmFu ZC91bHAvaXBvaWIvaXBvaWJfbXVsdGljYXN0LmMgfCAyMzQgKysrKysrKysrKysrKystLS0tLS0t LS0tLQ0KPiA+ICAzIGZpbGVzIGNoYW5nZWQsIDEzMSBpbnNlcnRpb25zKCspLCAxMDYgZGVsZXRp b25zKC0pDQo=
--
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

FWIW, a couple different customers have tried a test kernel I built internally with my patches and I've had multiple reports that all previously observed issues have been resolved.

Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

Hi Doug,
still see an issue with the last version, and as a result no sendonly or IPv6 is working.
The scenario is some how simple to reproduce, if there is a sendonly multicast group that failed to join (sm refuses, perhaps the group was closed, etc.) that causes all the other mcg's to be blocked behind it forever. for example, there is bad mcg ff12:601b:ffff:0000:0000:0000:0000:0016 that the sm refuses to join, and after some time the user tries to send packets to ip address 225.5.5.5 (mcg: ff12:401b:ffff:0000:0000:0000:0105:0505 ) the log will show something like:

[1561627.426080] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[1561633.726768] ib0: setting up send only multicast group for ff12:401b:ffff:0000:0000:0000:0105:0505
[1561643.498990] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[1561675.645424] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[1561691.718464] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[1561707.791609] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[1561723.864839] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[1561739.937981] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[1561756.010895] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join

....
....
for ever or till the sm will decide in the future to let ff12:601b:ffff:0000:0000:0000:0000:0016 join, till than no sendonly traffic.

The main cause is the concept that was broken for the send-only join, when you treat the sendonly like a regular mcg and add it to the mc list and to the mc_task etc. (not consider other issues like packet drop, and bw of the sendonly traffic)
IMHO, sendonly is part of the TX flow as it was till now in the ipoib driver and should stay in that way.

I went over your comments to my patch, will try to response/ cover them ASAP.

Thanks, Erez

--
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

I disagree with your assessment. See below.

This is not the case. "failed to join" implies that the join completed with an error. According to the logs below, something impossible is happening.

Your right. Because the join tasks are serialized and we only queue up one join at a time (something I would like to address, but not in the 3.19 timeframe), if one join simply never returns, as the logs below indicate, then it blocks up the whole system.

This is printed out by sendonly_join

This is part of mcast_send and indicates we have created the new group and added it to the mcast rbtree, but not that we've started the join.

Our mcast task got ran again, and we are restarting the same group for some reason. Theoretically that means we should have cleared the busy flag on this group somehow, which requires that sendonly_join_complete must have been called. However, I can't find a single code path in sendonly_join_complete that could possibly clear out the busy flag on this group and not result in a printout message of some sort. Are you trimming the list of messages here? The reason I say this is that these are the possibilities:

sendonly_join_complete with status == ENETRESET, we silently drop out but we would immediately get an ib_event with netreset and process a flush, which would print out copious messages sendonly_join_complete with status == 0, we would call mcast_join_finish and the only silent return is if this is our broadcast group and priv->broadcast is not set, in which case we don't have link up and the silent failures and the failure to join any other address are normal and expected since the ipoib layer considers things dead without the broadcast group joined, every other return path will print out something, and in particular the attempt to get an ah on the group will always print out either success or failure sendonly_join_complete with status anything else results in a debug message about the join failing.

As far as I know, your setup has the IPv4 broadcast attached and the link is up. The group you are actually showing as bad is the IPv6 MLDv2-capable Routers group and failure to join it should not be blocking anything else. So I'm back to what I said before: unless you are trimming messages, this debug output makes no sense whatsoever and should be impossible to generate. Are you sure you have a clean tree with no possible merge oddities that are throwing your testing off?

These are all being queued with successively longer backoffs, exactly as they should be.

In __ipoib_mcast_continue_join_thread, when we get called with delay != 0, we end up doing two things. One, we change the mcast->backoff from 1 to whatever it should be now (minimum of 2, maximum of 16), and we set delay_until to jiffies + (HZ * mcast->backoff). This lets the mcast task know to skip this group until such time as its backoff is expired. From the messages above, that seems to be working. The other thing we do is we don't queue up the mcast thread just once, we queue it up twice. Once at the right time for the backoff, and again now (although there is a bug here, the second instance queues it up with delay as the time offset, that should be 0, that's a holdover from before, at least fortunately it is only ever 1 or 0 so it's not horrible, but things would go a little faster if I fix that so that the second instance is always 0). That way the instance that's queued up first will know to skip the delayed group and continue on through the list, and that should allow us to get these other groups that you say are blocked.

I'm looking at 3.18 right now, and 3.18 adds sendonly groups to the mcg just like my current code does. The only difference, and I do mean *only*, is that it calls sendonly_join directly instead of via the mcast_task. The net effect of that is that sendonly joins are not serialized in the old version and they are in the new. I'm more than happy to deserialize them, but I don't think that necessarily means we have to call sendonly_join directly from mcast_send, it could also be done by allowing the mcast_task to fire off more than one join at a time. Whether we use the mcast_task or call sendonly_join directly is in-consequential, it's more an issue of whether or not we serialize the joins.

It seems like you're thinking of the mcast task thread as something that only runs as part of mcast joins or leaves when the upper layer calls set multicast list in our driver. It used to be that way, but since I reworked the task thread, that's no longer the case. As such, I don't think this distinction you are drawing is necessarily still correct.

Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

New (and full) dmesg attached, (after modprobe ib_ipoib, with all debug flags set) it is all there.

the case is simple: ping6
In device A:

# ip addr show ib0
19: ib0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc mq state UP qlen 256
    link/infiniband 
a0:04:02:10:fe:80:00:00:00:00:00:00:00:02:c9:03:00:9f:3b:0a brd 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
    inet 11.134.33.1/8 brd 11.255.255.255 scope global ib0
    inet6 fe80::202:c903:9f:3b0a/64 scope link
       valid_lft forever preferred_lft forever

In device B, where i have the latest version:
#ping6 fe80::202:c903:9f:3b0a -I ib0

double checked myself, new git, with the last 10 patches, recompiled all modules, reboot etc. the same issue.

#git log --oneline
362509a IB/ipoib: cleanup a couple debug messages
e8e1618 IB/ipoib: flush the ipoib_workqueue on unregister
867555c IB/ipoib: fix ipoib_mcast_restart_task
125a466 IB/ipoib: fix race between mcast_dev_flush and mcast_join
9b0908b IB/ipoib: remove unneeded locks
c2fb665 IB/ipoib: don't restart our thread on ENETRESET
2345213 IB/ipoib: Handle -ENETRESET properly in our callback
6ed68e8 IB/ipoib: make delayed tasks not hold up everything
bee92be5 IB/ipoib: Add a helper to restart the multicast task
2084aee IB/ipoib: fix IPOIB_MCAST_RUN flag usage
a7cfef2 Merge branches 'core', 'cxgb4', 'ipoib', 'iser', 'mlx4', 'ocrdma', 'odp' and 'srp' into for-next
....
....

Yes, and i already wrote that it is more than just "only", it changed the concept of the sendonly mc packet.

[ 2425.913245] ib0: bringing up interface
[ 2425.918035] ib0: starting multicast thread
[ 2425.922260] IPv6: ADDRCONF(NETDEV_UP): ib0: link is not ready
[ 2425.928542] ib0: joining MGID ff12:401b:ffff:0000:0000:0000:ffff:ffff
[ 2425.935065] ib0: restarting multicast task
[ 2425.939228] ib0: adding multicast entry for mgid ff12:601b:ffff:0000:0000:0000:0000:0001
[ 2425.939335] ib0: join completion for ff12:401b:ffff:0000:0000:0000:ffff:ffff (status 0)
[ 2425.955538] ib0: adding multicast entry for mgid ff12:401b:ffff:0000:0000:0000:0000:0001
[ 2425.964891] ib0: Created ah ffff8800ca0b24c0
[ 2425.969231] ib0: MGID ff12:401b:ffff:0000:0000:0000:ffff:ffff AV ffff8800ca0b24c0, LID 0xc000, SL 0
[ 2425.978434] ib0: joining MGID ff12:601b:ffff:0000:0000:0000:0000:0001
[ 2425.985613] ib0: joining MGID ff12:401b:ffff:0000:0000:0000:0000:0001
[ 2425.992595] IPv6: ADDRCONF(NETDEV_CHANGE): ib0: link becomes ready
[ 2425.998967] ib0: restarting multicast task
[ 2426.001183] ib0: setting up send only multicast group for ff12:601b:ffff:0000:0000:0000:0000:0016
[ 2426.012123] ib0: adding multicast entry for mgid ff12:601b:ffff:0000:0000:0001:ff43:3bf1
[ 2426.021059] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2426.330297] ib0: join completion for ff12:601b:ffff:0000:0000:0000:0000:0001 (status 0)
[ 2426.339054] ib0: Created ah ffff8800ca0b25c0
[ 2426.343389] ib0: MGID ff12:601b:ffff:0000:0000:0000:0000:0001 AV ffff8800ca0b25c0, LID 0xc016, SL 0
[ 2426.353901] ib0: join completion for ff12:401b:ffff:0000:0000:0000:0000:0001 (status 0)
[ 2426.362984] ib0: Created ah ffff88041f104440
[ 2426.367316] ib0: MGID ff12:401b:ffff:0000:0000:0000:0000:0001 AV ffff88041f104440, LID 0xc014, SL 0
[ 2426.376487] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2426.386032] ib0: joining MGID ff12:601b:ffff:0000:0000:0001:ff43:3bf1
[ 2426.393162] ib0: join completion for ff12:601b:ffff:0000:0000:0001:ff43:3bf1 (status 0)
[ 2426.402013] ib0: Created ah ffff88041f115340
[ 2426.406349] ib0: MGID ff12:601b:ffff:0000:0000:0001:ff43:3bf1 AV ffff88041f115340, LID 0xc018, SL 0
[ 2426.415567] ib0: successfully joined all multicast groups
[ 2426.421610] ib0: successfully joined all multicast groups
[ 2427.037903] ib0: setting up send only multicast group for ff12:601b:ffff:0000:0000:0000:0000:0002
[ 2427.047677] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0002, starting sendonly join
[ 2427.057362] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0002, status -22
[ 2429.073774] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2429.083391] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2433.100207] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2433.109820] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2434.594637] ib0: setting up send only multicast group for ff12:601b:ffff:0000:0000:0001:ff9f:3b0a
[ 2441.136671] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2441.146279] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2457.210044] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2457.219652] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2473.282869] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2473.292482] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2489.356138] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2489.365749] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2505.429325] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2505.438941] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2516.191339] ib0: neigh free for ffffff ff12:601b:ffff:0000:0000:0001:ff43:3bf1
[ 2521.502141] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2521.511744] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2537.575375] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2537.584980] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2553.648531] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2553.658137] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2569.721539] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2569.731143] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2585.794694] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2585.804307] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2601.867993] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2601.877600] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2617.940929] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2617.950538] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2634.014165] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2634.023776] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2650.087367] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2650.096972] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2666.160196] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2666.169811] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2682.233359] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2682.242969] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2698.306545] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2698.316151] ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[ 2714.379507] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2730.452688] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2746.525811] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2762.598925] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2778.672146] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2794.745171] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2810.818171] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2826.891541] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[ 2842.964546] ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join

Thank you, I know what's going on here now. Will correct shortly.

Be more specific please. What do you mean by "concept"? And just so we are clear, this all started because the existing multicast code was super easy to break and was racy, so if the "concept" you are referring to is what made the original code easy to break and racy, I'm not going to care one whit that I changed that concept.

Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

welcome -:)

I agree that you fixed many bugs in your patches to 3.18, where the mc flow was easy to break, no argue about that.
The only issue that i disagree is about the way now sendonly is handled (and i think that this is the reason for the regression we see now).
In general, IMHO, the sendonly join is part of the TX flow and not part of the ipoib_set_mcast_list flow.
The original meaning of the ipoib_set_mcast_list task that restart the mc_task is to be used for the kernel in order to add one or more new mcg's macs to the driver/HW (ndo_set_rx_mode), the sendonly mc is not such object, its mac should not be part of the "mac" list of the driver (in IB wards, no qp_attach for it) and from the kernel point of view whenever it sends packet from sendonly mcg type no need to do the join, it's a regular send, the only reason we have the sendonly join is the IB enforcement for such mcg. The reason the driver keeps the sendonly mcg in its mc_list is from others reasons, the first is to handle the case when the kernel decides to move a mcg from sendonly membership to full-member, one more other reason is to do the leave operation when needed and not for being handled as a full-member mcg.

--
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

It's not. The patch I sent you off list should be sufficient at this point to finally put your issues to rest.

I disagree. I'll get into that below.

This part I agree with.

This is where you and I differ. There is a requirement on us from the IB spec that we have to join a sendonly MC group in order to do what the kernel would think of as a normal send if we were on Ethernet. We aren't on Ethernet though, so acting like we are to the above layer is fine, but in our own layer we should be coding to the realities of our layer. And that means treating a sendonly MC group like a regular MC group.

It's been a long time since I first started working on this issue, so some of the details are fuzzy in my memory. I think my first 8 patch patchset is now about 6 months old. But, I think probably the most important thing you left off this list, and the one that trumps all the others, is that whether a sendonly MC group has a QP attached or not, we still have to account for these groups, we have to track them, and on shutdown we have to reliably leave them without oopsing. Putting the join of the sendonly groups directly in mcast_send opens us up to problems with synchronizing our mcast list (either due to a dev flush, a mcast restart task, or something else). I can't say for certain that the change to how we record the return value from ib_sa_join_multicast in sendonly_join is enough to keep the sendonly join code from racing with the flush code and causing problems. For that reason, I am highly reluctant to put the sendonly join back directly into the tx path because that could have been part of the original problem in the first place, it's just that my memory of back then is too fuzzy to say that with 100% certainty either way.

Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

Just to make it clear, the IB request to do sendonly join was done prior to your changes. (stopped after them.)
The IB spec doesn't care if you mix sendonly with full-member, the kernel does care, and by adding them to the mc_list you abuse the kernel ndo (by adding unnecessary joins to it, as i wrote in my previous mail) I think you should not mix between RX operation (like the full-member join, kernel rx ndo) to TX operation (sendonly join)

This is the reason the sendonly mcg was added to the mc_list. and should stay there.

That fear from the unknown is not a reason to break the original logic of the sendonly. if there is such a problem, we need to find it and to fix.

Thanks, Erez

--
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

Yes...

What do you mean here? With my changes we still do sendonly joins.

Correct.

Where? What part of the kernel cares?

How? We never add a sendonly during a typical set_multicast call invocation. And in fact, we still do the exact same thing we used to do when the kernel ndo is called, which is to call restart_task which will scan our currently joined groups (both regular and sendonly, like it always has), clear out any that don't have a matching address entry set via the kernel ndo, and rejoin those that do have a matching entry set via the ndo.

And I think having two distinctly different code paths when it doesn't matter whether the join is sendonly or normal, they still must all be accurately tracked and reaped on shutdown and various other events, is wrong and is an invitation to failures for no good reason.

No, the reason to break the original logic of the sendonly was that it was a bad design that contributed to other problems. The reason not to consider putting that bad logic back in place right now, IMO, is that I can't remember the details of the breakage better than I do at this moment.

I disagree. The original logic was an artificial and bad separation of too more or less identical things in the code that helped contribute to it being broken in the first place. We are under no obligation to put it back to that bad design. And in the future, I'd like to bring the sendonly and regular joins more in line with each other, quite possible doing away with them having separate functions. I'd also like to completely deserialize them. Both of these things will be easier to do from the starting point we have now versus going back to the old setup.

Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

I munged my opensm configuration so that I could forcibly replicate the situation here (I intentionally took several well known multicast groups and forbid their creation).

I was able to first replicate Eriz's problem.

Then I installed a new ib_ipoib module with my proposed fix for Erez's problem and it worked exactly as expected. It was a mistake in one of my earlier patches (the third in the series). When I added a delayed queue of the task thread, I didn't have a separate work struct and instead tried to queue the same work struct twice. I reworked it so that the work struct is only ever queued once and if the multicast task gets to the end of its run and there are delayed entries waiting still, it will queue itself to run again when the shortest delay has expired. I'll send that through.

Here's the log of the attempt:

[root@rdma-master linus (firewall/for-rc)]$ dmesg | tail -10

[337072.429488] mlx4_ib0: successfully joined all multicast groups
[337073.856932] mlx4_ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0002, starting sendonly join
[337073.869686] mlx4_ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0002, status -22
[337073.882754] mlx4_ib0: successfully joined all multicast groups
[337088.480082] mlx4_ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join
[337088.492789] mlx4_ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[337088.505819] mlx4_ib0: successfully joined all multicast groups
[337089.897041] mlx4_ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0002, starting sendonly join
[337089.909870] mlx4_ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0002, status -22
[337089.922893] mlx4_ib0: successfully joined all multicast groups

[root@rdma-master linus (firewall/for-rc)]$ ping6 -I mlx4_ib0 fe80::211:7500:77:d3cc
PING fe80::211:7500:77:d3cc(fe80::211:7500:77:d3cc) from fe80::f652:1403:7b:cba1 mlx4_ib0: 56 data bytes
64 bytes from fe80::211:7500:77:d3cc: icmp_seq=1 ttl=64 time=77.6 ms
64 bytes from fe80::211:7500:77:d3cc: icmp_seq=2 ttl=64 time=0.159 ms
64 bytes from fe80::211:7500:77:d3cc: icmp_seq=3 ttl=64 time=0.125 ms
64 bytes from fe80::211:7500:77:d3cc: icmp_seq=4 ttl=64 time=0.128 ms
^C
--- fe80::211:7500:77:d3cc ping statistics ---
4 packets transmitted, 4 received, 0% packet loss, time 3001ms
rtt min/avg/max/mdev = 0.125/19.503/77.600/33.542 ms
[root@rdma-master linus (firewall/for-rc)]$ dmesg | tail -10[337120.632427] mlx4_ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0016, starting sendonly join

[337120.645166] mlx4_ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0016, status -22
[337120.658292] mlx4_ib0: successfully joined all multicast groups
[337121.977733] mlx4_ib0: no multicast record for ff12:601b:ffff:0000:0000:0000:0000:0002, starting sendonly join
[337121.990478] mlx4_ib0: sendonly multicast join failed for ff12:601b:ffff:0000:0000:0000:0000:0002, status -22
[337122.003589] mlx4_ib0: successfully joined all multicast groups
[337130.410559] mlx4_ib0: setting up send only multicast group for ff12:601b:ffff:0000:0000:0001:ff77:d3cc
[337130.423203] mlx4_ib0: no multicast record for ff12:601b:ffff:0000:0000:0001:ff77:d3cc, starting sendonly join
[337130.436327] mlx4_ib0: MGID ff12:601b:ffff:0000:0000:0001:ff77:d3cc AV ffff882027235f00, LID 0xc01e, SL 0
[337130.448970] mlx4_ib0: successfully joined all multicast groups

[root@rdma-master linus (firewall/for-rc)]$

Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

In b2d408f78b3 (IB/ipoib: make delayed tasks not hold up everything) I added the ability to have some joins processed around delayed joins. Because the join task processes one join at a time, we needed to queue the task to run again immediately and then again at the appropriate delay time. I didn't think about the fact that our work struct can only be on the workqueue once at a given time and instead tried to queue the same struct up twice. Instead, kick the queue again immediately on a delayed restart attempt, when the task sees that it has nothing to do but delayed work, it will pick the shortest work item and queue a delay equal to its expiration. Finally, in case we have delayed work, in the send_mcast routine we need to cancel any delayed work before kicking the thread immediately.

---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 49 ++++++++++++++++----------
 1 file changed, 31 insertions(+), 18 deletions(-)

index ad2bcd105e4..68b47bed33a 100644
@@ -71,29 +71,28 @@ static void __ipoib_mcast_continue_join_thread(struct ipoib_dev_priv *priv,
 					       int delay)
 {
 	if (test_bit(IPOIB_MCAST_RUN, &priv->flags)) {
-		/*
-		 * Mark this mcast for its delay and set a timer to kick the
-		 * thread when the delay completes
-		 */
 		if (mcast && delay) {
 			mcast->backoff *= 2;
 			if (mcast->backoff > IPOIB_MAX_BACKOFF_SECONDS)
 				mcast->backoff = IPOIB_MAX_BACKOFF_SECONDS;
 			mcast->delay_until = jiffies + (mcast->backoff * HZ);
-			queue_delayed_work(priv->wq, &priv->mcast_task,
-					   mcast->backoff * HZ);
+			/*
+			 * Mark this mcast for its delay, but restart the
+			 * task immediately.  It will make sure to clear
+			 * out all entries without delays, and then
+			 * schedule itself for run again when the delay
+			 * expires
+			 */
+			queue_delayed_work(priv->wq, &priv->mcast_task, 0);
 		} else if (delay) {
-			/* Special case of retrying after a failure to
+			/*
+			 * 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);
-		}
-		/*
-		 * But also rekick the thread immediately for any other
-		 * tasks in queue behind this one
-		 */
-		queue_delayed_work(priv->wq, &priv->mcast_task, delay);
+		} else
+			queue_delayed_work(priv->wq, &priv->mcast_task, 0);
 	}
 }
 

@@ -568,6 +567,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
 	struct ib_port_attr port_attr;
 	struct ipoib_mcast *mcast = NULL;
 	int create = 1;
+	unsigned long delay_until = 0;
 
 	if (!test_bit(IPOIB_MCAST_RUN, &priv->flags))
 		return;

@@ -636,11 +636,14 @@ 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) &&
-		    (mcast->backoff == 1 ||
-		     time_after_eq(jiffies, mcast->delay_until))) {
-			/* Found the next unjoined group */
-			break;
+		    !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;
 		}
 	}
 

@@ -653,6 +656,9 @@ void ipoib_mcast_join_task(struct work_struct *work)
 		mcast = NULL;
 		ipoib_dbg_mcast(priv, "successfully joined all "
 				"multicast groups\n");
+		if (delay_until)
+			queue_delayed_work(priv->wq, &priv->mcast_task,
+					   delay_until - jiffies);
 	}
 
 out:

@@ -766,6 +772,13 @@ 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);
+		/*
+		 * Make sure that if we are currently waiting for a backoff
+		 * delay to expire, that we cancel that, run immediately,
+		 * and then requeue our task to fire when the backoff
+		 * timer is over.
+		 */
+		cancel_delayed_work(&priv->mcast_task);
 		__ipoib_mcast_continue_join_thread(priv, NULL, 0);
 	}
 

2.1.0

--
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