IB/ipoib: fixup multicast join thread usage

PATCH Superseded Doug Ledford 1 participant 4 messages Jan 14, 2015 1 message not in the archive InfiniBand

Share & tools

Doug Ledford

author

2015-01-14 19:05 UTC #0 lore raw

v2 cover IB/ipoib: fixup multicast join thread usage

This patch series fixes the multicast join behavior problems introduced by my previous patchset. In particular, the original code did not use the send only join code from the multicast thread context, and so it did not need to restart the multicast thread. After my previous patchset, it does get called from the thread context, and so the send only join completion areas need to restart the join thread but they don't. This patchset makes them do so. It then adds in some cleanups for restarting the thread, and fixes the fact that one delayed join holds up the entire list of joins.

Doug Ledford (3):
IB/ipoib: Fix failed multicast joins/sends IB/ipoib: Add a helper to restart the multicast task IB/ipoib: make delayed tasks not hold up everything

 drivers/infiniband/ulp/ipoib/ipoib.h           |  1 +
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 94 ++++++++++++++++++--------
 2 files changed, 66 insertions(+), 29 deletions(-)

Signature · 6 lines

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

A message that is not in the archive (sent off-list or not yet received): cover.1421261785.git.dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Doug Ledford

author

2015-01-14 19:05 UTC #2 lore raw

PATCH v2 1/3 IB/ipoib: Fix failed multicast joins/sends

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.

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

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

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +5 -6

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();
 }

Signature · 6 lines

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 Ledford

author

2015-01-14 19:05 UTC #3 lore raw

PATCH v2 2/3 IB/ipoib: Add a helper to restart the multicast task

In reply to a message not in the archive

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.

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

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

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +29 ~~-17~~

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();
 }

Signature · 6 lines

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 Ledford

author

2015-01-14 19:05 UTC #4 lore raw

PATCH v2 3/3 IB/ipoib: make delayed tasks not hold up everything

In reply to a message not in the archive

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.

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

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

drivers/infiniband/ulp/ipoib/ipoib.h +1 -0

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;

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +55 ~~-30~~

166 lines not on this page · raw message

Signature · 6 lines

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