IB multicast cleanup patches

PATCH In review Christoph Lameter 2 participants 9 messages Dec 11, 2015 – Dec 14, 2015 5 messages not in the archive InfiniBand

lore

Share & tools

lore

Christoph Lameter

author

2015-12-11 18:16 UTC #0 lore raw

cover IB multicast cleanup patches

This patchset cleans up the code a bit after the last round of multicast patches related to the sendonly join logic. Some of the bits of code landed in ipoib_main.c instead of ipoib_multicast.c.

- Move the multicast bits into that file so that everything is neatly together
- Reduce the number of functions exported from ipoib_multicast.c

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

Christoph Lameter

author

2015-12-11 18:16 UTC #1 lore raw

PATCH 1/2 ipoib mcast sendonly join: Isolate common list remove code

Code cleanup to remove multicast specific code from ipoib_main.c

The removal of a list of multicast groups occurs in three places. Create a new function ipoib_mcast_remove_list(). Use this new function in ipoib_main.c too.
That in turn allows the dropping of two functions that were exported from ipoib_multicast.c for expiration of mc groups.

Signed-off-by Christoph Lameter <[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib.h           |  3 +--
 drivers/infiniband/ulp/ipoib/ipoib_main.c      |  7 ++-----
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 24 ++++++++++++++----------
 3 files changed, 17 insertions(+), 17 deletions(-)

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

index 3ede103..989c409 100644
@@ -495,7 +495,6 @@ void ipoib_dev_cleanup(struct net_device *dev);
 void ipoib_mcast_join_task(struct work_struct *work);
 void ipoib_mcast_carrier_on_task(struct work_struct *work);
 void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb);
-void ipoib_mcast_free(struct ipoib_mcast *mc);
 
 void ipoib_mcast_restart_task(struct work_struct *work);
 int ipoib_mcast_start_thread(struct net_device *dev);

@@ -549,7 +548,7 @@ void ipoib_path_iter_read(struct ipoib_path_iter *iter,
 
 int ipoib_mcast_attach(struct net_device *dev, u16 mlid,
 		       union ib_gid *mgid, int set_qkey);
-int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast);
+void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list);
 struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid);
 
 int ipoib_init_qp(struct net_device *dev);

drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -5

index 7d32818..483ff20 100644
@@ -1150,7 +1150,7 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 	unsigned long flags;
 	int i;
 	LIST_HEAD(remove_list);
-	struct ipoib_mcast *mcast, *tmcast;
+	struct ipoib_mcast *mcast;
 	struct net_device *dev = priv->dev;
 
 	if (test_bit(IPOIB_STOP_NEIGH_GC, &priv->flags))

@@ -1207,10 +1207,7 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 
 out_unlock:
 	spin_unlock_irqrestore(&priv->lock, flags);
-	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
-		ipoib_mcast_leave(dev, mcast);
-		ipoib_mcast_free(mcast);
-	}
+	ipoib_mcast_remove_list(dev, &remove_list);
 }
 
 static void ipoib_reap_neigh(struct work_struct *work)

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +14 ~~-10~~

index f357ca6..8acb420a 100644
@@ -106,7 +106,7 @@ static void __ipoib_mcast_schedule_join_thread(struct ipoib_dev_priv *priv,
 		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
 }
 
-void ipoib_mcast_free(struct ipoib_mcast *mcast)
+static void ipoib_mcast_free(struct ipoib_mcast *mcast)
 {
 	struct net_device *dev = mcast->dev;
 	int tx_dropped = 0;

@@ -677,7 +677,7 @@ int ipoib_mcast_stop_thread(struct net_device *dev)
 	return 0;
 }
 
-int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
+static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	int ret = 0;

@@ -704,6 +704,16 @@ int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
 	return 0;
 }
 
+void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list)
+{
+	struct ipoib_mcast *mcast, *tmcast;
+
+	list_for_each_entry_safe(mcast, tmcast, remove_list, list) {
+		ipoib_mcast_leave(dev, mcast);
+		ipoib_mcast_free(mcast);
+	}
+}
+
 void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);

@@ -810,10 +820,7 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
 		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(dev, mcast);
