[openib-general] [PATCH 2 of 2] repost: ipoib neigh issues

Michael S. Tsirkin mst at mellanox.co.il
Wed Jan 4 03:38:17 PST 2006


Hi!
I'm reposting the series since a bug in the second patch has been fixed.

Patch 2 of 2. Applied on top of ipoib_all_neigh_issues_1.patch

---

IPoIB uses neighbour ops->destructor to clean up struct ipoib_neigh,
but ignores the fact that multiple neighbour objects can share
the same ops structure, so setting it to NULL affects multiple neighbours.

Fix this, by tracking all ipoib_neigh objects, and only cleaning 
destructor after no neighbour is going to use it.
Note that ops structure isnt per device, so we track them in a global list.

Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>

Index: openib/drivers/infiniband/ulp/ipoib/ipoib_main.c
===================================================================
--- openib.orig/drivers/infiniband/ulp/ipoib/ipoib_main.c	2005-12-28 07:55:03.000000000 +0200
+++ openib/drivers/infiniband/ulp/ipoib/ipoib_main.c	2006-01-04 13:27:52.000000000 +0200
@@ -71,6 +71,9 @@ static const u8 ipv4_bcast_addr[] = {
 
 struct workqueue_struct *ipoib_workqueue;
 
+static spinlock_t ipoib_all_neigh_list_lock;
+static LIST_HEAD(ipoib_all_neigh_list);
+
 static void ipoib_add_one(struct ib_device *device);
 static void ipoib_remove_one(struct ib_device *device);
 
@@ -244,9 +247,8 @@ static void path_free(struct net_device 
 		 */
 		if (neigh->ah)
 			ipoib_put_ah(neigh->ah);
-		*to_ipoib_neigh(neigh->neighbour) = NULL;
-		neigh->neighbour->ops->destructor = NULL;
-		kfree(neigh);
+
+		ipoib_neigh_free(neigh);
 	}
 
 	spin_unlock_irqrestore(&priv->lock, flags);
@@ -474,7 +476,7 @@ static void neigh_add_path(struct sk_buf
 	struct ipoib_path *path;
 	struct ipoib_neigh *neigh;
 
-	neigh = kmalloc(sizeof *neigh, GFP_ATOMIC);
+	neigh = ipoib_neigh_alloc(skb->dst->neighbour);
 	if (!neigh) {
 		++priv->stats.tx_dropped;
 		dev_kfree_skb_any(skb);
@@ -482,8 +484,6 @@ static void neigh_add_path(struct sk_buf
 	}
 
 	skb_queue_head_init(&neigh->queue);
-	neigh->neighbour = skb->dst->neighbour;
-	*to_ipoib_neigh(skb->dst->neighbour) = neigh;
 
 	/*
 	 * We can only be called from ipoib_start_xmit, so we're
@@ -526,11 +526,8 @@ static void neigh_add_path(struct sk_buf
 	return;
 
 err:
-	*to_ipoib_neigh(skb->dst->neighbour) = NULL;
 	list_del(&neigh->list);
-	neigh->neighbour->ops->destructor = NULL;
-	kfree(neigh);
-
+	ipoib_neigh_free(neigh);
 	++priv->stats.tx_dropped;
 	dev_kfree_skb_any(skb);
 
@@ -757,8 +754,7 @@ static void ipoib_neigh_destructor(struc
 		if (neigh->ah)
 			ah = neigh->ah;
 		list_del(&neigh->list);
-		*to_ipoib_neigh(n) = NULL;
-		kfree(neigh);
+		ipoib_neigh_free(neigh);
 	}
 
 	spin_unlock_irqrestore(&priv->lock, flags);
@@ -767,23 +763,45 @@ static void ipoib_neigh_destructor(struc
 		ipoib_put_ah(ah);
 }
 
-static int ipoib_neigh_setup(struct neighbour *neigh)
+struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neighbour)
 {
+	struct ipoib_neigh *neigh;
+
+	neigh = kmalloc(sizeof *neigh, GFP_ATOMIC);
+	if (!neigh)
+		return NULL;
+
+	neigh->neighbour = neighbour;
+	*to_ipoib_neigh(neighbour) = neigh;
+
 	/*
 	 * Is this kosher?  I can't find anybody in the kernel that
 	 * sets neigh->destructor, so we should be able to set it here
 	 * without trouble.
 	 */
-	neigh->ops->destructor = ipoib_neigh_destructor;
-
-	return 0;
+	spin_lock(&ipoib_all_neigh_list_lock);
+	list_add_tail(&neigh->all_neigh_list, &ipoib_all_neigh_list);
+	neigh->neighbour->ops->destructor = ipoib_neigh_destructor;
+	spin_unlock(&ipoib_all_neigh_list_lock);
+	return neigh;
 }
 
-static int ipoib_neigh_setup_dev(struct net_device *dev, struct neigh_parms *parms)
+void ipoib_neigh_free(struct ipoib_neigh *neigh)
 {
-	parms->neigh_setup = ipoib_neigh_setup;
+	struct ipoib_neigh *n;
 
-	return 0;
+	spin_lock(&ipoib_all_neigh_list_lock);
+	list_del(&neigh->all_neigh_list);
+
+	list_for_each_entry(n, &ipoib_all_neigh_list, all_neigh_list)
+		if (n->neighbour->ops == neigh->neighbour->ops)
+			goto found;
+
+	neigh->neighbour->ops->destructor = NULL;
+found:
+	spin_unlock(&ipoib_all_neigh_list_lock);
+	*to_ipoib_neigh(neigh->neighbour) = NULL;
+	kfree(neigh);
 }
 
 int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port)
@@ -859,7 +877,6 @@ static void ipoib_setup(struct net_devic
 	dev->tx_timeout 	 = ipoib_timeout;
 	dev->hard_header 	 = ipoib_hard_header;
 	dev->set_multicast_list  = ipoib_set_mcast_list;
-	dev->neigh_setup         = ipoib_neigh_setup_dev;
 
 	dev->watchdog_timeo 	 = HZ;
 
@@ -1142,6 +1159,8 @@ static int __init ipoib_init_module(void
 		goto err_fs;
 	}
 
+	spin_lock_init(&ipoib_all_neigh_list_lock);
+
 	ret = ib_register_client(&ipoib_client);
 	if (ret)
 		goto err_wq;
Index: openib/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
===================================================================
--- openib.orig/drivers/infiniband/ulp/ipoib/ipoib_multicast.c	2006-01-04 10:39:29.000000000 +0200
+++ openib/drivers/infiniband/ulp/ipoib/ipoib_multicast.c	2006-01-04 10:39:30.000000000 +0200
@@ -111,9 +111,7 @@ static void ipoib_mcast_free(struct ipoi
 		 */
 		if (neigh->ah)
 			ipoib_put_ah(neigh->ah);
-		*to_ipoib_neigh(neigh->neighbour) = NULL;
-		neigh->neighbour->ops->destructor = NULL;
-		kfree(neigh);
+		ipoib_neigh_free(neigh);
 	}
 
 	spin_unlock_irqrestore(&priv->lock, flags);
@@ -719,13 +717,11 @@ out:
 		if (skb->dst            &&
 		    skb->dst->neighbour &&
 		    !*to_ipoib_neigh(skb->dst->neighbour)) {
-			struct ipoib_neigh *neigh = kmalloc(sizeof *neigh, GFP_ATOMIC);
+			struct ipoib_neigh *neigh = ipoib_neigh_alloc(skb->dst->neighbour);
 
 			if (neigh) {
 				kref_get(&mcast->ah->ref);
 				neigh->ah  	= mcast->ah;
-				neigh->neighbour = skb->dst->neighbour;
-				*to_ipoib_neigh(skb->dst->neighbour) = neigh;
 				list_add_tail(&neigh->list, &mcast->neigh_list);
 			}
 		}
Index: openib/drivers/infiniband/ulp/ipoib/ipoib.h
===================================================================
--- openib.orig/drivers/infiniband/ulp/ipoib/ipoib.h	2005-12-28 07:55:03.000000000 +0200
+++ openib/drivers/infiniband/ulp/ipoib/ipoib.h	2006-01-04 10:39:30.000000000 +0200
@@ -214,6 +214,7 @@ struct ipoib_neigh {
 	struct neighbour   *neighbour;
 
 	struct list_head    list;
+	struct list_head    all_neigh_list;
 };
 
 static inline struct ipoib_neigh **to_ipoib_neigh(struct neighbour *neigh)
@@ -222,6 +223,9 @@ static inline struct ipoib_neigh **to_ip
 					(offsetof(struct neighbour, ha) & 4));
 }
 
+struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neigh);
+void ipoib_neigh_free(struct ipoib_neigh *neigh);
+
 extern struct workqueue_struct *ipoib_workqueue;
 
 /* functions */
-- 
MST



More information about the general mailing list