akepner at sgi.com wrote:
> @@ -810,9 +810,6 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
> list_del(&neigh->list);
> if (neigh->ah)
> ipoib_put_ah(neigh->ah);
> - ipoib_neigh_free(dev, neigh);
> -
> - tx->neigh = NULL;
> }
>
> if (test_and_clear_bit(IPOIB_FLAG_INITIALIZED, &tx->flags)) {
Can't calling ipoib_put_ah() and keeping the neighbour alive cause neigh->ah
pointer become invalid? What about doing list_del(&neigh->list) twice?
> @@ -1316,11 +1313,22 @@ static void ipoib_cm_tx_start(struct work_struct *work)
> if (ret) {
> neigh = p->neigh;
> if (neigh) {
> + struct sk_buff *skb;
> neigh->cm = NULL;
> list_del(&neigh->list);
> if (neigh->ah)
> ipoib_put_ah(neigh->ah);
> - ipoib_neigh_free(dev, neigh);
> + *to_ipoib_neigh(neigh->neighbour) = NULL;
> + ipoib_dbg(priv, "%s: releasing ref to %pI6 "
> + "(refcnt: %d)\n", __func__,
> + neigh->dgid.raw,
> + atomic_read(&neigh->neighbour->refcnt));
> + neigh_release(neigh->neighbour);
> + while ((skb = __skb_dequeue(&neigh->queue))) {
> + ++dev->stats.tx_dropped;
> + dev_kfree_skb_any(skb);
> + }
> + kfree(neigh);
> }
> list_del(&p->list);
> kfree(p);
The piece of code that does '*to_ipoib_neigh(neigh->neighbour) = NULL' and releases
skb's will appear 3 times in ipoib. I think it can be put in a separate function.
> @@ -1343,7 +1351,23 @@ static void ipoib_cm_tx_reap(struct work_struct *work)
> spin_lock_irqsave(&priv->lock, flags);
>
> while (!list_empty(&priv->cm.reap_list)) {
> + struct ipoib_neigh *neigh;
> + struct sk_buff *skb;
> p = list_entry(priv->cm.reap_list.next, typeof(*p), list);
> + neigh = p->neigh;
> + if (neigh) {
> + *to_ipoib_neigh(neigh->neighbour) = NULL;
> + ipoib_dbg(priv, "%s: releasing ref to %pI6 "
> + "(refcnt: %d)\n", __func__, neigh->dgid.raw,
> + atomic_read(&neigh->neighbour->refcnt));
> + neigh_release(neigh->neighbour);
> + while ((skb = __skb_dequeue(&neigh->queue))) {
> + ++dev->stats.tx_dropped;
> + dev_kfree_skb_any(skb);
> + }
> + kfree(neigh);
> + }
> + p->neigh = NULL;
> list_del(&p->list);
> spin_unlock_irqrestore(&priv->lock, flags);
> netif_tx_unlock_bh(dev);
Couldn't this race with ipoib_neigh_cleanup()?
--Yossi