-		ipoib_mcast_free(mcast);
-	}
+	ipoib_mcast_remove_list(dev, &remove_list);
 }
 
 static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)

@@ -939,10 +946,7 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 		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);
-	}
+	ipoib_mcast_remove_list(mcast->dev, &remove_list);
 
 	/*
 	 * Double check that we are still up

Signature · 7 lines

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

Christoph Lameter

author

2015-12-11 18:16 UTC #4 lore raw

PATCH 2/2 ipoib mcast sendonly join: Move multicast specific code out of ipoib_main.c.

In reply to Christoph Lameter

Code cleanup to move multicast specific code that checks for a sendonly join to ipoib_multicast.c. This allows the removal of the export of __ipoib_mcast_find().

Signed-off-by Christoph Lameter <[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib.h           |  3 ++-
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 13 +------------
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 21 ++++++++++++++++++++-
 3 files changed, 23 insertions(+), 14 deletions(-)

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

index 989c409..a13f48c 100644
@@ -549,7 +549,8 @@ void ipoib_path_iter_read(struct ipoib_path_iter *iter,
 int ipoib_mcast_attach(struct net_device *dev, u16 mlid,
 		       union ib_gid *mgid, int set_qkey);
 void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list);
-struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid);
+void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+				struct list_head *remove_list);
 
 int ipoib_init_qp(struct net_device *dev);
 int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca);

drivers/infiniband/ulp/ipoib/ipoib_main.c +1 ~~-12~~

index 483ff20..6b16428 100644
@@ -1150,7 +1150,6 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 	unsigned long flags;
 	int i;
 	LIST_HEAD(remove_list);
-	struct ipoib_mcast *mcast;
 	struct net_device *dev = priv->dev;
 
 	if (test_bit(IPOIB_STOP_NEIGH_GC, &priv->flags))

@@ -1179,18 +1178,8 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 							  lockdep_is_held(&priv->lock))) != NULL) {
 			/* was the neigh idle for two GC periods */
 			if (time_after(neigh_obsolete, neigh->alive)) {
-				u8 *mgid = neigh->daddr + 4;
 
-				/* Is this multicast ? */
-				if (*mgid == 0xff) {
-					mcast = __ipoib_mcast_find(dev, mgid);
-
-					if (mcast && test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags)) {
-						list_del(&mcast->list);
-						rb_erase(&mcast->rb_node, &priv->multicast_tree);
-						list_add_tail(&mcast->list, &remove_list);
-					}
-				}
+				ipoib_check_mcast_sendonly(priv, neigh->daddr + 4, &remove_list);
 
 				rcu_assign_pointer(*np,
 						   rcu_dereference_protected(neigh->hnext,

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +20 -1

index 8acb420a..1158819 100644
@@ -153,7 +153,7 @@ static struct ipoib_mcast *ipoib_mcast_alloc(struct net_device *dev,
 	return mcast;
 }
 
-struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid)
+static struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	struct rb_node *n = priv->multicast_tree.rb_node;

@@ -704,6 +704,25 @@ static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
 	return 0;
 }
 
+/*
+ * Check if the multicast group is sendonly. If so remove it from the maps
+ * and add to the remove list
+ */
+void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+				struct list_head *remove_list)
+{
+	/* Is this multicast ? */
+	if (*mgid == 0xff) {
+		struct ipoib_mcast *mcast = __ipoib_mcast_find(priv->dev, mgid);
+
+		if (mcast && test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags)) {
+			list_del(&mcast->list);
+			rb_erase(&mcast->rb_node, &priv->multicast_tree);
+			list_add_tail(&mcast->list, remove_list);
+		}
+	}
+}
+
 void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list)
 {
 	struct ipoib_mcast *mcast, *tmcast;

Signature · 7 lines

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

ira.weiny

Reviewed-by

2015-12-11 23:10 UTC #3 lore raw

Re: [PATCH 1/2] ipoib mcast sendonly join: Isolate common list remove code

In reply to a message not in the archive

Quoted 9 lines · Christoph Lameter

Code cleanup to remove multicast specific code from ipoib_main.c
The removal of a list of multicast groups occurs in three places. Create a new function ipoib_mcast_remove_list(). Use this new function in ipoib_main.c too.
That in turn allows the dropping of two functions that were exported from ipoib_multicast.c for expiration of mc groups.
Signed-off-by Christoph Lameter <[email protected]>

Reviewed-by Ira Weiny <[email protected]>

Quoted 123 lines

---
 drivers/infiniband/ulp/ipoib/ipoib.h           |  3 +--
 drivers/infiniband/ulp/ipoib/ipoib_main.c      |  7 ++-----
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 24 ++++++++++++++----------
 3 files changed, 17 insertions(+), 17 deletions(-)

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

index 3ede103..989c409 100644
@@ -495,7 +495,6 @@ void ipoib_dev_cleanup(struct net_device *dev);
 void ipoib_mcast_join_task(struct work_struct *work);
 void ipoib_mcast_carrier_on_task(struct work_struct *work);
 void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb);
