This patch worried me, because dropping the lock between
path_rec_start() failing and calling path_free() opens a window where
the path is in our path table with no locks held. Also dropping the
lock halfway through path_free() made me nervous too.
I thought a little about all this and came up with the following
patch. This moves the __path_add() out of path_rec_create() so that
it's OK to drop the lock in unicast_arp_send() before calling
path_free(). Also, it uses priv->lock to serialize the freeing of
IPoIB neighbour structs.
Do you see any problems with this?
Thanks,
Roland
--- infiniband/ulp/ipoib/ipoib_main.c (revision 1909)
+++ infiniband/ulp/ipoib/ipoib_main.c (working copy)
@@ -215,16 +215,25 @@ static int __path_add(struct net_device
return 0;
}
-static void __path_free(struct net_device *dev, struct ipoib_path *path)
+static void path_free(struct net_device *dev, struct ipoib_path *path)
{
struct ipoib_dev_priv *priv = netdev_priv(dev);
struct ipoib_neigh *neigh, *tn;
struct sk_buff *skb;
+ unsigned long flags;
while ((skb = __skb_dequeue(&path->queue)))
dev_kfree_skb_irq(skb);
+ spin_lock_irqsave(&priv->lock, flags);
+
list_for_each_entry_safe(neigh, tn, &path->neigh_list, list) {
+ /*
+ * It's safe to call ipoib_put_ah() inside priv->lock
+ * here, because we know that path->ah will always
+ * hold one more reference, so ipoib_put_ah() will
+ * never do more than decrement the ref count.
+ */
if (neigh->ah)
ipoib_put_ah(neigh->ah);
*to_ipoib_neigh(neigh->neighbour) = NULL;
@@ -232,11 +241,11 @@ static void __path_free(struct net_devic
kfree(neigh);
}
+ spin_unlock_irqrestore(&priv->lock, flags);
+
if (path->ah)
ipoib_put_ah(path->ah);
- rb_erase(&path->rb_node, &priv->path_tree);
- list_del(&path->list);
kfree(path);
}
@@ -248,15 +257,20 @@ void ipoib_flush_paths(struct net_device
unsigned long flags;
spin_lock_irqsave(&priv->lock, flags);
+
list_splice(&priv->path_list, &remove_list);
INIT_LIST_HEAD(&priv->path_list);
+
+ list_for_each_entry(path, &remove_list, list)
+ rb_erase(&path->rb_node, &priv->path_tree);
+
spin_unlock_irqrestore(&priv->lock, flags);
list_for_each_entry_safe(path, tp, &remove_list, list) {
if (path->query)
ib_sa_cancel_query(path->query_id, path->query);
wait_for_completion(&path->done);
- __path_free(dev, path);
+ path_free(dev, path);
}
}
@@ -361,8 +375,6 @@ static struct ipoib_path *path_rec_creat
path->pathrec.pkey = cpu_to_be16(priv->pkey);
path->pathrec.numb_path = 1;
- __path_add(dev, path);
-
return path;
}
@@ -422,6 +434,8 @@ static void neigh_add_path(struct sk_buf
(union ib_gid *)
(skb->dst->neighbour->ha + 4));
if (!path)
goto err;
+
+ __path_add(dev, path);
}
list_add_tail(&neigh->list, &path->neigh_list);
@@ -497,8 +511,12 @@ static void unicast_arp_send(struct sk_b
skb_push(skb, sizeof *phdr);
__skb_queue_tail(&path->queue, skb);
- if (path_rec_start(dev, path))
- __path_free(dev, path);
+ if (path_rec_start(dev, path)) {
+ spin_unlock(&priv->lock);
+ path_free(dev, path);
+ return;
+ } else
+ __path_add(dev, path);
} else {
++priv->stats.tx_dropped;
dev_kfree_skb_any(skb);
@@ -658,7 +676,7 @@ static void ipoib_set_mcast_list(struct
static void ipoib_neigh_destructor(struct neighbour *n)
{
- struct ipoib_neigh *neigh = *to_ipoib_neigh(n);
+ struct ipoib_neigh *neigh;
struct ipoib_dev_priv *priv = netdev_priv(n->dev);
unsigned long flags;
struct ipoib_ah *ah = NULL;
@@ -670,6 +688,7 @@ static void ipoib_neigh_destructor(struc
spin_lock_irqsave(&priv->lock, flags);
+ neigh = *to_ipoib_neigh(n);
if (neigh) {
if (neigh->ah)
ah = neigh->ah;
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