-void ipoib_mcast_free(struct ipoib_mcast *mc);
 
 void ipoib_mcast_restart_task(struct work_struct *work);
 int ipoib_mcast_start_thread(struct net_device *dev);

@@ -549,7 +548,7 @@ void ipoib_path_iter_read(struct ipoib_path_iter *iter,
 
 int ipoib_mcast_attach(struct net_device *dev, u16 mlid,
 		       union ib_gid *mgid, int set_qkey);
-int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast);
+void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list);
 struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid);
 
 int ipoib_init_qp(struct net_device *dev);

drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -5

index 7d32818..483ff20 100644
@@ -1150,7 +1150,7 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 	unsigned long flags;
 	int i;
 	LIST_HEAD(remove_list);
-	struct ipoib_mcast *mcast, *tmcast;
+	struct ipoib_mcast *mcast;
 	struct net_device *dev = priv->dev;
 
 	if (test_bit(IPOIB_STOP_NEIGH_GC, &priv->flags))

@@ -1207,10 +1207,7 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 
 out_unlock:
 	spin_unlock_irqrestore(&priv->lock, flags);
-	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
-		ipoib_mcast_leave(dev, mcast);
-		ipoib_mcast_free(mcast);
-	}
+	ipoib_mcast_remove_list(dev, &remove_list);
 }
 
 static void ipoib_reap_neigh(struct work_struct *work)

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +14 ~~-10~~

index f357ca6..8acb420a 100644
@@ -106,7 +106,7 @@ static void __ipoib_mcast_schedule_join_thread(struct ipoib_dev_priv *priv,
 		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
 }
 
-void ipoib_mcast_free(struct ipoib_mcast *mcast)
+static void ipoib_mcast_free(struct ipoib_mcast *mcast)
 {
 	struct net_device *dev = mcast->dev;
 	int tx_dropped = 0;

@@ -677,7 +677,7 @@ int ipoib_mcast_stop_thread(struct net_device *dev)
 	return 0;
 }
 
-int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
+static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	int ret = 0;

@@ -704,6 +704,16 @@ int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
 	return 0;
 }
 
+void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list)
+{
+	struct ipoib_mcast *mcast, *tmcast;
+
+	list_for_each_entry_safe(mcast, tmcast, remove_list, list) {
+		ipoib_mcast_leave(dev, mcast);
+		ipoib_mcast_free(mcast);
+	}
+}
+
 void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);

@@ -810,10 +820,7 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
 		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(dev, mcast);
-		ipoib_mcast_free(mcast);
-	}
+	ipoib_mcast_remove_list(dev, &remove_list);
 }
 
 static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)

@@ -939,10 +946,7 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 		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);
-	}
+	ipoib_mcast_remove_list(mcast->dev, &remove_list);
 
 	/*
 	 * Double check that we are still up

Signature · 7 lines

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

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

ira.weiny

2015-12-11 23:21 UTC #6 lore raw

Re: [PATCH 2/2] ipoib mcast sendonly join: Move multicast specific code out of ipoib_main.c.

In reply to a message not in the archive

Quoted 16 lines · Christoph Lameter

Code cleanup to move multicast specific code that checks for a sendonly join to ipoib_multicast.c. This allows the removal of the export of __ipoib_mcast_find().
Signed-off-by Christoph Lameter <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib.h           |  3 ++-
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 13 +------------
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 21 ++++++++++++++++++++-
 3 files changed, 23 insertions(+), 14 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +0 -0
index 989c409..a13f48c 100644

drivers/infiniband/ulp/ipoib/ipoib.h void ipoib_path_iter_read(struct ipoib_path_iter *iter, · line 553

Quoted 6 lines

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

@@ -549,7 +549,8 @@ void ipoib_path_iter_read(struct ipoib_path_iter *iter,
 int ipoib_mcast_attach(struct net_device *dev, u16 mlid,
 		       union ib_gid *mgid, int set_qkey);
 void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list);
-struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid);
+void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+				struct list_head *remove_list);

I think I would rather see this called something like

ipoib_add_to_list_sendonly

Or something...

Calling it iboib_check* sounds like it should return a bool.

Quoted 51 lines

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

 int ipoib_init_qp(struct net_device *dev);
 int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca);

drivers/infiniband/ulp/ipoib/ipoib_main.c +1 ~~-12~~

index 483ff20..6b16428 100644
@@ -1150,7 +1150,6 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 	unsigned long flags;
 	int i;
 	LIST_HEAD(remove_list);
-	struct ipoib_mcast *mcast;
 	struct net_device *dev = priv->dev;
 
 	if (test_bit(IPOIB_STOP_NEIGH_GC, &priv->flags))

@@ -1179,18 +1178,8 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 							  lockdep_is_held(&priv->lock))) != NULL) {
 			/* was the neigh idle for two GC periods */
 			if (time_after(neigh_obsolete, neigh->alive)) {
-				u8 *mgid = neigh->daddr + 4;
 
-				/* Is this multicast ? */
-				if (*mgid == 0xff) {
-					mcast = __ipoib_mcast_find(dev, mgid);
-
-					if (mcast && test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags)) {
-						list_del(&mcast->list);
-						rb_erase(&mcast->rb_node, &priv->multicast_tree);
-						list_add_tail(&mcast->list, &remove_list);
-					}
-				}
+				ipoib_check_mcast_sendonly(priv, neigh->daddr + 4, &remove_list);
 
 				rcu_assign_pointer(*np,
 						   rcu_dereference_protected(neigh->hnext,

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

index 8acb420a..1158819 100644
@@ -153,7 +153,7 @@ static struct ipoib_mcast *ipoib_mcast_alloc(struct net_device *dev,
 	return mcast;
 }
 
-struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid)
+static struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	struct rb_node *n = priv->multicast_tree.rb_node;

@@ -704,6 +704,25 @@ static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
 	return 0;
 }

drivers/infiniband/ulp/ipoib/ipoib_multicast.c static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast) · line 715

Quoted 10 lines

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

 
+/*
+ * Check if the multicast group is sendonly. If so remove it from the maps
+ * and add to the remove list
+ */
+void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+				struct list_head *remove_list)
+{
+	/* Is this multicast ? */
+	if (*mgid == 0xff) {

Odd to see a mgid variable which is only u8?

How about "gid_prefix"?

Ira

Quoted 21 lines

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

+		struct ipoib_mcast *mcast = __ipoib_mcast_find(priv->dev, mgid);
+
+		if (mcast && test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags)) {
+			list_del(&mcast->list);
+			rb_erase(&mcast->rb_node, &priv->multicast_tree);
+			list_add_tail(&mcast->list, remove_list);
+		}
+	}
+}
+
 void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list)
 {
 	struct ipoib_mcast *mcast, *tmcast;

Signature · 7 lines

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

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

ira.weiny

2015-12-11 23:24 UTC #8 lore raw

Re: [PATCH 2/2] ipoib mcast sendonly join: Move multicast specific code out of ipoib_main.c.

In reply to a message not in the archive

Quoted 96 lines · ira. weiny

Quoted 16 lines · Christoph Lameter

Code cleanup to move multicast specific code that checks for a sendonly join to ipoib_multicast.c. This allows the removal of the export of __ipoib_mcast_find().
Signed-off-by Christoph Lameter <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib.h           |  3 ++-
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 13 +------------
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 21 ++++++++++++++++++++-
 3 files changed, 23 insertions(+), 14 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +0 -0
index 989c409..a13f48c 100644

drivers/infiniband/ulp/ipoib/ipoib.h void ipoib_path_iter_read(struct ipoib_path_iter *iter, · line 553

Quoted 6 lines

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

@@ -549,7 +549,8 @@ void ipoib_path_iter_read(struct ipoib_path_iter *iter,
 int ipoib_mcast_attach(struct net_device *dev, u16 mlid,
 		       union ib_gid *mgid, int set_qkey);
 void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list);
-struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid);
+void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+				struct list_head *remove_list);

I think I would rather see this called something like

ipoib_add_to_list_sendonly

Or something...

Calling it iboib_check* sounds like it should return a bool.

Quoted 51 lines

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

 int ipoib_init_qp(struct net_device *dev);
 int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca);

drivers/infiniband/ulp/ipoib/ipoib_main.c +1 ~~-12~~

index 483ff20..6b16428 100644
@@ -1150,7 +1150,6 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 	unsigned long flags;
 	int i;
 	LIST_HEAD(remove_list);
-	struct ipoib_mcast *mcast;
 	struct net_device *dev = priv->dev;
 
 	if (test_bit(IPOIB_STOP_NEIGH_GC, &priv->flags))

@@ -1179,18 +1178,8 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 							  lockdep_is_held(&priv->lock))) != NULL) {
 			/* was the neigh idle for two GC periods */
 			if (time_after(neigh_obsolete, neigh->alive)) {
-				u8 *mgid = neigh->daddr + 4;
 
-				/* Is this multicast ? */
-				if (*mgid == 0xff) {
-					mcast = __ipoib_mcast_find(dev, mgid);
-
-					if (mcast && test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags)) {
-						list_del(&mcast->list);
-						rb_erase(&mcast->rb_node, &priv->multicast_tree);
-						list_add_tail(&mcast->list, &remove_list);
-					}
-				}
+				ipoib_check_mcast_sendonly(priv, neigh->daddr + 4, &remove_list);
 
 				rcu_assign_pointer(*np,
 						   rcu_dereference_protected(neigh->hnext,

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

index 8acb420a..1158819 100644
@@ -153,7 +153,7 @@ static struct ipoib_mcast *ipoib_mcast_alloc(struct net_device *dev,
 	return mcast;
 }
 
-struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid)
+static struct ipoib_mcast *__ipoib_mcast_find(struct net_device *dev, void *mgid)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	struct rb_node *n = priv->multicast_tree.rb_node;

@@ -704,6 +704,25 @@ static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
 	return 0;
 }

drivers/infiniband/ulp/ipoib/ipoib_multicast.c static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast) · line 715

Quoted 10 lines

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

 
+/*
+ * Check if the multicast group is sendonly. If so remove it from the maps
+ * and add to the remove list
+ */
+void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+				struct list_head *remove_list)
+{
+	/* Is this multicast ? */
+	if (*mgid == 0xff) {

Odd to see a mgid variable which is only u8?

Quoted 1 line

How about "gid_prefix"?

Nevermind on this one, I misread this, it's a pointer to the full gid...

Ira

Quoted 20 lines

Ira

Quoted 17 lines

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

+		struct ipoib_mcast *mcast = __ipoib_mcast_find(priv->dev, mgid);
+
+		if (mcast && test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags)) {
+			list_del(&mcast->list);
+			rb_erase(&mcast->rb_node, &priv->multicast_tree);
+			list_add_tail(&mcast->list, remove_list);
+		}
+	}
+}
+
 void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list)
 {
 	struct ipoib_mcast *mcast, *tmcast;

Signature · 1 line

2.5.0

Quoted 8 lines

Quoted 4 lines
--
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

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

Christoph Lameter

author

2015-12-14 16:04 UTC #9 lore raw

Re: [PATCH 2/2] ipoib mcast sendonly join: Move multicast specific code out of ipoib_main.c.

In reply to a message not in the archive

Quoted 7 lines · ira.weiny

I think I would rather see this called something like
ipoib_add_to_list_sendonly
Or something...
Calling it iboib_check* sounds like it should return a bool.

Hmm... It only adds the multicast group if the check was successful.

How about

ipoib_check_and_add_mcast_sendonly()

Quoted 9 lines

drivers/infiniband/ulp/ipoib/ipoib_multicast.c static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast) · line 715
Quoted 5 lines
drivers/infiniband/ulp/ipoib/ipoib_multicast.c +5 -0
+void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+				struct list_head *remove_list)
+{
+	/* Is this multicast ? */
+	if (*mgid == 0xff) {
Odd to see a mgid variable which is only u8?
How about "gid_prefix"?

That is only used in the qib driver and there it is a field.

mgid is a pointer to the seres of bytes of the MGID and the first byte of that signifies multicast if 0xff....

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

Weiny, Ira

2015-12-14 16:07 UTC #11 lore raw

RE: [PATCH 2/2] ipoib mcast sendonly join: Move multicast specific code out of ipoib_main.c.

In reply to a message not in the archive

Quoted 9 lines · ira.weiny

Quoted 7 lines
I think I would rather see this called something like
ipoib_add_to_list_sendonly
Or something...

Quoted 7 lines

Quoted 1 line
Calling it iboib_check* sounds like it should return a bool.
Hmm... It only adds the multicast group if the check was successful.
How about
ipoib_check_and_add_mcast_sendonly()

Better.

Quoted 11 lines

Quoted 3 lines
Quoted 3 lines
+void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8
*mgid,
Quoted 7 lines
drivers/infiniband/ulp/ipoib/ipoib_multicast.c static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast) · line 715
Quoted 4 lines
drivers/infiniband/ulp/ipoib/ipoib_multicast.c +4 -0
+				struct list_head *remove_list)
+{
+	/* Is this multicast ? */
+	if (*mgid == 0xff) {
Odd to see a mgid variable which is only u8?

Quoted 6 lines

Quoted 1 line
How about "gid_prefix"?
That is only used in the qib driver and there it is a field.
mgid is a pointer to the seres of bytes of the MGID and the first byte of that signifies multicast if 0xff....

Understood, I misread the code at first.

Ira

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

Christoph Lameter

author

2015-12-14 16:24 UTC #13 lore raw

RE: [PATCH 2/2] ipoib mcast sendonly join: Move multicast specific code out of ipoib_main.c.

In reply to a message not in the archive

Quoted 5 lines · Ira

Quoted 3 lines
How about
=20
ipoib_check_and_add_mcast_sendonly()
Better.

Fixup patch:

Subject: ipoib: Fix up naming of ipoib_check_and_add_mcast_sendonly

Signed-off-by Christoph Lameter <[email protected]>

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

Index: linux/drivers/infiniband/ulp/ipoib/ipoib.h
===================================================================
@@ -549,7 +549,7 @@ void ipoib_path_iter_read(struct ipoib_p
 int ipoib_mcast_attach(struct net_device *dev, u16 mlid,
 		       union ib_gid *mgid, int set_qkey);
 void ipoib_mcast_remove_list(struct net_device *dev, struct list_head *remove_list);
-void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+void ipoib_check_and_add_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
 				struct list_head *remove_list);
 
 int ipoib_init_qp(struct net_device *dev);

drivers/infiniband/ulp/ipoib/ipoib_main.c +1 -1

Index: linux/drivers/infiniband/ulp/ipoib/ipoib_main.c
===================================================================
@@ -1179,7 +1179,7 @@ static void __ipoib_reap_neigh(struct ip
 			/* was the neigh idle for two GC periods */
 			if (time_after(neigh_obsolete, neigh->alive)) {
 
-				ipoib_check_mcast_sendonly(priv, neigh->daddr + 4, &remove_list);
+				ipoib_check_and_add_mcast_sendonly(priv, neigh->daddr + 4, &remove_list);
 
 				rcu_assign_pointer(*np,
 						   rcu_dereference_protected(neigh->hnext,

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

Index: linux/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
===================================================================
@@ -708,7 +708,7 @@ static int ipoib_mcast_leave(struct net_
  * Check if the multicast group is sendonly. If so remove it from the maps
  * and add to the remove list
  */
-void ipoib_check_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
+void ipoib_check_and_add_mcast_sendonly(struct ipoib_dev_priv *priv, u8 *mgid,
 				struct list_head *remove_list)
 {
 	/* Is this multicast ? */

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